This is a follow-up task for #2756519: Convert assertIdentical() to assertSame() for some uses in kernel tests that already have assertSame()'s parameter order, since just replacing it didn't work for the whole testbase. The previous task is now only about kernel tests.

It's unclear if other core tests than kernel tests should also have their assertIdentical methods removed in favour of assertSame.

This should be simple with sed:
grep -rl assertIdentical /path/to/Drupal | xargs sed -i s@assertIdentical@assertSame@g

There are other similar functions, it's also unclear if these should be replaced as well.

CommentFileSizeAuthor
#44 2867959-44-91x.patch586.51 KBmondrake
#18 2867959-18.patch1.98 KBmondrake
#6 2867959-6.patch895.84 KBdanielnv18
#6 interdiff-2867959-3-6.txt803 bytesdanielnv18
#5 interdiff-2867959-3-5.txt803 bytesdanielnv18
#3 replace-assertIdentical-with-assertSame-2867959-3.patch896.36 KBdanielnv18

Issue fork drupal-2867959

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ZeiP created an issue. See original summary.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

danielnv18’s picture

danielnv18
Primary language Spanish
CreditAttribution: danielnv18 at ParallelDevs commented
Assigned: Unassigned » danielnv18
FileSize
replace-assertIdentical-with-assertSame-2867959-3.patch896.36 KB
danielnv18’s picture

danielnv18
Primary language Spanish
CreditAttribution: danielnv18 at ParallelDevs commented
Status: Active » Needs work

well, it didn't go as smooth as I thought. I will keep working on it

danielnv18’s picture

danielnv18
Primary language Spanish
CreditAttribution: danielnv18 at ParallelDevs commented
FileSize
interdiff-2867959-3-5.txt803 bytes

Let's try again

danielnv18’s picture

danielnv18
Primary language Spanish
CreditAttribution: danielnv18 at ParallelDevs commented
FileSize
interdiff-2867959-3-6.txt803 bytes
2867959-6.patch895.84 KB
danielnv18’s picture

danielnv18
Primary language Spanish
CreditAttribution: danielnv18 at ParallelDevs commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

IMHO we should do the same as in the kernel test issue aka. just convert the usages where the parameter order was already right.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kkalashnikov’s picture

kkalashnikov CreditAttribution: kkalashnikov at Material for Drupal India Association commented

This patch is failing for drupal 9.1. So moving the ticket to need work.

kkalashnikov’s picture

kkalashnikov CreditAttribution: kkalashnikov at Material for Drupal India Association commented
Status: Needs review » Needs work
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Postponed
Parent issue: #2756519: Convert assertIdentical() to assertSame() for some uses in kernel tests that already have assertSame()'s parameter order » #3027952: [Plan] Remove the usage of deprecated methods in tests
Related issues: +#2756519: Convert assertIdentical() to assertSame() for some uses in kernel tests that already have assertSame()'s parameter order

Reparenting, and postponing on #3114617: Properly deprecate methods in Drupal\FunctionalTests\AssertLegacyTrait and Drupal\KernelTests\AssertLegacyTrait, while still keeping deprecations silenced

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Postponed » Active

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

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Title: Remove assertIdentical methods in favour of assertSame » Remove assert(Not)Identical methods in favour of assert(Not)Same
Status: Active » Needs work
FileSize
2867959-18.patch1.98 KB

Restart fresh, by removing the deprecation silencer. Let's see the size of the prize.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Postponed

Suggest to wait for more issues from the meta #3128573: [meta] Replace assertions with more appropriate ones to be completed before tackling this one.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Issue tags: +Deprecated assertions

Adding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Filed #3142351: Replace usages of AssertLegacyTrait::assertIdentical() in traits and base classes to have a first reduction of scope here by removing occurences in traits and base classes. Even with that in there will be more than 1500 conversions to be done, so this will likely need further slicing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Assigned: danielnv18 » Unassigned
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: Remove assert(Not)Identical methods in favour of assert(Not)Same » [meta] Remove assert(Not)Identical methods in favour of assert(Not)Same
Category: Task » Plan

Yep, I would also definitely make assertIdentical() and assertNotIdentical() separate patches for example.

A very large patch is OK if it literally only changes exactly one thing. git diff --color-words is scannable, and if the reviewer tires too much from that, git diff --porecelain can be used with a script to verify the results. (For example I used git diff --porcelain to review a 12 GB patch to convert all array() in core to [].)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Note however that #23 only works if we do not do other cleanups like switching the order of arguments in the same patch. It needs to be only 1:1 replacements of the method name for best reviewability.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: [meta] Remove assert(Not)Identical methods in favour of assert(Not)Same » [meta] Remove assert(Not)Identical() methods in favor of assert(Not)Same()

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Title: [meta] Remove assert(Not)Identical() methods in favor of assert(Not)Same() » Replace usages of deprecated AssertLegacyTrait::assertIdentical
Component: other » phpunit
Status: Postponed » Active

Rescoping to remove assertIdentical since assertNotIdentical has been converted already.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Active » Postponed

Thinking again, let’s have an issue that swaps the arguments (where appropriate) first. Then this one will be a mere search and replace.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Filed #3192221: [backport] Swap assertIdentical arguments in preparation to replace with assertSame.

mondrake opened merge request !260

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Postponed » Needs review

Blocker was committed, this is the final leap.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

Testbot is not happy.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Needs review
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

All the code changes look good to me.
There is a deprecation message test added.
The suppression of the deprecation message has been removed.
All instances of assertIdentical() have been replaced or we would have had an error message.
For me it is RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a rebase (already).

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Just patch context reroll, due to 2 context lines being changed in #3193600: Convert assertEqual() calls involving NULL, TRUE and FALSE to more appropriate PHPUnit assertions

  • catch committed 6c64763 on 9.2.x 
    Issue #2867959 by mondrake, danielnv18, xjm, ZeiP, daffie: Replace...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Committed/pushed to 9.2.x, thanks!

Needs another rebase against 9.1.x

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Replace usages of deprecated AssertLegacyTrait::assertIdentical » [backport] Replace usages of deprecated AssertLegacyTrait::assertIdentical
Version: 9.2.x-dev » 9.1.x-dev
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Postponed

Needs #3192221: [backport] Swap assertIdentical arguments in preparation to replace with assertSame first.

mondrake closed merge request !260

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Postponed » Needs work

#3192221: [backport] Swap assertIdentical arguments in preparation to replace with assertSame is in.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2867959-44-91x.patch586.51 KB

Reroll for 9.1

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Applies cleanly to 9.1.x, no instances remain. The suppression is still in place as we should only change that in 9.2.x.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: [backport] Replace usages of deprecated AssertLegacyTrait::assertIdentical » Replace usages of deprecated AssertLegacyTrait::assertIdentical
Status: Reviewed & tested by the community » Fixed

Committed 3edfa21 and pushed to 9.1.x. Thanks!

  • catch committed 3dcbc67 on 9.1.x 
    Issue #2867959 by mondrake, danielnv18, xjm, ZeiP, daffie: Replace...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone credited karthikkumarbodu.

quietone credited legolasbo.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Related issues: +#2908842: Replace deprecated function calls in CKEditorLoadingTest

Closed #2908842: Replace deprecated function calls in CKEditorLoadingTest as duplicate.