Replace assertEqual() with assertEquals()

@jungle - for removal of usages of legacy assertions, I would suggest to use #3027952: [Plan] Remove the usage of deprecated methods in tests as a parent.

IMHO #3114617: Properly deprecate methods in Drupal\FunctionalTests\AssertLegacyTrait and Drupal\KernelTests\AssertLegacyTrait, while still keeping deprecations silenced shoul be done before we tackle any of these conversions. Also, this one specifically will be *massively huge*, so maybe this can be tackled later on after we convert as many as possible to more specific assertions.

#3114617: Properly deprecate methods in Drupal\FunctionalTests\AssertLegacyTrait and Drupal\KernelTests\AssertLegacyTrait, while still keeping deprecations silenced is in.

I've closed #2912056: Remove deprecated method assertEqual from core as a duplicate of this one.

#8 yeah, right.

Hi @jungle
As the issue in #8 has been closed, I am submitting a fix for this one. Moving this to Needs Review.

There was a small issue in the above patch. Re uploading

Please review the following patch.
Thanks

Fixes the failed test cases!

Fixes the failed test cases in patch #22!

@rahulrasgon.
I have review it, the patch looks good to me, Thanks for the lengthy patch.
t() will be removed here for assertEqual : https://www.drupal.org/project/drupal/issues/3153468

This patch needs automating and scheduling. It's going to cause a lot of patches to need rerolling.

Can I suggest that we first work on a bash script or something similar to create the bulk of the patch.

This is a monster patch - and one that I woud suggest to manage carefully, too. In changing from Simpletest's assertEqual($actual, $expected, ...) to PHPUnit's assertEquals($expected, $actual, ...) we have a swap in the expected and actual argument.

This is right now a problem in HEAD, where we have cases of mistmatching arguments. I agree this is only as important as you get a failure of the test, BUT when you get one PHPUnit will report what is expected and the actual, and if that's swapped then developers get itchy scratch syndromes.

So if we just convert massively we will lose the opportunity of a more thorough review of what is expected and what is actual in the comparison. Probably for good. Also, IMHO when we are comparing scalars we should be using the more strict assertSame instead.

I would suggest pausing here a moment and see how the conversion of assertIdentical goes, which faces similiar issues in converting to assertSame, but on a smaller size, and has already some proposals on how to split the conversions. See #2867959: Replace usages of deprecated AssertLegacyTrait::assertIdentical.

Needs some prep issues to be landed first, similarly to what's been done for assertIdentical.

Needs a deprecation test for assertEqual()

@mondrake: Do you only want to do the change from assertEqual() to assertEquals() or can we change assertEqual(TRUE, $something) to assertTrue($something)?

As far as I can tell the methods assertEqual() and assertEquals() have no return value. Only when you search the code for "= $this->assertEqual" and/or "&& $this->assertEqual" you will get code that thinks that there is a return value. Is there an issue for that?

Do we can/want to change assertions like $this->assertEquals([], $something) and $this->assertEquals('', $something) to some other assertion(s)?

All the changes from assertEqual() to assertEquals() look good.

Thanks @daffie

Do you only want to do the change from assertEqual() to assertEquals() or can we change assertEqual(TRUE, $something) to assertTrue($something)?

I suggest to do only the method renaming here, it's a 1.7 MB patch alone. We can do the other optimizations in follow-ups like we did earlier in #3192553: Convert assertIdentical(NULL..) to assertNull(...), #3193600: Convert assertEqual() calls involving NULL, TRUE and FALSE to more appropriate PHPUnit assertions and others ... we missed some then because they were in multi-line code snippets and only with #3193955: Swap assertEqual arguments in preparation to replace with assertEquals that was scripted we cleaned that up. A follow-up with the missing ones will be a few KB patch.

As far as I can tell the methods assertEqual() and assertEquals() have no return value. Only when you search the code for "= $this->assertEqual" and/or "&& $this->assertEqual" you will get code that thinks that there is a return value. Is there an issue for that?

#3131900: Refactor assertions that assign return values to variables

Do we can/want to change assertions like $this->assertEquals([], $something) and $this->assertEquals('', $something) to some other assertion(s)?

Same as above, followup to replace with assertSame when type matters and assertEmpty otherwise

All instances of assertEqual() have been changed to assertEquals().
A deprecation message test has been added.
The supression of the deprecation message has been removed from core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php.
All code changes look good to me.
For me it is RTBC.

Edit: @mondrake thank you for your reply!

We should schedule this one for the 9.2.0 beta (which starts i May 17, 2021) since it's touching hundreds of files at once and is scriptable.

Unscriptable/smaller phpunit changes I think we should still try to commit outside of betas.

edit: also nearly forgot, @xjm suggested in slack that while we should schedule it for the beta, we should still backport to 9.1.x during the beta too - since it'll make backports easier - and this makes sense to me too.

Time to get this one home.

@mondrake your changes to date are OK by me, so I think it is fine for you to RTBC after reviewing my merge and final fixes here.

Only 4 minor outlining issues. After that it is RTBC for me.

The MR is fine. It is a bug in the gitlab UI. All thread can be marked as solved.
The method is deprecated and testing therefore is added.
All usage of the method has been replaced.
For me it is RTBC.

Really weird that only who opened the MR can mark comment threads as resolved. IMHO it should be MR creators, the initiator of the comment thread, maintainers. Dunno if it's an integration point or just how GitLab works.

@longwave I reviewed commit https://git.drupalcode.org/project/drupal/-/merge_requests/273/diffs?com..., all good to me, just a comment in the MR.

Is there a policy on rebasing MRs and adding more changes? - me personally I prefer to rebase, push, then add changes and push again, so to get a clean MR version.

Committed and pushed 3f75f72de2 to 9.3.x and 6c199eccdb to 9.2.x. Thanks!

We should backport this to 9.1.x but without the skip deprecation removal.

Created new MR for the backport.

Assigned: Unassigned » Spokje

Created new MR for the backport.

longwave opened merge request !692
Issue #3131281: Replace assertEqual() with assertEquals()

PHP 7.3 & MySQL 5.7 Queued

Add test / retest

Well...never mind

Sorry @Spokje I should have assigned this first :(

I'll work on it.

RTBC; as far as I can see, this is only removing lines with assertEqual(...) and adding lines with assertEquals(...).

Committed 97b61da and pushed to 9.1.x. Thanks!

Can someone open a quick follow to replace assertEqual( is some comments...

• field_test.module
• \Drupal\Tests\aggregator\Kernel\Migrate\d7\MigrateAggregatorItemTest::testAggregatorItem
• \Drupal\plugin_test\Plugin\MockBlockManager::createContextDefinition

could do with an update.

The follow-up issue #3215143: Replace replace assertEqual() in some comments

Closed #3177480: Replace deprecated assertEqual( with assertEquals( in Kernel Unit Test for module views as a duplicate.