Convert assertIdentical() to assertSame() for some uses in kernel tests that already have assertSame()'s parameter order

Patch Applied.

Thanks @Anul, and thanks @timmillwood for cleaning this up.

The automated testing infrastructure tells us whether the patch applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

In the case of this issue, I think this is not a good scope for removing these deprecated methods. If we do one patch per kernel test class that has the deprecated methods, that will be like 305 patches, which is probably about 300 more than we need to implement the change:

[mandelbrot:drupal | Wed 03:20:58] $ grep -lr "assertIdentical" * | grep "Kernel" | wc -l
     305

See https://www.drupal.org/core/scope#incomplete for more background on why we avoid committing impartial scopes like this. In the case of this issue, we should just make the change in an issue that is dedicated to removing the deprecated method everywhere. That issue probably exists and this issue could be marked as a duplicate of it. (If the issue does not exist, it probably should be created as a plan to convert tests.) However, I'm marking the issue "Needs work" for now so both @timmillwood and @Anul get the chance to see this feedback.

Thanks for your work on this!

Thank You @xjm . I understood everything and will take care of it in future.
So shall this issue be moved to closed(duplicate) state?

I'll work on a patch for this today.

I made a patch to make the replace. The command mostly worked, but some unrelated calls and function definitions were replaced as well; those have been excluded from the patch.

The patch is quite huge and as it touches quite many test files, it probably ages poorly, so a speedy review would probably be necessary. I ran the test suite on my local environment after the changes and any problems seem unrelated to the patch; I'm hoping the testbot will confirm that.

Right, that was quick... New version of the patch that hopefully applies to the current tip of 8.3.x.

The patch wasn't sent to the testbot, trying again, this time with the correct comment number.

Hmm, lots of "test runner returned a non-zero error code (255)". Lets give a shot again and try.

Testbot, do your magic.

Attached is a new patch with some changes to apply to the most recent version of the branch. I tried running the tests locally and they seem to work, so let's see if it works now.

+1 to convert just that single function but in one go. This makes it much easier to review.

Again an updated patch. Based on the error messages on my local testing environment it seems that updating traits might be the problem, so I excluded those from the patch. Additionally I updated four new files that have been added calls to assertIdentical() and fixed one simple typo in one testing-related file.

Here's to hoping this one works...

Changing only KernelTests now.

Right, so that one at least works. Could someone a bit more familiar with the testing system give some insight on why some of the tests fail with assertSame()?

At least the KernelTests part would be ok to review and commit, should we do that now to make sure at least a part of the change goes through without too many more iterations with the patch? :)

Attached is a new patch against 8.4.x-dev, still only touching KernelTests.

The patch works for KernelTests, so I'm changing this task to concern only the kerneltests and creating a follow-up task #2867959: Replace usages of deprecated AssertLegacyTrait::assertIdentical to deal with the rest of core tests.

Good call on splitting the issue. The patch very long and tedious to manually review as it is.

I ran the following to get a rough count of assertIdentical throughout core:

grep -R -o assertIdentical core/ | wc -l

2531

I applied the patch to 8.4.x curl https://www.drupal.org/files/issues/2756519-26-replace_assertidentical_with_assertsame.patch | git apply

and then ran grep -R -o assertIdentical core/ | wc -l

1980 (includes non kernel tests)

I took a look at the number of files there and did find kernel tests listed such as

core/modules/syslog/tests/src/Kernel/Migrate/d6/MigrateSyslogConfigsTest.php: $this->assertIdentical('128', $config->get('facility'));

I attached the list here after pruning non kernel tests from the list, which takes it down to 1373 assertIdenticals in kernel tests (and traits used exclusively in kernel tests).

Also some questions, should assertIdentical in classes extending the deprecated Drupal\simpletest\KernelTestBase be changed here as well? GenericCacheBackendUnitTestBase contains references to assertIdentical

Mh, there is actually one major difference between assertSame and assertIdentical, their arguments are swapped:

  protected function assertIdentical($actual, $expected, $message = '') {

    public static function assertSame($expected, $actual, $message = '')

I kinda think that changing this might be a good idea.

@dawehner: In the trait the choice has been made to call the assertIdentical parameters that way but to me it looks like it's maybe not well established? The SimpleTest version just calls them $first and $second. I don't see the docs giving any more guidance on what is what either. From a quick grep it looks like in practice you find both choices in core tests too although the order of actual, expected was more common in the small sample I looked at and certainly in KernelTests most tests did follow that.

So I'd say this is what's needed to get this done:
1. Add the remaining kernel tests we're missing
2. Change the argument order for all calls with a regex search and replace to get most of them right
3. Check all the calls and change the order back manually for any that actually have it the other way around

The idea was to only make the change for tests inside core/tests/Drupal/KernelTests for now, clarified this in the title. Attached is a new patch containing a new run of the same routine with arguments switched, where appropriate.

I suggest going with this scope for this issue in order to get the patch in, unless there's more things to fix in these files – making a new patch for the full scope tends to be quite laborious, so getting a bulk of the changes in gets us quite a bit closer towards having the issue completely solved.

That is, if there weren't a simple syntax mistake from the last few manual changes. Fixed those.

A few tests outside KernelTests had slipped in with my git add -p, and they failed; attached is a patch containing strictly only tests inside core/tests/Drupal/KernelTests.

Also a few assertSameObject()s fixed.

@Zeip, probably best to keep this to assertIdentical( only, and not expand the scope for assertIdenticalObject. Just from experience that should go in it's own issue.

Also, some of them you swapped the arguments around, and some them you left, was this on purpose?

@joelpittet, yep, the patches in comments 33 through 37 got a few assertIdenticalObject() calls changed by mistake, but those have been reverted in the latest patch.

I tried to be smart about where to switch the arguments: ie. it seems that the arguments were quite randomly used either way, and I made the change so that all the changed calls would use the new order assertSame($expected, $actual[...]. That's why not all of the calls had their arguments switched; those are the ones that I thought used an incorrect order originally.

Thanks @ZeIP, I must have missed that posting. I checked that indeed there are no more assertIdentical inside of core/tests/Drupal/KernelTests and the patch only acts within that folder. And double checked the smart switcharoo of the arguments so that expected are first in all the cases by scanning git diff --color-words -- core/tests/Drupal/KernelTests

Thanks for your help with this patch @ZeIP

Thanks @ZeiP for taking the time to reroll this patch in response to the feedback. It'll be great to convert these so that we can deprecate and eventually remove the BC layer for the SimpleTest method.

I do agree with earlier feedback that we should convert all of the kernel tests in a single patch despite its size. Note that these sorts of patches don't only have to be reviewed manually; you can use:

• git diff --color-words and scan visually
• git diff --porcelain with a script

Discussed this with @larowlan and he also recommended actually converting all the kernel tests even though it is a very large patch. That way, we can also properly deprecate the previous method (see https://www.drupal.org/core/deprecation) and ensure that no new usages are introduced in core.

A point that is actually more of a concern for me is that switching the order of the arguments is actually making the patch a lot more difficult to review with a word diff like those above. I see @dawehner's comment above on this point, but let's take care of this in a followup issue postponed on this one. So I agree with @joelpittet's original feedback here on that as well. I think @Antti J. Salminen's recommendation for the order of steps in #32 is great -- but let's just make each of those steps a separate issue.

You can read more about best practices for issue scope (related to both the above points) on https://www.drupal.org/core/scope

Given the scope of the patch that will be required, I think we probably actually should schedule this change for during the beta or release candidate phase of 8.4.x development, because it will cause many existing patches to need reroll (as we've seen by all the rerolls needed here!). That way we also don't need to have a different patch generated for the backport (and because this will also make it difficult to cherry-pick future patches.)

So, I'm postponing this issue and marking it as an rc target. We can reroll it around Aug. 15 when 8.4.x has entered beta, removing the reordering of the parameters, and making it change only assertIdentical() to assertSame() in all kernel tests.

I've also updated the issue credit for this issue and I am removing the novice tag because it's actually no small feat to roll such a large patch, even programmatically. Thanks everyone for your diligence on this issue and patience with competing recommendations.

(Editing my comment to try to make it shorter...)

@xjm, was trying to write a follow up but realized that assertSame and assertIdentical have different parameter order. It seems like a wasted effort to "fix" assertIdentical(), just to swap it back later? Or if we swap the arguments to assertSame there are dozens of patches that may not follow that and make a bit of a mess of coordination. Maybe the correct order of things should be that we remove the argument swaps in this patch and commit this patch with the swaps in another, or is that what you were asking for in #43?

assertSame($expected, $actual
assertIdentical($actual, $expected

I am personally not convinced of making the swap later, how would you be able to do this easily? We have all the already valid orders of assertSame in tests, which means we need to look at every single instance. For now we know that assertIdentical is known to be the wrong way round, so swapping now is actually easier than swapping later.

Here's the follow-up as I see it from my thoughts in #44 #2887161: Ensure argument order is correct for assertSame() assertions

And if I understand correctly, we are going to expand this to bleed outside core/tests/Drupal/KernelTests?

Here are the two opportunities:

1. Change from assertIdentical to assertSame but keep the order or parameters
Just replacing the method would be totally scriptable. The next step would be to swap the parameters:

A point that is actually more of a concern for me is that switching the order of the arguments is actually making the patch a lot more difficult to review with a word diff like those above. I see @dawehner's comment above on this point, but let's take care of this in a followup issue postponed on this one

. The more I thought about it, the less I think this would be a good idea. Let's assume the current assertSame and assertIdentical calls have the right order, converting assertIdentical to assertSame would introduce a HUGE number of wrong usages, while loosing the context which calls are right and which are wrong. Just swapping would not be possible now, as we would basically make all the current valid calls invalid. Given that we would need to go through every single instance.

2. Change from assertIdentical to assertSame but swap the order of parameters
This change is scriptable entirely, given we assume that most of the assertIdenticals are correct

Given that I think 2) is the by far better solution as going through every single assertSame instance would be a HUGE time investment. Would you accept a "prove" the patch is correct by writing a script which "tests" the patch?

I'm +1 to #47.2 but would like to get @xjm and ZeiP's thoughts before proceeding.

If there wouldn't be the problem that there are any calls to assertSame out there already, I would be totally in favour of just replacing the function call.

My gut feeling would say that the assumption that most assertIdenticals are in correct order isn't a very valid one; there were a lot of instances where I didn't change the order because it was originally incorrect. IMO making the change selectively based on the patch I provided will certainly give the best result.

As for scoping the issue outside the directory I assigned, there were some instances of assertIdentical() calls that didn't work as expected outside core/tests/Drupal/KernelTests. The scope of all KernelTests does sound more logical *if* there's someone available to figure out the tests that can't be simply replaced.

Making the change without swapping the parameters doesn't sound good to me: As pointed out in #47.1 this will cause us to lose *any* idea on what calls should be wrong (obviously, current assertSame() calls might be incorrect, as well as current assertIdentical() calls might use the assertSame() parameter order, but I suppose at least half of each uses the parameters correctly according to the used function's signature.)

So basically I'd amend #47.2 by either trying to re-roll my patch, use it as a reference when ”cherry-picking” the changes or use logic to determine which parameters to switch. It's a bit more work, but not that much (once we can actually agree on when to make this change and get the patch approved & in before it gets sour. Without that it's just frustrating.)

My previous recommendation still stands. From a scan of the patch, it looks like nearly half the method calls have the wrong parameter order, so we do not lose that much by briefly having the "other" wrong order, and we gain a lot in terms of reviewability. It should not be too difficult to take the current patch and split out the changes that only change parameter order into a separate postponed issue, which is a patch that could easily be rebased on top of the scope I recommend when the time comes.

Regardless, though, this remains an RC target. It's too disruptive and divergent to the queue to change during the development phase. This kind of change is exactly what the RC phase is for.

@xjm
As far as I understand your recommendation is NOT to flip around every call to assertSame later. Given that the only thing IMHO we can do in a practical manner is to apply the patch without changing parameters now, and then copy the existing patch with changing the order directly as step 2, which is weird, we simply touch twice the same code.

Maybe a better plan would be to split up the patch like this:

* Just convert the calls which are already in the "right" order, so assertIdentical($expected, $actual)
* Then in another patch convert the others, so assertIdentical($actual, $expected)

I guess this would make review much easier, given there is one scope?

Hmmm, I don't think #53 will really work as that patch would be very complicated to generate. If the changes in the current patch are correct, the order of the parameters is not even consistent within a single test class in some cases.

Re: #52, I'm not really concerned about touching the same code twice; that's totally fine during RC if one patch is easy/scripted and the second requires manual review. I was trying to think of an easy way to not lose the manual work that's already gone into this patch of checking some of core's calls to the method to validate whether their existing parameter order is correct.

I was thinking it'd be totally easy to rebase the reordered methods in this patch on top of that one to correct the parameter order within the scope that this already touches and that would not add too much work. It's something like:

1. Apply the current patch without --index or unstage it.
2. Use https://stackoverflow.com/questions/10873882/how-to-use-color-words-with... to remove the hunks that have the "correct" parameter order already (wrong for HEAD, but correct for assertSame()). Attach that patch to a postponed followup.
3. Simple scripted patch here to change assertIdentical() to assertSame() only. Review/commit that (easy).
4. Rebase the followup on top of that scripted patch to correct the parameter order for those method calls. Review/commit that (requires checking each call manually to verify @ZeiP's fixes and therefore using brains on those calls only).
5. Bulk change the method call order in other files not yet included in this patch. Review/commit that.
6. (Optional) Validate the parameter order is correct in other files.

I've realized it adds some tedium to expand the scope of this to all kernel tests, then fix only some, then bulk change others. So I guess we are stuck with the current scope of affected tests if we don't want to add a bunch of extra work. We could bulk change the method calls in this patch, commit the reorder followup, and then use core's kernel test discovery to identify remaining calls that are assertIdentical() and bulk change the method and argument order of those in a single patch for my step 5 and @dawehner has suggested. I'll think a little more about it. Edit: Actually, my steps 5 and 6 maybe should wait until after the web tests are all converted, because that's what truly allows us to deprecate the BC layer methods and at that point we don't need to get fancy with test discovery; it's just phpstorm and grep to verify it.

At least in the future though we should always do manual and automatic work in separate patches, rather than adding manual work on top of automatic patches. Touching the same line twice in separate patches is always okay.

@ZeiP, can you add your most current script or sed to the summary? Edit: Is it #29?

Okay I've thought about this and I have an idea to optimize it for the least work; I'll show rather than tell to save everyone reading since that also takes up our brains' time just like manual review does. :) I need to sleep right now though. We can unassign from me if I don't do this by July 5.

Sure. It was almost the one suggested in some earlier post, but I think I added the parentheses.

Actually. I think I misunderstood #53 and my "new" idea is just @dawehner's. lol. Sorry for the noise; updating the scope. I need to remember that @dawehner is usually right.

So basically we split this, but keep the changes that do not change the parameter order, rather than those that do. Then those method calls we never have to think about again. And then once all the web tests are converted, only then do we do a massive change of all the method calls and all parameter orders across core.

So use git -c interactive.diffFilter="git diff --color-words" add -p and stage only hunks like this:

Then attach that patch to the issue; maybe that only is even small enough to do now. Exclude all the ones that currently change parameter order and let them be caught by a later conversion (that does actually switch parameter order across the board).

I can make #58 if we like, but if someone else wants to make it and a different person wants to peer-review it then I can save my participation for the committer review. :)

I'll take a stab at it, let's see if I understood.

At least in the future though we should always do manual and automatic work in separate patches, rather than adding manual work on top of automatic patches. Touching the same line twice in separate patches is always okay.

That is a really wise statement. It helps a LOT to do reviewable bits!

) I need to sleep right now though.

I feel you, you skipped sleep to think about order of parameters instead.

Actually. I think I misunderstood #53 and my "new" idea is just @dawehner's. lol. Sorry for the noise; updating the scope. I need to remember that @dawehner is usually right.

So what I thought when I read your post: Wow, someone is actually able to articulate what I tried to express. I hope you got sleep now :)

TL;DR: Nice constructive discussion.

@xjm, the command looks ok but it seems to work very randomly for me – it fails to select the parts I added and selects some parts I didn't. I wonder if I have a too outdated Git version – did it work correctly for you & on what version?

@xjm, the command looks ok but it seems to work very randomly for me – it fails to select the parts I added and selects some parts I didn't. I wonder if I have a too outdated Git version – did it work correctly for you & on what version?

I'm not sure exactly how your problem looks like, but I was always able to split ("s") up the hunk more with or at least edit ("e") it manually.

It looks like it's working correctly, splitting and accepting works fine, but for example the first file has some chunks I can't accept and it keeps repeating those in front of all other files. And additionally it seems to add chunks I didn't accept to index.

Unfortunately I couldn't get the word coloring to properly work, but I think I managed without it. Attached are two new patches: One simple, ie. only containing those changes that didn't require argument order switch, and one that contains the ones that required an argument change.

Here are the patch lengths, along with the previous patch I used as a base:

zeip@jameson ~/vcs/drupal ±8.4.x⚡ » wc -l 2756519-39-replace_assertidentical_with_assertsame.patch                                                                                                                                          
3683 2756519-39-replace_assertidentical_with_assertsame.patch
zeip@jameson ~/vcs/drupal ±8.4.x⚡ » wc -l 2756519-65-replace_assertidentical_with_assertsame_argumentschanged.patch 2756519-65-replace_assertidentical_with_assertsame_simple.patch                                                         
  2841 2756519-65-replace_assertidentical_with_assertsame_argumentschanged.patch
   996 2756519-65-replace_assertidentical_with_assertsame_simple.patch
  3837 total

So it seems it's all there :) The simple patch is quite a bit smaller than the other one, is it small enough to put through straight away?

Right. The patch with arguments changed probably doesn't apply, because it would require the changes in the other patch first. I think that's probably ok for now.

Yep, the patch we want is the first one. It looks like about 30% of the method calls in HEAD use the wrong order, and half of all classes have some, wow. I think it's probably small enough to do now and okay to cherry-pick, so the main disruption will be to patches that affect these 34 tests, but once rerolled the patches would be okay for their backports.

The second patch we won't need as such -- just the sed from the summary that generates the first part of it as a starting point for a new issue, that (IMO) we postpone until web tests are done and we can just get rid of assertIdentical() everywhere and change parameter order on top of that.

Scanning the patch, it looks like the correct subset of changes from the original patch. So a reviewer needs to review two things:

1. That the patch only changes assertSame() to assertIdentical() (this is easy to see).
2. That in each test, these particular method calls already have the order $expected, $actual and so are wrong in HEAD but correct when converted to assertSame(). This requires reading each test a bit and thinking about it.

Unsure why the files are all in reverse order.

Could someone review this? The patch still seems to apply to 8.4.x HEAD, I added a new test in #65 just to be sure.

Nope, it didn't. Reverting to Needs work.

Attached is a reroll against current 8.4.x HEAD.

I think personally this change should be against 8.4.x only. @ZeiP do you disagree?

I think @dawehner meant 8.5.x only since an 8.4.x-only patch would be an interesting change. :) However, RC is actually a good time to make big noisy cleanups.

#74 isn't the right patch though; we want one like the "simple" one from #65 to start. I gave tips for reviewers in #68.

Oups, sorry! I thought I took the correct patch to reapply but apparently didn't. Attached is the correct patch reapplied against 8.4.x HEAD.

I agree with @xjm, IMO it would make sense to make the change for 8.4.x as well as obviously 8.5.x.

Haha, yeah I think we should do 8.0.x only changes, just because we can :)

I went through the patch and found just a couple of instances, on which I believe the order should be actually changed.

1. +++ b/core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php
   @@ -205,13 +205,13 @@ public function testScriptLoad() {
        // Ensure the test config has been replaced.
        $config = unserialize(db_query("SELECT data FROM {config} WHERE name = 'test_config'")->fetchField());
   -    $this->assertIdentical($config, $this->data, 'Script has properly restored the config table data.');
   +    $this->assertSame($config, $this->data, 'Script has properly restored the config table data.');
   

   Note: This one needs a swap of the arguments

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
   @@ -103,7 +103,7 @@ public function testConnectionOptions() {
        $connectionOptions = $db->getConnectionOptions();
   -    $this->assertIdentical($connectionOptions, $connectionOptions2, 'The default and replica connection options are identical.');
   +    $this->assertSame($connectionOptions, $connectionOptions2, 'The default and replica connection options are identical.');
   

   Given that you test getConnectionOptions here, I think the other way round is what it expected here ...

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityQueryTest.php
   @@ -403,7 +403,7 @@ public function testCount() {
   -    $this->assertIdentical($count, count($this->entities));
   +    $this->assertSame($count, count($this->entities));
   

   $count is the number which returned by the config entity query, so it should be the other way round here

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php
   @@ -66,8 +66,8 @@ public function testEntityReferenceAutocompletion() {
        $data = $this->getAutocompleteResult($input);
   -    $this->assertIdentical($data[0]['label'], Html::escape($entity_1->name->value), 'Autocomplete returned the first matching entity');
   -    $this->assertIdentical($data[1]['label'], Html::escape($entity_2->name->value), 'Autocomplete returned the second matching entity');
   +    $this->assertSame($data[0]['label'], Html::escape($entity_1->name->value), 'Autocomplete returned the first matching entity');
   +    $this->assertSame($data[1]['label'], Html::escape($entity_2->name->value), 'Autocomplete returned the second matching entity');
    
   
   @@ -77,13 +77,13 @@ public function testEntityReferenceAutocompletion() {
   -    $this->assertIdentical($data[0]['label'], Html::escape($entity_2->name->value), 'Autocomplete returned the second matching entity');
   +    $this->assertSame($data[0]['label'], Html::escape($entity_2->name->value), 'Autocomplete returned the second matching entity');
   
   @@ -95,7 +95,7 @@ public function testEntityReferenceAutocompletion() {
   -    $this->assertIdentical(reset($data), $target, 'Autocomplete returns an entity label containing a comma and a slash.');
   

   $data is what is fetched via the API, so it should be on the right side

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php
   @@ -77,13 +77,13 @@ public function testEntityReferenceAutocompletion() {
   -    $this->assertIdentical(reset($data), $target, 'Autocomplete returns only the expected matching entity.');
   +    $this->assertSame(reset($data), $target, 'Autocomplete returns only the expected matching entity.');
   

   $target should be on the left side here

@dawehner, thanks! You're quite right. core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php had a few other replacements too that were incorrect. Attached is a new patch fixing these.

Thank you @ZeiP!

Great, thanks @dawehner and @ZeiP. Committed and pushed to 8.5.x, and backported to 8.4.x as a test cleanup during RC.

And now we need a followup for the patch that changes the argument order, as well as a postpone followup to check browser tests the same way we checked kernel tests here. I think we might actually want to postpone both followup on all the web tests being converted.

We have the two follow ups now, afaik.