Convert existing drupal_render() KernelTestBase tests to PHPUnit tests

Now unblocked :)

For the testbot.

@dawehner is this part of a META?

Yay! Once #2379741: Add Renderer::getCacheableRenderArray() to encapsulate which data is needed for caching a render array and have views use it is committed, this will be able to go further again.

dawehner++

Reviewed the code and reviewed the porting of only the testDrupalRenderBasics() test. Attached is a reroll that already fixes all nitpicks and slightly improves some comments, to better convey which tests cases form logical groups, hence making it more doable to compare the old test with the new test; otherwise it's hard to guarantee that I didn't overlook something.

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -311,7 +311,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -    if (isset($elements['#cache'])) {
   +    if (isset($elements['#cache']) && $elements['#cache'] !== ['tags' => []]) {
   

   I've long considered adding this optimization also. +1 to this.

   Perhaps update the preceding comment, from if #cache is set to if caching is enabled

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -142,7 +142,7 @@ public function render(&$elements, $is_root_call = FALSE) {
   -      if (is_string($elements['#access_callback']) && strpos($elements['#access_callback'], '::') === FALSE) {
   +      if (is_string($elements['#access_callback']) && strpos($elements['#access_callback'], '::') !== FALSE) {
   
   @@ -197,7 +197,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -        if (is_string($callable) && strpos($callable, '::') === FALSE) {
   +        if (is_string($callable) && strpos($callable, '::') !== FALSE) {
   
   @@ -457,7 +457,7 @@ protected function processPostRenderCache(array &$elements) {
   -        if (strpos($callback, '::') === FALSE) {
   +        if (strpos($callback, '::') !== FALSE) {
   

   These changes are wrong.

   Run this little test script to convince yourself:

   
   $callbacks = [
     'service:method',
     'class::method',
   ];
   foreach ($callbacks as $callback) {
     print 'Controller resolver needs to be called?' . "\n";
     var_dump(strpos($callback, '::') === FALSE);
   }
   

3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1131 @@
   +    if (isset($setup_code)) {
   +      $setup_code = $setup_code->bindTo($this);
   +      $setup_code();
   +    }
   

   Wow. Never seen this before! Suddenly I understand why you like this issue so much :)

   A bit of documentation would help though. What about: Optionally, a closure can be provided that makes the necessary preparations to test the provided case. We bind the closure to $this, to make it run as if the mocks it creates were created right here.

4. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1131 @@
   +    $setup_code_type_link = function() {
   

   Why the "link" part in this variable name? It makes sense in the next one, but not here.

5. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1131 @@
   +      '#url' => 'http://drupal.org',
   ...
   +      '#url' => 'http://drupal.org',
   

   The deleted code said Url::fromUri('http:/drupal.org').

   Why this change?

6. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1131 @@
   +  public function testRenderWithPresetAccess($access) {
   ...
   +  public function testRenderWithAccessPropertyAndCallback($access) {
   

   "Preset" vs "Property"; let's standardize on "Property"

7. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1131 @@
   +  public function testRenderWithAccessCallbackCallable($access) {
   

   "AccessCallbackCallable" seems a bit much, why isn't just "AccessCallback" sufficient?

8. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1131 @@
   +  protected function setupThemeContainer($matcher = NULL) {
   +    $this->themeManager->expects($matcher ?: $this->at(1))
   

   Clever!

Wow, great progress!

RE: that last failure in RendererTest:

• HEAD: - $output = drupal_render_root($element);
• patch: $output = $this->renderer->render($element);
• should be: $output = $this->renderer->renderRoot($element);

:)

Now reviewing!

Continuing the reviewing of tests in my next comment. Wanted to post this because otherwise the interdiff will become unwieldy.

The included interdiff shows all changes, but the topical interdiffs show the steps I took to get there.

Code changes review

1. +++ b/core/includes/common.inc
   @@ -1366,25 +1366,7 @@ function show(&$element) {
    function drupal_render_cache_generate_placeholder($callback, array &$context) {
   

   Could use an @deprecated.

   Fixed.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -344,7 +345,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    if (isset($elements['#cache']) && $elements['#cache'] !== ['tags' => []]) {
   

   Ever since #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations, this should actually be ['contexts' => [], 'tags' => []]. But, what would be better, is if we'd instead check if either #cache['cid'] or #cache['keys'] is set. Because if either of those is set, then we *actually* want to do caching, hence that won't change in the future.

   (E.g. when max-age is added to cache metadata.)

   Fixed.

3. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -348,4 +348,29 @@ public static function mergeBubbleableMetadata(array $a, array $b);
   +   * This is used by drupal_pre_render_render_cache_placeholder() to generate
   +   * placeholders, but should also be called by #post_render_cache callbacks that
   +   * want to replace the placeholder with the final markup.
   

   This is only copied but it's actually wrong (it was right at one point but then wasn't updated when that #pre_render callback was removed).

   Fixed.

4. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -348,4 +348,29 @@ public static function mergeBubbleableMetadata(array $a, array $b);
   +  public function generateCachePlaceholder($callback, array &$context);
   

   Given #2364303: [meta] Formalize and define usage of #post_render_cache and the fact that we don't want break this API once it is introduced to minimize disruption… we might want to check with Fabianx which name makes most sense. I suspect just generatePlaceholder() will be better.

   (It's only a moving around of code, but technically this is also an API addition to this interface, as you will probably agree.)

Moved tests review

The following were not moved to the PHPUnit test:

1. testDrupalRenderThemePreprocessAttached(): because it requires a module to be enabled

1. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +  public function testRenderBasic($build, $expected, callable $setup_code = NULL) {
   ...
   +  public function providerTestRenderBasic() {
   

   This only had code formatting nitpicks, comment nitpicks and missing @covers. Last but not least: it was very difficult to find your way through this long list of test cases, because they actually have logical/semantical grouping, but it's not clear. I added "Part X" comments in between to clarify the grouping.

   Fixed all that.

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +    // Basic #markup based renderable array.
   +    $data[] = [
   +      ['#markup' => 'foo']
   +    , 'foo'];
   +
   +    // Test handling of #children and child renderable elements.
   +    // #theme is not set, #children is not set and the array has children.
   +    $data[] = [
   +      [
   +        'child' => ['#markup' => 'bar'],
   +      ], 'bar'];
   +    // #theme is not set, #children is set but empty and the array has
   +    // children.
   +    $data[] = [
   +      [
   +        '#children' => '',
   +        'child' => ['#markup' => 'bar'],
   +      ], 'bar'
   +    ];
   +    // #theme is not set, #children is not empty and will be assumed to be the
   +    // rendered child elements even though the #markup for 'child' differs.
   +    $data[] = [
   +      [
   +        '#children' => 'foo',
   +        'child' => ['#markup' => 'bar'],
   +      ], 'foo'
   +    ];
   

   … the one other change to the basic tests that I made is the moving of these new (! — that's great1) test cases to the top of the test, where they have a more logical place.

   Why? The first bunch of test cases specifically test without the theme system, and so does this.

3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +   * @covers ::render
   +   */
   +  public function testRenderSorting() {
   ...
   +   * @covers ::render
   +   */
   +  public function testRenderSortingWithSetHashSorted() {
   

   These also cover ::doRender(), same goes for every other @covers ::render; adding @covers ::doRender in all those places.

4. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +  public function testRenderWithPresetAccess($access) {
   ...
   +  public function testRenderWithAccessCallbackCallable($access) {
   ...
   +  public function testRenderWithAccessPropertyAndCallback($access) {
   ...
   +  public function testRenderWithAccessControllerResolved($access) {
   

   Cleaned these up: their matching assert method now has docs, and sets '#markup' => 'test', so that not each of these has to do that.

5. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +  public function testRenderTwice() {
   +    $build = [
   +      '#markup' => 'test',
   +    ];
   +
   +    $this->assertEquals('test', $this->renderer->render($build));
   +    // @todo This behaviour is odd ...
   +    $this->assertEquals('', $this->renderer->render($build));
   +  }
   

   This is a new test. Great.

   It's not actually that odd. The first time, #printed = TRUE is set. Hence the second time, we don't get any output.

   Updated this test accordingly.

6. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +  public function testRenderWithoutThemeArguments() {
   

   In HEAD, the "children attached" test comes before this. So I moved it back up here. It wasn't in a better (more logical) place in this patch, so we might as well keep these things in sync.

7. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +    $cache = new MemoryBackend('render');
   +    $this->cacheFactory->expects($this->any())
   +      ->method('get')
   +      ->with('render')
   +      ->willReturn($cache);
   ...
   +    $cache = new MemoryBackend('render');
   +    $this->cacheFactory->expects($this->any())
   +      ->method('get')
   +      ->willReturn($cache);
   

   In ::testRenderCache & ::testDrupalRenderChildrenAttached, these can be replaced with calls to the ::setupMemoryCache helper.

   Fixed.

8. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +  public function testDrupalRenderChildrenAttached() {
   

   This is not specific to children or #attached, what this actually tests, is bubbling without #pre_render. Renamed as such: testRenderBubblingWithoutPreRender.

   And renamed the existing testRenderBubbling to testRenderBubblingWithPrerender.

   So actually, disregard the prior comment about the "children attached" test function; this *does* have a better place: together with the other bubbling tests.

9. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1496 @@
   +    // Mock the cache contexts.
   +    $this->cacheContexts->expects($this->any())
   +      ->method('convertTokensToKeys')
   +      ->willReturnArgument(0);
   

   This mock isn't necessary in this test (children attached still).

   Removed.

10. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -0,0 +1,1496 @@
    +  protected function drupalRenderPostCacheCacheElement() {
    

    Renamed this to generatePostRenderCacheElement and added docs.

11. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -0,0 +1,1496 @@
    +    $this->setupNullCache();
    ...
    +  protected function setupNullCache() {
    +    $this->cacheFactory->expects($this->any())
    +      ->method('get')
    +      ->willReturn(new NullBackend('render'));
    +  }
    

    These are wrong. The intent here was to capture in the test assertions that nothing gets written to the cache. But it's still possible to write things to the null cache, you just can't get anything out of it.

    Renamed it to setUpUnusedCache and changed the assertion to match the intent.

12. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -0,0 +1,1496 @@
    +  public function testPostRenderCacheWithCacheMiss() {
    

    s/WithCacheMiss/WithColdCache/

13. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -0,0 +1,1496 @@
    +  public function testPostRenderCacheWithPostRequest() {
    ...
    +    $this->setupMemoryCache();
    ...
    +    // POST request: Ensure no data was cached.
    +    $cached_element = $this->memoryCache->get('post_render_cache_test_POST');
    +    $this->assertFalse($cached_element, 'No data is cached because this is a POST request.');
    

    Changing this to use ::setUpUnusedCache captures the same intent, but with simpler test code. It's more in line with PHPUnit tests.

The goal of this issue is to turn as much as possible of RenderTest into unit tests. Because unit tests allow us to truly test the Renderer, in isolation.

But since it is so very complex — it's one of the most complex pieces of code in Drupal core — I feel it makes more sense to spread the test cases across multiple files. One file with test cases that cover a certain aspect. That will make this much easier to review, and also much easier to maintain, and most importantly: much easier for other people to start to understand the renderer (and the involved complexities).

After all, RendererTest in its current incarnation is a whopping 1500 LoC. Renderer itself is only ~600!

Discussed with dawehner & Fabianx and they were generally +1, so doing that in the next reroll.

EDIT: to clarify: this is a pre-existing problem, not something introduced by this patch. But if we're doing this work to make the render system more maintainable, we might as well make the unit tests themselves maintainble too!

1. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1523 @@
   +  public function testRenderRecursivePostRenderCache() {
   

   This is a special case of #post_render_cache, yet it is listed before all other #post_render_cache tests. Does not make sense. Moved.

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1523 @@
   +    list ($test_element, $context) = $this->generatePostRenderCacheElement();
   ...
   +    list ($test_element, $context) = $this->generatePostRenderCacheElement();
   

   Here and elsewhere, we don't want the space: s/list (/list(/. Fixed.

3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -0,0 +1,1523 @@
   +  protected function setupPostRenderCallbacks() {
   +    $post_render_callback_recursive = new PostRenderCacheRecursion($this->renderer);
   +    $post_render_callback = new PostRenderCache($this->renderer);
   +    return [[$post_render_callback_recursive, 'callback'], [$post_render_callback, 'callback']];
   +  }
   

   This doesn't do anything. Removed it, and all its invocations.

This doesn't change a single comment, or method name, or variable name. It only spreads RendererTest across the following files:

1. RendererTestBase (170 LoC)
2. RendererBubblingTest (200 LoC)
3. RendererPostRenderCacheTest (470 LoC)
4. RendererRenderPlainTest (150 LoC)
5. and still RendererTest (600 LoC), which now only contains the basics.

This also lets us quite clearly see where the complexities lie (or perhaps where we are lacking test coverage).

Now going back to actually reviewing again.

RendererTest review: finished, no more complaints. Next: the 3 remaining test files.

+++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
@@ -0,0 +1,172 @@
+  protected function setupGetRequest() {

1. This is not used everywhere where it can be: sometimes this helper method is used, sometimes requests are pushed directly onto the stack. We should always use this helper method.
2. With a tiny change, it can be used to set up any kind of request, not only GET requests.
3. s/setup/setUp/

Docs fixes, minor function name improvements and most of all: restoring of the original test logic/intent of RenderTest::testDrupalRenderBubbling() to RendererBubblingTest::testBubblingWithPrerender(). Now with many more comments. Should be much more understandable than what's in HEAD.

Raising to critical and adding the blocker tag, based on #2429257-30: Bubble cache contexts.

RendererPostRenderCacheTest review: finished, no more complaints. See below for an overview of changes.

1. Docs fixes, minor function name improvements, and some clarifications of the original test logic/intent.
2. Deleted common_test_post_render_cache(), we now have PostRenderCache::callback() for that.
3. Deleted common_test_post_render_cache_recursion(), we now have PostRenderCacheRecursion::callback() for that.
4. Moved RenderTest::testDrupalRenderRenderCachePlaceholder() (from the kernel test) to \Drupal\Tests\Core\Render\RendererPostRenderCacheTest::testPlaceholder() (unit test).
5. Moved RenderTest::testDrupalRenderChildElementRenderCachePlaceholder() (from the kernel test) to \Drupal\Tests\Core\Render\RendererPostRenderCacheTest::testChildElementPlaceholder() (unit test).
6. Deleted common_test_post_render_cache_placeholder(), we now have PostRenderCache::placeholder() for that.
7. Deleted core/modules/system/tests/modules/common_test/templates/common-test-render-element.html.twig — that is now mocked behavior.

i.e. moved the last two test cases from Kernel tests to Unit tests for which that conversion is possible.

RendererRecursionTest review: finished, no more complaints.

While cleaning up RendererRenderPlainTest, I noticed the name is not a perfect match. RendererRecursionTest is better.
Fixed docs.

While definitely not perfect unit test coverage, I feel like @dawehner's awesome work is in no way a regression, and only an improvement. The rerolls I've been posting since #25 are solely part of the review process:

1. the original RenderTest had grown organically over the years and was extremely unwieldy
2. hence dawehner made the best out of it while converting each test method, one by one
3. in doing so, some bits of the original test intent were lost — but only a few. I only know because I'm partially responsible for that organic growth. I've added docs and made minor changes to bring back the original intent where necessary.
4. finally, in making sure that not a single test was lost, the different method orders (which doesn't actually matter for the tests themselves of course) and introduction of new helper methods (yay!), and after having talked to dawehner and Fabianx, we decided to split it up into several unit tests, each covering a different aspect of the renderer. This makes it both easier to review for the scope of this issue, but also makes it easier to maintain the renderer and its test suite going forward, and last but not least: it should help to make it easier to understand the renderer!

Since I feel the reroll work was a necessary part of the review process — or at least necessary to speed it up — and since I'm one of the very few people who actually know the RenderTest tests, I still feel comfortable RTBC'ing this patch.

All of #35 is minor clean-up, no fundamental changes.

RTBC +1

Added beta evaluation.

Hm. I'm not really clear on why this must be done in order to address the parent issue, it seems like more of a clean-up (the issue summary doesn't help). It's a good clean-up to make nevertheless, and it's not changing the public API, and makes things less fragile, so is fine to do during beta.

Committed and pushed to 8.0.x. Thanks!