While working on #352951: Make JS & CSS Preprocessing Pluggable, I got very much fed up with the extreme slowness of CascadingStylesheetsTest.php.

Difference

  • Before: 281 seconds
  • After: 8 seconds
  • (That's 35.125 times faster, or only 2.85% of the original test running time.)

It used WebTestBase for historical reasons, while it could easily use DrupalUnitTestBase now.

Ideally, this would be a PHPUnit test, but that's not technically possible right now. This will at least make it a whole lot faster, while retaining the same test coverage.

Changes

I changed as little as possible:

  • DrupalWebTestBase -> DrupalUnitTestBase
  • Removed testRenderInlineFullPage() since it was rather pointless: it tests adding inline CSS, but that's already tested in two other tests, and in fact to do so it used the PHP filter of all things…
  • Instead of relying on a certain page callback in common_test module, which would indeed need WebTestBase, I just moved the relevant parts of common_test.module into the test itself (2 lines of code).
CommentFileSizeAuthor
#20 tests.png91.39 KBdawehner
#1 cascadingstylesheetstest_35_times_faster-2026255-1.patch3.41 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: ckeditor.module » system.module
Status: Active » Needs review
StatusFileSize
new cascadingstylesheetstest_35_times_faster-2026255-1.patch3.41 KB

Status: Needs review » Needs work

The last submitted patch, cascadingstylesheetstest_35_times_faster-2026255-1.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Testbot failed to run 4 tests.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

#1: cascadingstylesheetstest_35_times_faster-2026255-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, cascadingstylesheetstest_35_times_faster-2026255-1.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
mkdir(): No space left on device

This is sad.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

#1: cascadingstylesheetstest_35_times_faster-2026255-1.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
commented

Maybe people would disagree, but I think if you manage to run your code in DrupalUnitTest it is a step forward to proper unit tests.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.phpundefined
@@ -8,19 +8,19 @@
+  public static $modules = array('language', 'system');

I try to understand why you need system. comment_test at least did not needed system module.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I think if you manage to run your code in DrupalUnitTest it is a step forward to proper unit tests.

Yes! Totally agreed.

I try to understand why you need system. comment_test at least did not needed system module.

It's necessary for this:

/**
 * Implements hook_element_info().
 */
function system_element_info() {
  …
  $types['styles'] = array(
    '#items' => array(),
    '#pre_render' => array('drupal_pre_render_styles'),
    '#group_callback' => 'drupal_group_css',
    '#aggregate_callback' => 'drupal_aggregate_css',
  );
  …
}

Without system module, the pre render callback (and other callbacks) are not registered, hence drupal_render(array('#type' => 'styles', '#items' => array(…)) will render to the empty string.

(Also: WebTestBase always includes system module implicitly of course :))

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Similar improvements for JavaScriptTest: #2026349: Make JavaScriptTest 3200% faster.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Test suite performance

.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.phpundefined
@@ -225,9 +192,14 @@ function testAlter() {
+    $this->assertTrue(strpos($styles, str_replace('&', '&', $css_with_query_string)), 'Query string not escaped on a URI.');

Can't we just use htmlspecialchars() here instead of the str_replace()?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yes, we can, but there is no point in doing so, because we control the input, and the input uses only "&". If you find that necessary, I'll reroll.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Well, I don't care to be honest.

sdboyer’s picture

sdboyer commented

yeah, this always should have been a unit test. this is great. +1 on the RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Nice...

Committed 104b436 and pushed to 8.x. Thanks!

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

Awesome improvement, but why is it added to the Pluggable CSS & JS preprocessing change record? Let's instead create a separate change record "Vastly improved performance for many Simpletests" or so. It's no API change, but taken together it's very much noteworthy.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#17: we don't create change records for faster tests. I just referenced it in the pluggable CSS/JS change record because as part of working on that, I also did this. If it bothers you, feel free to remove it from that change record. You're right that it's strictly not functionally related.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

Added it to the New DrupalUnitTestBase change record instead.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new tests.png91.39 KB

I am sorry but please let not add additional drupal unit tests to this change note. The class hierachy looks kind of cool.
tests.png

Automatically closed -- issue fixed for 2 weeks with no activity.