Fix risky phpunit tests

Yeah, we don't have any risky tests :)

ROFL

:-)

+1 for the concept. Don't have time for a proper review ATM.

It looks like the way to repro this is to:

• Apply the patch here: https://www.drupal.org/node/2105583#comment-10167708
• Run PHPUnit cd core;./vendor/bin/phpunit -v. (Include -v to get PHPUnit to tell you which tests are risky.)
• Then you apply the patch in #3, run PHPUnit again, and the risky notices go away.

Which is what happens, so yay, mission accomplished.

Some review stuff:

1. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/SpecialAttributesRouteSubscriberTest.php
   @@ -84,6 +84,7 @@ public function testOnRouteBuildingValidVariables(Route $route) {
        $route_collection->add('test', $route);
        $event = new RouteBuildEvent($route_collection, 'test');
        $this->specialAttributesRouteSubscriber->onAlterRoutes($event);
   +    $this->assertTrue(TRUE, 'No errors where thrown');
   

   Not really a test of much, is it? You've swapped one kind of risky test for another. :-)

   Rather than assertTrue(TRUE), we should ignore $this->specialAttributesRouteSubscriber and mock our own. That way we can set expectations for all the stuff SpecialAttributesRouteSubscriber::onAlterRoutes() does.

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
   @@ -140,6 +145,10 @@ protected function setUp() {
   +  public function tearDown() {
   +    unset($this->currentUserRole);
   +  }
   +
   

   currentUserRole is a string... Why unset it? It's about to go out of scope anyway.

#2364467: SpecialAttributesRouteSubscriber::onAlterRoutes() doesn't return a value. Nice. An issue I'd forgotten I made, lost to the ravages of antiquity, reborn to new life though the use of the search function!

This looks good, and we can then argue about mocking in that other issue. :-)

Fixed my remarks so this can stay at RTBC:

1. +++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
   @@ -434,7 +434,8 @@ public function testScalarLazybuilderCallbackContext() {
   -    $this->renderer->renderRoot($element);
   +    $result = $this->renderer->renderRoot($element);
   +    $this->assertEquals('<p>This is a rendered placeholder!</p>', (string) $result);
   

   These changes don't belong here. That's not what the test is testing. Should be reverted.

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
   @@ -80,6 +80,11 @@ class RendererTestBase extends UnitTestCase {
      /**
   +   * @var string
   +   */
   +  protected $currentUserRole;
   

   Missing docs.

@neclimdul: huh? That test only is the inverse of RendererPlaceholdersTest::testInvalidLazyBuilderArguments(): it asserts that no exceptions are thrown when only scalar arguments are specified for the #lazy_builder. It should not assert any of the output.

Where the risk there? There's no global involved.

The risky part is that there are no assertions in that test. There is no expected behavior, since we can't specify that no exception was thrown.

One solution is to decide on some behavior to test, and having it match the documented return type and value is reasonable and it gives us information in the future if things change.

Another way to do it would be a try/catch block with the catch failing. This more explicitly says what you're testing, so it might be better.

Or, you know.. Combine the two. :-)

This patch fixes all the risky tests except for SpecialAttributesRouteSubscriberTest::testOnRouteBuildingValidVariables(), which is marked as risky after applying the patch at https://www.drupal.org/node/2105583#comment-10176962

Ahhh! That makes sense. Thanks :) Sorry for missing that!

Test only change. Committed 4710b09 and pushed to 8.0.x. Thanks!