Refactor assertions that use return values in conditionals

There are a handful of other cases, can we expand scope to include all of these at once?

$ rg '\s*if.*>assert' core
core/modules/migrate_drupal/src/Tests/StubTestTrait.php
24:      if (!$this->assertIdentical(count($violations), 0, 'Stub is a valid entity')) {

core/modules/file/tests/src/Functional/DownloadTest.php
177:    if ($this->assertResponse(200) == 'pass') {

core/modules/image/tests/src/Functional/ImageStyleDeleteTest.php
48:    if ($this->assertNotNull($component = $view_display->getComponent('foo'))) {
54:    if ($this->assertNotNull($component = $form_display->getComponent('foo'))) {

core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
372:    if ($this->assertTrue(is_object($entity), 'Entity found')) {

core/tests/Drupal/KernelTests/Core/Entity/EntityCrudHookTest.php
71:      if ($this->assertTrue($position !== FALSE, $message)) {

core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
57:      if ($this->assertTrue(is_array($record), 'Record is an array.')) {
75:      if ($this->assertTrue($record instanceof FakeRecord, 'Record is an object of class FakeRecord.')) {
94:      if ($this->assertTrue($record instanceof FakeRecord, 'Record is an object of class FakeRecord.')) {
112:      if ($this->assertTrue(is_array($record), 'Record is an array.')) {
128:      if ($this->assertTrue(is_array($record), 'Record is an array.')) {

core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php
436:    if ($this->assertTrue(isset($dynamic_library['version']))) {
441:    if ($this->assertTrue(isset($dynamic_library['dependencies']))) {

+++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
@@ -54,9 +54,8 @@ public function testQueryFetchArray() {
+      $this->assertIdentical($record['name'], 'John', 'Record can be accessed associatively.');

please use assertSame($expected, $actual) instead. assertIdentical is deprecat-ish :)

#6 I think it's better proceed by 'type of messed up assertion' here, because otherwise the scope will become giant in a blink. See more issues at #3128573: [meta] Replace assertions with more appropriate ones. There's one specifically for #3, see #3128905: Replace assert* involving an instanceof operator with assertInstanceOf()/assertNotInstanceOf().

In line with #7 I think if we expand scope here we should just remove the conditionals and leave the actual assertions as they are - for other, similar issues to resolve next. That gives us a fairly tightly scoped issue that we can hopefully get committed quite quickly.

Agree with #8, that also means #5 is OOS here.

So if we do #2 in this one, it could be re-titled and re-scoped to "Remove assertions that are expecting a result to execute conditionally further assertions". Since in PHPUnit assertions do not return any value (differently from Simpletest), we may already being never executing some tests if we e.g. expect a TRUE from an assert*()...

Raising to major because of that.

EDIT - Xposted with #9.

Please review!

+++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
@@ -25,10 +25,10 @@ public function testQueryFetchDefault() {
+      $this->asserSame('25 year old is John.', $record->name, 'John');

$this->assertSame() typo

Thank you @Lal_ and @Suresh. In fact per #8 and #10 we should not change any assertion here, only remove them from the if () {} blocks, and inline further assertions.

Interdiff from #3.

+++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
@@ -369,11 +369,10 @@ protected function doTestTranslationDeletion() {
-      $this->assertEmpty($translations[$langcode], 'Translation successfully deleted.');
...
+    $this->assertArrayNotHasKey($langcode, $translations, 'Translation successfully deleted.');

Note that this change is required or the test fails with

1) Drupal\Tests\node\Functional\NodeTranslationUITest::testTranslationUI
Undefined index: fr

+++ b/core/modules/migrate_drupal/src/Tests/StubTestTrait.php
@@ -21,11 +21,7 @@ protected function performStubTest($entity_type_id) {
-      if (!$this->assertIdentical(count($violations), 0, 'Stub is a valid entity')) {
-        foreach ($violations as $violation) {
-          $this->fail((string) $violation->getMessage());
-        }
-      }
+      $this->assertIdentical(count($violations), 0, 'Stub is a valid entity');

While we said we shouldn't change any assertions, this is probably better off as $this->assertEmpty($violations) with no message, and then I think any violations would be automatically reported?

I think #18 makes sense. NW for that.

Note there will be other like-for-like cases when the return value of an assert* is assigned to a variable and then the variable is used in a conditional:

  $success = $this->assertXxx(...);
  if ($success) {

but it's tricky so it's probably better do it in a follow up.

On this.

Addressed #18.

WRT #19, I found the following:

./core/modules/editor/tests/src/Functional/EditorUploadImageScaleTest.php:    $same_width = $this->assertEqual($width, $expected['width'], 'Actual width of "' . $width . '" equals the expected width of "' . $expected['width'] . '"');

./core/modules/editor/tests/src/Functional/EditorUploadImageScaleTest.php:    $same_height = $this->assertEqual($height, $expected['height'], 'Actual height of "' . $height . '" equals the expected width of "' . $expected['height'] . '"');

./core/modules/filter/tests/src/Kernel/FilterKernelTest.php:    $success = $this->assertEqual($result, '<p>' . $source . "</p>\n", 'Line break filter can process very long strings.');

./core/modules/system/tests/src/Functional/Mail/HtmlToTextTest.php:    $pass = $this->assertEqual($result, $text, Html::escape($message));

./core/modules/system/tests/src/Functional/Mail/HtmlToTextTest.php:    $pass = $this->assertNotRegExp('/\][^\n]*\[/s', $output, 'Block-level HTML tags should force newlines');

./core/modules/system/tests/src/Functional/Mail/HtmlToTextTest.php:    $pass = $this->assertEqual(

./core/modules/system/tests/src/Functional/Session/SessionTest.php:    $pass = $this->assertText($user->getAccountName(), new FormattableMarkup('Found name: %name', ['%name' => $user->getAccountName()]), 'User login');

./core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php:    $success = $this->assertResponse(200);

./core/tests/Drupal/KernelTests/Core/Field/FieldItemTest.php:    $result = $this->assertEqual($entity->field_test_item->value, $base_field_expected_value);

./core/tests/Drupal/KernelTests/Core/Field/FieldItemTest.php:    $result = $result && $this->assertEqual($entity->{$this->fieldName}->value, $expected_value);

./core/tests/Drupal/KernelTests/Core/Field/FieldItemTest.php:    $result = $result && $this->assertEqual($entity->field_test_item->value, $base_field_expected_value);

./core/tests/Drupal/KernelTests/Core/Field/FieldItemTest.php:    $result = $result && $this->assertEqual($entity->{$this->fieldName}->value, $expected_value);

IMHO we should address them in this issue, but would like to get +1

@kapilkumar0324 please note that when a contributor self-assigns an issue to her/himself, it usually means that s/he is working on it and will post a patch soon (well, at least sooner or later) - it's a kind of etiquette to prevent other people duplicating her/his effort.

Removing 'Contrib database driver' tag as I cannot see the relation to this issue.

#24 is RTBC

-1 to dealing with the assignments here, some of them look tricky and/or like they might have further refactors involved, it will be easier to review if we do these separately.

#26 yes, I also doubleclicked on them and my hair went up, so better tackle them separately.

@mondrake, sorry I will remember next time. Thanks

+++ b/core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php
@@ -433,14 +433,12 @@ public function testDynamicLibrary() {
+    $this->assertTrue(isset($dynamic_library['version']));
...
+    $this->assertTrue(isset($dynamic_library['dependencies']));

BTW, just filed a new issue for this

+++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
@@ -369,11 +369,10 @@ protected function doTestTranslationDeletion() {
+    $this->assertTrue(is_object($entity), 'Entity found');

Filed #3131818: Replace assertions involving calls to is_object() with assertIsObject()/assertIsObject() for this. meanwhile inspired by this one, filed 5 more is_ish ones.

We aren't actually removing assertions here, if anything we are adding ones that were previously never executed!

Opened #3131900: Refactor assertions that assign return values to variables as a followup

Simpler title.

+++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
@@ -369,11 +369,10 @@ protected function doTestTranslationDeletion() {
+    $this->assertTrue(is_object($entity), 'Entity found');

+++ b/core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php
@@ -433,14 +433,12 @@ public function testDynamicLibrary() {
+    $this->assertTrue(isset($dynamic_library['version']));

+++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
@@ -142,10 +138,9 @@ public function testQueryFetchBoth() {
+      $this->assertTrue(is_array($record), 'Record is an array.');

This patch will break other ongoing patches probably. And assertion messages did not remove.

Just postponed #3131818: Replace assertions involving calls to is_object() with assertIsObject()/assertIsObject() to wait for this one landing first. meanwhile, #3131816: Replace assertions involving calls to is_array() with assertIsArray()/assertIsNotArray() was RTBC'ed. So probably, this one should be postponed or NW, to remove assertion messages and use more appropriate assertions.

Addressed #34 - tried to use the most appropriate assertion, added a couple where I think it makes sense, and removed all the messages.

1. +++ b/core/modules/migrate_drupal/src/Tests/StubTestTrait.php
   @@ -19,14 +19,8 @@
   -    if ($entity_id) {
   -      $violations = $this->validateStub($entity_type_id, $entity_id);
   -      if (!$this->assertIdentical(count($violations), 0, 'Stub is a valid entity')) {
   -        foreach ($violations as $violation) {
   -          $this->fail((string) $violation->getMessage());
   -        }
   -      }
   -    }
   +    // When validateStub fails, it will return an array with the violations.
   +    $this->assertEmpty($this->validateStub($entity_type_id, $entity_id));
   

   Checked the line before if ($entity_id) {, there is an assertion for $entity_id already. so the change is good.

       $this->assertNotEmpty($entity_id, 'Stub successfully created');
   

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
   @@ -142,10 +140,11 @@ public function testQueryFetchBoth() {
   -      if ($this->assertTrue(is_array($record), 'Record is an array.')) {
   -        $this->assertIdentical($record[0], 'John', 'Record can be accessed numerically.');
   -        $this->assertIdentical($record['name'], 'John', 'Record can be accessed associatively.');
   -      }
   +      $this->assertIsArray($record);
   +      $this->assertArrayHasKey(0, $record);
   +      $this->assertSame('John', $record[0]);
   +      $this->assertArrayHasKey('name', $record);
   +      $this->assertSame('John', $record['name']);
   

   +1 to this

LGTM. @longwave, thanks!

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!

the 8.9 commit needs reversal since the patch is using methods not present in PHPUnit 6, HEAD testing is broken

Reverted the 8.9.x commit, moving to postponed.

Postponed on #3126787: [D8 only] Add forwards-compatibility shim for assertInternalType() replacements in phpunit 6&7.

Bloacker is in.

#35 is now green on D8.9, too.

Reverted the revert so things should work now. 🤞 Thanks!