Running all the PHPUnit tests using PHPUnit results in failure [#2502477]

Drupal\Tests\Core\Render\RendererPlaceholdersTest fails since #2273925: Ensure #markup is XSS escaped in Renderer::doRender() was committed. Looks a like we have something static (SafeMarkup perhaps) getting in the way.

1) Drupal\Tests\Core\Render\RendererPlaceholdersTest::testUncacheableParent with data set #2 (array('<drupal-render-placeholder ca...older>', array(array('bar'), array(array(array('Drupal\Tests\Core\Render\Plac...llback', array('alpaca')))))), array('alpaca'), false, array())
Output is overridden.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-<p>#cache disabled</p><p>This is a rendered placeholder!</p>
+<p>#cache disabled</p>

/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php:240

2) Drupal\Tests\Core\Render\RendererPlaceholdersTest::testUncacheableParent with data set #3 (array('<drupal-render-placeholder ca...older>', array(array('bar'), array(array(array('Drupal\Tests\Core\Render\Plac...llback', array('alpaca')), array(array('placeholder', 'output', 'can', 'be', 'render', 'cached', 'too')))))), array('alpaca'), array('placeholder', 'output', 'can', 'be', 'render', 'cached', 'too'), array('<p>This is a rendered placeholder!</p>', array(array('alpaca')), array(array(), array(), -1)))
Output is overridden.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-<p>#cache disabled</p><p>This is a rendered placeholder!</p>
+<p>#cache disabled</p>

/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php:240

3) Drupal\Tests\Core\Render\RendererPlaceholdersTest::testCacheableParent with data set #2 (array('<drupal-render-placeholder ca...older>', array(array('bar'), array(array(array('Drupal\Tests\Core\Render\Plac...llback', array('llama')))))), array('llama'), false, array())
Output is overridden.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-<p>#cache enabled, GET</p><p>This is a rendered placeholder!</p>
+<p>#cache enabled, GET</p>

/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php:286

4) Drupal\Tests\Core\Render\RendererPlaceholdersTest::testCacheableParent with data set #3 (array('<drupal-render-placeholder ca...older>', array(array('bar'), array(array(array('Drupal\Tests\Core\Render\Plac...llback', array('llama')), array(array('placeholder', 'output', 'can', 'be', 'render', 'cached', 'too')))))), array('llama'), array('placeholder', 'output', 'can', 'be', 'render', 'cached', 'too'), array('<p>This is a rendered placeholder!</p>', array(array('llama')), array(array(), array(), -1)))
Output is overridden.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-<p>#cache enabled, GET</p><p>This is a rendered placeholder!</p>
+<p>#cache enabled, GET</p>

/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php:286

5) Drupal\Tests\Core\Render\RendererPlaceholdersTest::testCacheableParentWithPostRequest with data set #2 (array('<drupal-render-placeholder ca...older>', array(array('bar'), array(array(array('Drupal\Tests\Core\Render\Plac...llback', array('elk')))))), array('elk'), false, array())
Output is overridden.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-<p>#cache enabled, POST</p><p>This is a rendered placeholder!</p>
+<p>#cache enabled, POST</p>

/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php:354

6) Drupal\Tests\Core\Render\RendererPlaceholdersTest::testCacheableParentWithPostRequest with data set #3 (array('<drupal-render-placeholder ca...older>', array(array('bar'), array(array(array('Drupal\Tests\Core\Render\Plac...llback', array('elk')), array(array('placeholder', 'output', 'can', 'be', 'render', 'cached', 'too')))))), array('elk'), array('placeholder', 'output', 'can', 'be', 'render', 'cached', 'too'), array('<p>This is a rendered placeholder!</p>', array(array('elk')), array(array(), array(), -1)))
Output is overridden.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-<p>#cache enabled, POST</p><p>This is a rendered placeholder!</p>
+<p>#cache enabled, POST</p>

/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php:354

Comment	File	Size	Author
#4	2502477.4.patch	1.49 KB	alexpott
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 8 June 2015 at 16:31

Removing the changes #2273925: Ensure #markup is XSS escaped in Renderer::doRender() made to Drupal\Tests\Core\Render\RendererPlaceholdersTest does not fix the test. Resetting the SafeMarkup::$safeStrings before or after each PHPUnit test does not work. Hmmm... this is going to be tricky.

Comment #2

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 8 June 2015 at 17:10

So we're calling SafeMarkup::set() 3312 times before we've even run a single test :)

Comment #3

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 8 June 2015 at 18:02

So much sleuthing has lead me to Drupal\Tests\Core\Form\FormCacheTest and it's resetSafeMarkup() method.

The problem here is that all providers are called before running any tests so this clears the affect of any SafeMarkup::set()'s they do. The obvious thing to do here is to make SafeMarkup reset in the setUp of all PHPUnit tests - this however is going to be an awful lot of work because quite a few tests rely on the side effect of safe markup being run then.

Comment #4

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 8 June 2015 at 18:11

Status:

Active

» Needs review

File	Size
2502477.4.patch	1.49 KB

This is the quick fix. I think we should open a followup to implement a SafeMarkup reset on all PHPUnit tests since this is one massive side effect going on here. I would like to go forward with the quickfix because atm this breaks my committing workflow and potential allows us to introduce other side effect breaks into PHPUnit if I change my workflow.

Comment #5

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 8 June 2015 at 18:29

Whoops, sorry about that. I had no idea *all* providers ran before *all* tests.

Comment #6

joelpittet

English

Vancouver

CreditAttribution: joelpittet commented 8 June 2015 at 19:35

Status:

Needs review

» Reviewed & tested by the community

+++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
@@ -15,6 +15,8 @@
+ * @runTestsInSeparateProcesses
+ * @preserveGlobalState disabled

That seems like a nice way to keep this separated. Good to know!

Comment #7

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 8 June 2015 at 20:43

@joelpittet well it is and it is not. In fact we shouldn't be relying on side effects in tests. Opened #2502645: Don't use SafeMarkup in data providers in PHPUnit - massive side effects to do the followup work to properly resolve this.

Comment #8

8 June 2015 at 20:47

catch committed 74c8c59 on 8.0.x

Issue #2502477 by alexpott: Running all the PHPUnit tests using PHPUnit...

Comment #9

catch

he/him

English

CreditAttribution: catch commented 8 June 2015 at 20:48

Status:

Reviewed & tested by the community

» Fixed

Committed/pushed to 8.0.x, thanks!

Note that #2488538: Add SafeMarkup::remove() to free memory from marked strings when they're printed could end up removing the static as well (although does not yet).

Comment #10

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan as a volunteer commented 8 June 2015 at 21:37

Thanks @alexpott, fwiw I did run the render group on the cli in #2273925: Ensure #markup is XSS escaped in Renderer::doRender(), but clearly we had bleeding issues.