Make CascadingStylesheetsTest 3500% faster [#2026255]

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).

Comment	File	Size	Author
#20	tests.png	91.39 KB	dawehner
#1	cascadingstylesheetstest_35_times_faster-2026255-1.patch	3.41 KB	Wim Leers
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 June 2013 at 15:36

Component:	ckeditor.module	» system.module
Status:	Active	» Needs review

File	Size
cascadingstylesheetstest_35_times_faster-2026255-1.patch	3.41 KB

Comment #2

23 June 2013 at 16:59

Status:

Needs review

» Needs work

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

Comment #3

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 June 2013 at 17:49

Testbot failed to run 4 tests.

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 June 2013 at 17:49

Status:

Needs work

» Needs review

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

Comment #5

23 June 2013 at 19:06

Status:

Needs review

» Needs work

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

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 June 2013 at 19:10

Status:

Needs work

» Needs review

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

Comment #8

dawehner

German

CreditAttribution: dawehner commented 23 June 2013 at 20:38

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.

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 June 2013 at 20:43

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 :))

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 June 2013 at 20:45

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

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 June 2013 at 21:03

Issue tags:

+Test suite performance

Comment #12

dawehner

German

CreditAttribution: dawehner commented 24 June 2013 at 15:34

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

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

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 24 June 2013 at 18:14

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.

Comment #14

dawehner

German

CreditAttribution: dawehner commented 24 June 2013 at 23:34

Status:

Needs review

» Reviewed & tested by the community

Well, I don't care to be honest.

Comment #15

sdboyer CreditAttribution: sdboyer commented 25 June 2013 at 14:49

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

Comment #16

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 25 June 2013 at 15:30

Status:

Reviewed & tested by the community

» Fixed

Nice...

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

Comment #17

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 6 July 2013 at 21:48

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.

Comment #18

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 7 July 2013 at 06:57

#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.