Problem/Motivation
We're working towards replacing assertIdentical with assertSame. assertSame requires the arguments to be ($expected, $actual, ...), whereas assertIdentical requires ($actual, $expected,...).
Proposed resolution
In preparation for the final cleanup, change all assertIdentical to have ($expected, $actual, ...) like assertSame so that in a follow up we can just search/replace the method name.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 3192221-31.patch | 293.91 KB | mondrake |
Issue fork drupal-3192221
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:
- 3192221-swap-assertidentical-arguments
changes, plain diff MR !249
Comments
Comment #2
mondrakeComment #4
mondrakeComment #5
mondrakeComment #6
longwaveTechnically this means we are calling assertIdentical() incorrectly after this is committed but before the followup is.
I wonder if instead we should do the swap and rename in one go (this could be scripted), and perhaps break out the
assertIdentical(..., NULL)conversion toassertNull()and any other similar changes to their own issue first?Comment #7
mondrakeThanks for looking into this @longwave.
The situation is a bit grim here. In HEAD right now part of the assertIdentical calls DO abide to its signature, but a lot of them (most?) are actually already set to ($expected, $actual,...) as it should be for assertSame.
So, I used the script below to swap all instances, and later I only git added those that require swapping. Manually.
In other words - in this issue we are just swapping those cases where the swap is needed, NOT ALL the instances.
Comment #8
longwaveUrgh, I didn't realise that. In that case I guess it is best to do the swap first and then the rename is simple.
However do we still want to break out the ~35 changes to assertNull to a separate issue? That would be easier to get in and make the patch here more straightforward.
Comment #9
mondrakeOK so let's pull out the conversions to assertNull to a separate issue.
Comment #10
mondrakeOpened #3192553: Convert assertIdentical(NULL..) to assertNull(...)
Comment #11
mondrakeUnpostponing, related issue was committed. Rerolled.
Comment #12
daffie commentedI could only one thing wrong with the patch.
Comment #13
mondrakeComment #14
daffie commentedAll code changes look good to me.
We do this issue to make #2867959: Replace usages of deprecated AssertLegacyTrait::assertIdentical more easy.
For me it is RTBC.
Comment #15
catchNeeds a rebase.
Normally we'd not leave HEAD in an interim state, but given the wrong order is already prevalent across core, and the general momentum on phpunit clean-up is very good, I think it makes sense to do this in two reviewable chunks.
Comment #16
mondrakeRerolled.
Comment #17
mondrakeComment #18
daffie commented+1 for RTBC. In the patch chunks from the rebase are all the changes to
assertIdentical()what they should be.Comment #20
catchCommitted d722e9d and pushed to 9.2.x. Thanks!
Needs a rebase for 9.1.x
Comment #21
mondrakeLet's reroll #3192427: [backport] Replace usages of deprecated AssertLegacyTrait::assertNotEqual first, to keep the sequence with what went in 9.2.x
Comment #22
catchComment #23
mondrakeComment #24
mondrakeRerolled for 9.1.x
Comment #25
daffie commentedThe backport patch for 9.1 is for me RTBC.
Comment #26
catchNeeds a reroll already.
Comment #27
ayushmishra206 commentedWill be working on this.
Comment #28
ayushmishra206 commentedRerolled the patch for 9.1.x
Comment #31
mondrakeReroll of #24.
Comment #32
catchCommitted e9b83ef and pushed to 9.1.x. Thanks!