Replace theme() with drupal_render() in simpletest module

WDG (Ukraine,Kharkov) want to implement this on CodeSprintUA.

Updated assertThemeOutput function in /core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php.

+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -319,7 +319,11 @@ function _simpletest_batch_operation($test_list_init, $test_id, &$context) {
+   );   ¶
+  $context['message'] .= drupal_render($items_list);
...
+++ b/core/modules/simpletest/simpletest.theme.incundefined
@@ -27,13 +27,30 @@ function theme_simpletest_test_table($variables) {
+   );  ¶
+ ¶
   $js = array(
...
+++ b/core/modules/simpletest/simpletest.theme.incundefined
@@ -27,13 +27,30 @@ function theme_simpletest_test_table($variables) {
+ ¶
   // Cycle through each test group and create a row.
...
+++ b/core/modules/simpletest/simpletest.theme.incundefined
@@ -120,7 +137,13 @@ function theme_simpletest_test_table($variables) {
+      '#rows' => $rows,
+      '#attributes' => array('id' => 'simpletest-form-table'),     ¶
...
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -47,31 +47,44 @@ public static function create(ContainerInterface $container) {
     // Initialize image mapping property.
+    ¶
+    $image_pass = array(
...
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -47,31 +47,44 @@ public static function create(ContainerInterface $container) {
+    );
+    ¶
+    $image_fail = array(
...
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -47,31 +47,44 @@ public static function create(ContainerInterface $container) {
+    );
+    ¶
+    $image_exception = array(
...
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -47,31 +47,44 @@ public static function create(ContainerInterface $container) {
+    );
+    ¶
+    $image_debug = array(
...
+    );
+    ¶
     $this->statusImageMap = array(

https://drupal.org/coding-standards#indenting
whitespace errors

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -2577,7 +2577,12 @@ protected function assertNoTitle($title, $message = '', $group = 'Other') {
+    $theme = array('#theme'=>$callback);

needs whitespaces around of
=>

Thanks, whitespace errors were solved.

#6 not solved
try http://dgo.to/dreditor and look at Your patch
whitespace indenting not tabs
whitespace endings
still needs work

Solved whitespace issues in patch #6.

Solved whitespace issues in patch #6.

Made changes as suggested by thedavidmeister.

I also changed the alt and title because i think this is wrong.

+  $image_collapsed = array(
+    '#theme' => 'image',
+    '#uri' => 'core/misc/menu-collapsed.png',
+    '#width' => '7',
+    '#height' => '7',
+    '#alt' => t('Expand'),
+    '#title' => t('Expand'),
+  );
+  $image_extended = array(
+    '#theme' => 'image',
+    '#uri' => 'core/misc/menu-expanded.png',
+    '#width' => '7',
+    '#height' => '7',
+    '#alt' => t('Collapse'),
+    '#title' => t('Collapse'),
+   );

Marking as needs review.

accidentally deleted tag.

Removing changes to 'alt' and 'title' and fixed minor whitespace issues. Also note that there is a possible error with the class name for the link in suffix:

+  $image_collapsed = array(
+    '#theme' => 'image',
...
+    '#suffix' => '<a href="#" class="simpletest-collapse">(' . t('Expand') . ')</a>',
+  );
+  $image_extended = array(
+    '#theme' => 'image',
...
+    '#suffix' => ' <a href="#" class="simpletest-collapse">(' . t('Collapse') . ')</a>',
+   );

I have left it like this for now.

#16: simpletest_replace_theme_with_drupal_render-2009670-16.patch queued for re-testing.

#16: simpletest_replace_theme_with_drupal_render-2009670-16.patch queued for re-testing.

Looks like all the tests have passed on the patch in #16.

#16 looks good to go. #2021551: Possibly confusing class name in simpletest.pages.inc is opened to fix the CSS class name.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Committed 6038891 and pushed to 8.x. Thanks!

diff --git a/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
index 8c46872..6ba54b4 100644
--- a/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -2639,7 +2639,11 @@ protected function assertNoTitle($title, $message = '', $group = 'Other') {
    *   TRUE on pass, FALSE on fail.
    */
   protected function assertThemeOutput($callback, array $variables = array(), $expected, $message = '', $group = 'Other') {
-    $output = theme($callback, $variables);
+    $build = array('#theme' => $callback);
+    foreach($variables as $key => $variable) {
+      $build["#$key"] = $variable;
+    }
+    $output = drupal_render($build);
     $this->verbose('Variables:' . '<pre>' .  check_plain(var_export($variables, TRUE)) . '</pre>'
       . '<hr />' . 'Result:' . '<pre>' .  check_plain(var_export($output, TRUE)) . '</pre>'
       . '<hr />' . 'Expected:' . '<pre>' .  check_plain(var_export($expected, TRUE)) . '</pre>'

assertThemeOutput() was designed for modules to prevent regressions in their theme hook implementations. It takes just a theme callback en variables. This hunk should be reverted.

The breaking of test coverage cannot be a normal task...

@Eric_A The change above should not break tests. Are you sure it does?

Calling drupal_render() on a render array with "#theme' => 'callback' should produce exactly the same output as calling theme('callback'). Since this test only needs the $output from that call, there should be no regression here.

Additionally, since there will be no calls to theme() anywhere else in core, this test *should* test the output in the same way it will be generated, which is via drupal_render().

Changing issue status back.

If there is actually a problem with this test, can you create a follow-up issue that references this one?

Calling drupal_render() on a render array with "#theme' => 'callback' should produce exactly the same output as calling theme('callback').

Agreed that it *should* produce the exact same output, but this is not always the case (See #2041283: theme_status_report() is severely broken). Anyway, this is a low level method that was introduced to test theme function output directly, not the drupal_render() route. See #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output. If we had an assertThemeOutput() for theme_status_report() it would have failed with the patch that was committed here. So yes, the drupal_render() implementation is useful, it may make loads of sense to create a method for testing the output returned from the drupal_render() route, but that one should get a name different from assertThemeOutput() and we should not just throw away the low level testing in a completely different issue.

Fixing this regression here as a quick follow-up does look like the right thing to do.

Hm, if drupal_render() != theme() that is a bug in itself. So I don't see the need for it that core calls a soon-to-be-deprecated function.

Hm, if drupal_render() != theme() that is a bug in itself. So I don't see the need for it that core calls a soon-to-be-deprecated function.

I don't think the theme() system is soon-to-be-deprecated at all. Rather, soon-to-be-considered-internal.
Yes, the idea is to use the higher level render API instead of calling theme() directly. But the theme() system will still be around, theme hook implementations will still need to be provided by modules, and there should still be a way to *test* these components on their own, by directly calling these functions. So yes, I'm saying drupal_render() != theme() and I don't consider that a bug at all.

Well, from the function definition of drupal_render => drupal_render with appropriate input is always equal to theme and anything else is a bug.

And if we deprecate theme() as public function, which we do, then Drupal internally would call a deprecated public function, which feels strange, but should be okay.

But I don't have strong feelings about this here and I am fine with either way and let the core committers to decide.

If we had an assertThemeOutput() for theme_status_report() it would have failed with the patch that was committed here.

Wouldn't that be a good thing?

It is true that a drupal_render() version of this test helper can uncover bugs in theme hook implementations that take a render element. But theoretically it could go another way. A bug in drupal_render() could make a test pass or fail when it shouldn't. We're adding more and more unit testing. Surely we can't do this at the same time? At least not hidden in this issue.

Expanding the original test helper by adding a #theme property when the hook needs a render element gets us the same coverage, but without the dependency on drupal_render(). I'd be happy to do that.

When you look at the two tests that currently call assertThemeOutput() there's more evidence that we've broken the contract:

\Drupal\system\Tests\Theme\FunctionsTest::testItemList() - "Tests theme_item_list()."
\Drupal\system\Tests\Theme\FunctionsTest::testLinks() - "Tests theme_links()."

I just learned that we have \Drupal\system\Tests\Common\RenderTest which extends \Drupal\simpletest\WebTestBase. We could override WebTestBase::assertThemeOutput() in RenderTest (or whichever test class makes the most sense). So we'd be restoring the original contracts and have a drupal_render() version for tests that want to "Assert basic render element output" rather than "Assert themed output".

The patch was installed and the testing module ran swimmingly. Everything is looking dandy. GO TEAM DRUPAL!!!!

Still needs review.

Still needs review.

Could this perhaps be assigned to @tim.plunkett or @effulgentsia? @tim.plunkett did #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output and @effulgentsia maintains both Theme system and Render system.

We will be deprecating theme() as it is - that has been decided now on twig call. The only way to get output from theme() will be drupal_render() going forward (from D9 then). This solves a lot of major tasks for the theme system cleanup and drillability and how to make #type work.

There will be a theme_info() call and theme() will just be a wrapper around it probably that calls the #render_function gotten from theme_info() with the #render_variables. (Issue link is coming as soon as I have written this up).

I am fine to change this back and forth, but in the end this will be changed anyway.

Unit Testing is great - yes. The question is: Where is the "border" of calling internal APIs?

Maybe another option is to change the function name of themeTest to renderTest to make clear it is testing drupal_render() and not theme() - soon to be deprecated?

Thanks!

( Removed tags on purpose as they don't apply to the current patch and this is not Novice. )

Why would deprecation of theme() in itself mean that we no longer care about (unit) testing module provided parts in a low-level output generating subsystem? If we cannot use theme() anymore, then we'll use what comes closest. Which can never be any higher level Render API. If we want to remove this level of testing, that's a different issue. (It's not a conversion issue, that's for sure.)
When testing the output of a template and its (pre)processors we really don't care much what clients will be using all of this. The notion of Drupal render elements and #type properties are irrelevant here.

Right now assertThemeOutput() has become incomprehensible for anyone who knows what it was about, reads the current doxygen and then takes a look at the current implementation.

 /**
* Asserts themed output.
* 
* @param $callback
*   The name of the theme function to invoke; e.g. 'links' for theme_links().
* @param $variables
*   An array of variables to pass to the theme function.
* @param $expected
*   The expected themed output string. 

Hm. I don't fully grok what the problem is here, and it seems like there is disagreement within the Twig team about whether or not it actually is a problem.

If something needs to get rolled back, let's get agreement on this and an updated issue summary that explains what's going on to core committers.

Changing status back to CNR

actual summary

doing my homework.

Okie dokie, I just re-read everything and updated the issue summary. And here's what I think we should do:

1) Use the patch in #27 since that fixes broken tests, which we introduced in the initial commit.

2) We should add a test that will fail appropriately without the patch in #27, as per #34.

3) We will be deprecating theme() - or making it internal - or whatever - later on. Let's postpone the rest of the debate (including calling only drupal_render() in tests, renaming themeTest to renderTest, etc) until then, and at least get things back in a working order right now.

Another regression: assertThemeOutput() no longer works when passing a renderable array as the only argument. The current implementation just expects that it is passed an associative array of variables, which I believe is an unneeded assumption. The assertThemeOutput() method "Asserts themed output", so a full functional mapping to theme() is what really helps client code.

1) Use the patch in #27 since that fixes broken tests, which we introduced in the initial commit.

2) We should add a test that will fail appropriately without the patch in #27, as per #34.

3) We will be deprecating theme() - or making it internal - or whatever - later on. Let's postpone the rest of the debate (including calling only drupal_render() in tests, renaming themeTest to renderTest, etc) until then, and at least get things back in a working order right now.

@jenlampton, are you working on this? Otherwise, I might give it a shot.

I went looking for existing assertions involving renderable array theme hooks and found a few in \Drupal\system\Tests\System\ThemeTest. I converted all assertions to assertThemeOutput() and expect the ones involving renderable array theme hooks to fail without the fix from #27.

So the testbot revealed yet another issue: theme() and the fixed version of assertThemeOutput() return the same value if the passed $callback is not implemented. (Currently this is the boolean FALSE.) The current version of assertThemeOutput() returns what drupal_render() returns. (Currently the empty string.)

So the testbot revealed yet another issue: theme() and the fixed version of assertThemeOutput() return the same value if the passed $callback is not implemented. (Currently this is the boolean FALSE.) The current version of assertThemeOutput() returns what drupal_render() returns. (Currently the empty string.)

Yes, I was working on these tests. That's why I assigned it to myself ;)

I decided to use assertIdentical() instead of assertThemeOutput() to confirm that the output from theme() and drupl_render() are identical. Unfortunately, the output was identical. Even without this patch.

Since your tests catch the bug you were referring to (and mine don't) I'll leave this to you.

update

WebTestBase::assertThemeOutput() currently broken for theme hooks that work with a render element instead of variables.

Added the most pressing part of the current state to the issue summary.

Note that WebTestBase::assertThemeOutput() must work for both theme hook types, but is currently broken for theme hooks that work with a render element instead of variables. The patch in #47 proves and fixes this.

Test looks fine. Let's go back to how it was for now and revisit later.

Committed cc1f3fd and pushed to 8.x. Thanks!