Refactor assertions that assign return values to variables

Reviewed patch and it looks good for me!

Fun issue. I think we need to be careful of BC. I think if a method return FALSE when it passes then we have something that doesn't make sense and can be fixed. But if it returns TRUE for a pass then we need to maintain that because of BC implications for contrib and custom code.

1. +++ b/core/modules/editor/tests/src/Functional/EditorUploadImageScaleTest.php
   @@ -224,9 +222,8 @@ protected function assertSavedMaxDimensions($width, $height) {
   -    return $same_width && $same_height;
   

   So this is always going to be returning FALSE (ie. NULL && NULL) so the return value makes no sense so I think changing this without a CR is okay.

2. +++ b/core/modules/system/tests/src/Functional/Cache/AssertPageCacheContextsAndTagsTrait.php
   @@ -147,7 +144,6 @@ protected function assertCacheContexts(array $expected_contexts, $message = NULL
        $this->assertIdentical($actual_contexts, $expected_contexts, $message);
   -    return $actual_contexts === $expected_contexts;
   

   Yeah we'll never hit this code if we fail but this is a sort of API change for contrib. Imo we should change this to return TRUE so it's explicit that is always a pass at this point and then deprecate the return value - its possible contrib/custom has tests that depend on it. We need a CR for this.

3. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
   @@ -123,21 +120,13 @@ protected function assertToolbarCacheContexts(array $cache_contexts, $message =
   -    return $return;
   

   This will always return TRUE so again I think we need a CR and to return TRUE. The other issue here is now the $message is completely ignored and useless.

4. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php
   @@ -169,10 +166,10 @@ public function assertPagePath($path) {
   -    return $success && $this->assertResponse(200);
   

   This will always return FALSE afaics so removing this is fine.

5. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldItemTest.php
   @@ -85,20 +85,16 @@ public function testSaveWorkflow() {
   -    return $result;
   

   This will always return FALSE too so removing is fine.

The only way I can think of for deprecating the return value is to deprecate the entire methods and write new ones that return void. TBH it feels a bit overkill, though. Maybe just a CR can do; 'tests are not API', and these are not even in base classes. If some projects were extending these classes back when they were still in Simpletest, they would have been forced to adjust to PHPUnit already anyway.

1. +++ b/core/modules/editor/tests/src/Functional/EditorUploadImageScaleTest.php
   @@ -215,8 +215,6 @@ protected function uploadImage($uri) {
   -   *
   -   * @return bool
       */
      protected function assertSavedMaxDimensions($width, $height) {
        $image_upload_settings = Editor::load('basic_html')->getImageUploadSettings();
   @@ -224,9 +222,8 @@ protected function assertSavedMaxDimensions($width, $height) {
   
   @@ -224,9 +222,8 @@ protected function assertSavedMaxDimensions($width, $height) {
          'width' => $image_upload_settings['max_dimensions']['width'],
          'height' => $image_upload_settings['max_dimensions']['height'],
        ];
   -    $same_width = $this->assertEqual($width, $expected['width'], 'Actual width of "' . $width . '" equals the expected width of "' . $expected['width'] . '"');
   -    $same_height = $this->assertEqual($height, $expected['height'], 'Actual height of "' . $height . '" equals the expected width of "' . $expected['height'] . '"');
   -    return $same_width && $same_height;
   +    $this->assertEquals($expected['width'], $width);
   +    $this->assertEquals($expected['height'], $height);
      }
   

   Don't we need to return TRUE for the same reasons, here? I read the original intention of this assertion to have TRUE returned when dimensions are as expected, so that further tests can continue. Here if one assertion fails OK, all stops, but if they still pass and downstream an extended class is expecting true to continue testing, this will not happen. But.. see my header comment.

2. +++ b/core/modules/system/tests/src/Functional/Mail/HtmlToTextTest.php
   @@ -242,25 +234,11 @@ public function testDrupalHtmlToTextBlockTagToNewline() {
        $output_upper = mb_strtoupper($output);
        $upper_input = mb_strtoupper($input);
   ...
   +    $this->assertEquals($output_upper, $upper_output, 'Tag recognition should be case-insensitive');
   

   I think here we can inline all the logic in the assert w/o assigning to variables first.

3. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php
   @@ -157,9 +157,6 @@ public function testPagePaths() {
   -   *
   -   * @return bool
   -   *   Assertion result.
       */
      public function assertPagePath($path) {
        $view = Views::getView('test_page_display_path');
   @@ -169,10 +166,10 @@ public function assertPagePath($path) {
   
   @@ -169,10 +166,10 @@ public function assertPagePath($path) {
        $this->container->get('router.builder')->rebuild();
        // Check if we successfully changed the path.
        $this->drupalGet($path);
   -    $success = $this->assertResponse(200);
   +    $this->assertResponse(200);
        // Check if we don't get any error on the view edit page.
        $this->drupalGet('admin/structure/views/view/test_page_display_path');
   -    return $success && $this->assertResponse(200);
   +    $this->assertResponse(200);
      }
   

   same as point 1

4. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldItemTest.php
   @@ -85,20 +85,16 @@ public function testSaveWorkflow() {
   -   *
   -   * @return bool
   -   *   TRUE if the item value matches expectations, FALSE otherwise.
       */
      protected function assertSavedFieldItemValue(EntityTest $entity, $expected_value) {
        $entity->setNewRevision(TRUE);
        $entity->save();
        $base_field_expected_value = str_replace($this->fieldName, 'field_test_item', $expected_value);
   -    $result = $this->assertEqual($entity->field_test_item->value, $base_field_expected_value);
   -    $result = $result && $this->assertEqual($entity->{$this->fieldName}->value, $expected_value);
   +    $this->assertEqual($entity->field_test_item->value, $base_field_expected_value);
   +    $this->assertEqual($entity->{$this->fieldName}->value, $expected_value);
        $entity = $this->reloadEntity($entity);
   -    $result = $result && $this->assertEqual($entity->field_test_item->value, $base_field_expected_value);
   -    $result = $result && $this->assertEqual($entity->{$this->fieldName}->value, $expected_value);
   -    return $result;
   +    $this->assertEqual($entity->field_test_item->value, $base_field_expected_value);
   +    $this->assertEqual($entity->{$this->fieldName}->value, $expected_value);
   

   same as point 1

NW (IMO, just for point 2)

on this

Addressed #6.

Reverted this from #5 as IMHO it's out of scope

+++ b/core/modules/system/tests/src/Functional/Cache/AssertPageCacheContextsAndTagsTrait.php
@@ -129,8 +129,8 @@ protected function assertCacheTags(array $expected_tags, $include_default_tags =
    * @param bool $include_default_contexts
    *   (optional) Whether the default contexts should automatically be included.
    *
-   * @return bool
-   *   TRUE if the assertion succeeded, FALSE otherwise.
+   * @return true
+   *   Always returns TRUE.
    */
   protected function assertCacheContexts(array $expected_contexts, $message = NULL, $include_default_contexts = TRUE) {
     if ($include_default_contexts) {
@@ -147,7 +147,8 @@ protected function assertCacheContexts(array $expected_contexts, $message = NULL

@@ -147,7 +147,8 @@ protected function assertCacheContexts(array $expected_contexts, $message = NULL
     sort($expected_contexts);
     sort($actual_contexts);
     $this->assertIdentical($actual_contexts, $expected_contexts, $message);
-    return $actual_contexts === $expected_contexts;
+
+    return TRUE;
   }

+++ b/core/modules/editor/tests/src/Functional/EditorUploadImageScaleTest.php
@@ -216,7 +216,10 @@ protected function uploadImage($uri) {
+   * @todo make this assert method return void in D10.

+++ b/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php
@@ -158,8 +158,10 @@ public function testPagePaths() {
+   * @todo make this assert method return void in D10.

+++ b/core/tests/Drupal/KernelTests/Core/Field/FieldItemTest.php
@@ -86,19 +86,22 @@ public function testSaveWorkflow() {
+   * @todo make this assert method return void in D10.

"A @todo without a followup is just wishful thinking." -- @catch

Is there a reason not to refactor the callers here to account for no return value? We're already changing (edit: or already have changed) the return values, and we don't provide BC for individual tests.

Reading previous comments, I guess I am contradicting #4, in a rare case of release manager being more permissive about BC rather than less. But these methods aren't test API; they're internal helpers for individual tests. Edit: It'd be different if they were on traits or base classes.

I mean, there are a couple cases where we deliberately extend non-abstract test classes in order to run the parent's test methods on all the child classes. So it's not unheard of. But individual tests being not subject to any BC is maybe the only part of our internal API policy we haven't had to walk back our "no BC guarantee" for during the D8 lifecycle.

Anywho, there's #2727011: [policy, no patch] Private vs protected, and the role of inheritance for a policy discussion about private. (Technically, core policy dating to 2012 or so is that we never use private, although that's been enforced inconsistently.)

Removed todos, I will file a separate issue to make all custom assert*ions to be typehinted to void, to have a single issue where to do that in a consistent manner. It's not a follow up really, rather an issue of its own. That would have to be D9 (or D10?) only though, since void return typehint cannot be used in PHP 7.0 that D8 still supports.

Also swapped arguments in a assertEquals to ensure $expected comes on the left hand side, and converted calls of (now formally) deprecated Simpletest assertions in favor of WebAssert ones.

Filed #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods

The patch needs a reroll, because #3139218: Replace usages of AssertLegacyTrait::assertResponse(), which is deprecated landed.

on this

Rerolled. Only DisplayPageWebTest was impacted by the change from assertResponse() to assertSession()->statusCodeEquals().

So in #3082859: The AssertMailTrait should cast the data type to boolean in assertMailPattern, a contributor is relying on the return value of AssertMailTrait to be true. In that case it's a trait, and actual public API, so it makes sense for us to provide BC there. I'm still not sure that means we should here, as these are not public API but #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods is on an interesting track there. I don't know though that it's helping callers that are doing $this->assertTrue($this->assertCustomThing($foo, $bar));

Given the contrib example from #3082859: The AssertMailTrait should cast the data type to boolean in assertMailPattern and the fact that that trait is not included here, I think the scope is not complete. Callers can do other things with the return value than assign it to variables, like conditional logic or even assertTrue()ing the conditional logic. So we should look at

If the full scope of "Refactor tests that rely on return values from assertions" is too unwieldy, we can make this part of a meta also, but we'd need the parent.

So NW to either file a meta or expand the scope here. Thanks!

IMHO this is good sense cleanup. I'd defer #27 and #28 together to #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods.

+++ b/core/modules/system/tests/src/Functional/Cache/AssertPageCacheContextsAndTagsTrait.php
@@ -142,7 +142,7 @@ protected function assertCacheContexts(array $expected_contexts, $message = NULL
     $this->assertSame($expected_contexts, $actual_contexts, $message ?? '');
-    return $actual_contexts === $expected_contexts;
+    return TRUE;
   }

Should we have a follow-up to change this to null as well?

#32 it will be done in #3138087: openstory 8.x-2.23

OK I didn't realise that issue was trying to deal with traits too. Left a comment there, but seems OK to go ahead with this.

Committed bcfecbb and pushed to 9.3.x. Thanks!

This is tagged for a followup, #23, to address this problem in a wider scope. After some searching I rediscovered #3043939: [Meta] Remove usages of void return, which is a suitable follow up. I have updated the IS over there. I am removing the tag.