Replace assert*() involving equality comparison operators with assert(Not)(Equals|Same)

Re-rolled patch. Please review!

Rerolled patch to 9.1.x as #11 failed to apply.

Removed "Needs reroll" tag

Thanks all for working on this.

There are 52 matches matching the regex >assert.*\(.*( == | === | != | !== ) , but none of them meets the scope there. RTBC +1.

Leave NR expecting another review or a joint RTBC.

@mondrake Here is the re-rolled patch for 9.1.x could you please review.
There us no setUp function in AssertMailTrait

Thanks
Pavnish

@mondrake Fixed #18 test failed issue here could you please review.

Thanks
Pavnish

Made some changes in the patch.

Rerolled patch #26, please review.

Patches from #28 and #26 failed to apply.

The attached patch is rerolled from #25.

$ egrep -Ril ">assert.*\(.*( == | === | != | !== )" core/ | wc -l
106
$ git apply 3132919-29.patch
$ egrep -Ril ">assert.*\(.*( == | === | != | !== )" core/ | wc -l
49

Fixing Php-lint error in #29

@ridhimaabrol. When applying the patch I saw some Hunk Failed messages

patching file core/lib/Drupal/Core/Test/AssertMailTrait.php
patching file core/modules/aggregator/tests/src/Functional/UpdateFeedItemTest.php
Hunk #1 FAILED at 67.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/aggregator/tests/src/Functional/UpdateFeedItemTest.php.rej
patching file core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
Hunk #1 succeeded at 306 (offset -3 lines).
patching file core/modules/block/tests/src/Functional/BlockRenderOrderTest.php
patching file core/modules/block/tests/src/Kernel/BlockViewBuilderTest.php
patching file core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php
Hunk #1 succeeded at 381 (offset -6 lines).
patching file core/modules/comment/tests/src/Functional/CommentActionsTest.php
patching file core/modules/comment/tests/src/Functional/CommentInterfaceTest.php
Hunk #1 FAILED at 101.
Hunk #2 succeeded at 113 (offset -1 lines).
1 out of 2 hunks FAILED -- saving rejects to file core/modules/comment/tests/src/Functional/CommentInterfaceTest.php.rej
patching file core/modules/config/tests/src/Functional/ConfigOtherModuleTest.php
patching file core/modules/editor/tests/src/Functional/EditorAdminTest.php
patching file core/modules/field/tests/src/Kernel/BulkDeleteTest.php
patching file core/modules/field/tests/src/Kernel/FieldCrudTest.php
patching file core/modules/field_ui/tests/src/Functional/ManageFieldsFunctionalTest.php
patching file core/modules/file/tests/src/Functional/FileFieldValidateTest.php
patching file core/modules/filter/tests/src/Functional/FilterAdminTest.php
patching file core/modules/forum/tests/src/Functional/ForumTest.php
Hunk #1 succeeded at 467 (offset 1 line).
Hunk #2 succeeded at 596 (offset 2 lines).
Hunk #3 succeeded at 678 (offset 2 lines).
patching file core/modules/language/tests/src/Functional/LanguageLocaleListTest.php
patching file core/modules/language/tests/src/Functional/LanguageNegotiationContentEntityTest.php
patching file core/modules/language/tests/src/Functional/LanguageUILanguageNegotiationTest.php
patching file core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
Hunk #1 succeeded at 850 (offset -33 lines).
patching file core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php
Hunk #1 FAILED at 105.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php.rej
patching file core/modules/locale/tests/src/Functional/LocaleJavascriptTranslationTest.php
patching file core/modules/locale/tests/src/Functional/LocaleTranslationUiTest.php
patching file core/modules/locale/tests/src/Kernel/LocaleStringTest.php
patching file core/modules/node/tests/src/Functional/NodeFieldMultilingualTest.php
patching file core/modules/node/tests/src/Functional/NodeRevisionsAllTest.php
patching file core/modules/node/tests/src/Functional/NodeRevisionsTest.php
Hunk #2 FAILED at 197.

Please let me know if this needs to be fixed

@samiullah Please update your branch to the latest head. There are no such issues on the latest head.

The patch on comment #32 can be applied in 9.1.x-dev. Not moving to RTBC yet. Might need more detailed review.

Rerolled patch from #32 due to a recent change in core/modules/editor/tests/src/Functional/EditorAdminTest.php (commit a44a9b1cd044d56e7b0c3a7412b38c9292fc83f2).

I reviewed the patch with git diff --color-words and I think it is looking close. However, should we also be removing useless assertion messages here? There are quite a few we could get rid of.

+++ b/core/tests/Drupal/KernelTests/Core/Database/UpdateLobTest.php
@@ -49,6 +49,9 @@ public function testUpdateMultipleBlob() {
     $r = $this->connection->query('SELECT * FROM {test_two_blobs} WHERE [id] = :id', [':id' => $id])->fetchAssoc();
     $this->assertTrue($r['blob1'] === 'and so' && $r['blob2'] === 'is this', 'Can update multiple blobs per row.');
+    $r = $this->connection->query('SELECT * FROM {test_two_blobs} WHERE id = :id', [':id' => $id])->fetchAssoc();
+    $this->assertSame('and so', $r['blob1']);
+    $this->assertSame('is this', $r['blob2'], 'Can update multiple blobs per row.');

The old assertions need removing here.

I also grepped for remaining assertions. There are a lot of false positives but a few that I think we should look at here:

core/modules/search/tests/src/Functional/SearchAdvancedSearchFormTest.php:        $this->assertFalse(isset($elements[0]) && $elements[0]->getValue() == $value, "Field $key is not set to $value");

This should be split into two assertions.

core/modules/book/tests/src/Functional/BookTestTrait.php:    $this->assertNotNull(($node === FALSE ? NULL : $node), 'Book node found in database.');
core/modules/book/tests/src/Functional/Views/BookRelationshipTest.php:    $this->assertNotNull(($node === FALSE ? NULL : $node), 'Book node found in database.');
core/modules/book/tests/src/Functional/BookBreadcrumbTest.php:    $this->assertNotNull(($node === FALSE ? NULL : $node), 'Book node found in database.');

The ternary operator in each of these cases is a bit odd. Probably better to check $node is an object?

core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php:    $this->assertTrue(($expected < 0 && $result < 0) || ($expected > 0 && $result > 0) || ($expected === 0 && $result === 0), 'Numbers are either both 
negative, both positive or both zero.');

Unsure if this is out of scope, but is this sort of thing better written out like the below? Not sure.

if ($expected < 0) {
  $this->assertLessThan(0, $result);
}
elseif ($expected > 0) {
  $this->assertGreaterThan(0, $result);
}
else {
  $this->assertSame(0, $result);
}

Looking pretty good, just a couple of comments:

1. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -900,7 +900,7 @@ public function testExtraFields() {
        $extra_field = strpos($content_fields_category->getText(), 'Extra label');
   -    $this->assertTrue($extra_field !== FALSE);
   +    $this->assertNotFalse($extra_field);
   

   Should this be assertStringContainsString()?

2. +++ b/core/tests/Drupal/Tests/Core/Config/ReadOnlyStorageTest.php
   @@ -104,7 +104,7 @@ public function testWriteOperations($method, $arguments, $fixture) {
        // Assert that the memory storage has not been altered.
   -    $this->assertTrue($backup == $this->memory);
   +    $this->assertEquals($backup, $this->memory);
   

   Should this be tightened to assertSame()?

Addressed comment #50, please review.

Alright, I was wrong about #50.2, sorry!

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -898,9 +898,7 @@ public function testExtraFields() {
     $page = $this->getSession()->getPage();
...
+    $assert_session->elementTextContains('xpath', '//details/summary[contains(text(),"Content fields")]/parent::details', 'Extra label');

$page is unused now.

Ah, OK. That looks like it could be $this instead but that's enough scope creep already.

All review points addressed - RTBC if bot agrees.

Rerolled, stay RTBC

Committed cf64091 and pushed to 9.2.x. Thanks!