Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
As part of working on #2345833 I perused all of the d6 migration files and found that multiple styles are used for assertions. For example, sometimes the first argument is $actual and sometimes $expected. And testing for NULL is done is several ways:
$this->assertTrue(is_null($component));
$this->assertIdentical($field, NULL, 'No body field found');
$this->assertNull($component);Clearly, it isn't difficult to read the code but consistency does make it easier for everyone. Having a decision on the following points would be a step in that direction.
- Preferred assertion to use
- Preferred way to test Null/empty
- Preferred way to test not Null/empty
- Order of arguments
Proposed resolution
- Preferred assertion to use
- Preferred way to test Null/empty
- Preferred way to test not Null/empty
- Order of arguments
As discussed in #2345833 use assertIdentical. And I would add if assertEqual is used the reason needs to be documented.
??
??
($actual, $expected)
Document as required.
Remaining tasks
Decide if a style convention is wanted or not.
User interface changes
None
API changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 2403743-15.patch | 144.87 KB | rpayanm |
| #5 | 2403743-5.patch | 144.9 KB | quietone |
| #3 | 2403743-3.patch | 145.15 KB | quietone |
Comments
Comment #1
benjy CreditAttribution: benjy commentedNot sure if this is migrate specific, feels almost like some kind of doc issue, something that should be core wide. Below are my personal preferences.
Comment #2
jhodgdonWe had an issue a while back about making some of the methods more consistent with others... but nothing came of it.
Anyway, I was asked to comment here by benjy...
My thoughts:
I don't think changing $expected and $actual order around in assertEqual or assertIdentical etc. would have any impact at all on Core, so I agree that should be done to make them all consistent, and I agree phpUnit would be a good standard to follow to model our classes on.
Regarding which assertion to use where... I think assertNull and assertNotNull make sense if you really want to test that something is really a NULL value. But a lot of the time, you just need to verify it evaluates to TRUE/FALSE. It just depends on where the value is used and what it's used for, that determines how to test it. Same with assertEqual vs. assertIdentical, it's like whether you would use == or === in an if() statement, depends on what you really need to test. So I doubt we need to or even could make a good rule for this.
That's my 2 cents...
Comment #3
quietone CreditAttribution: quietone commented@jhodgdon, thanks for your comments. Both you and benjy agree that arguments should be ($expected, $actual) - Same as PHPUnit. So, I've implemented only that in the attached patch.
BTW, there are currently only two instances of NULL being used in an assertIdentical:
Comment #5
quietone CreditAttribution: quietone commentedAnd again
Comment #6
quietone CreditAttribution: quietone commentedRight, I used sed (always nice to refresh regex) for over 95% of the changes. But still had to do some my hand. And after a while, it got a bit tedious, so there may be cases where the arguments are the wrong way around. Please keep that in mind while reviewing.
Comment #7
benjy CreditAttribution: benjy commentedThis is a really nice clean up, personally I think the params should be renamed in assertEqual and assertIdentical to indicate that it should be $expected/$actual.
We'd probably have to seek approval to make such a change.
Comment #8
jhodgdonActually, changing the name of a parameter can be done as a "documentation" change, since this type of change does not have an impact on functionality (it is just the name of a local variable in a method).
There is actually already an (old) issue that was aimed at doing that type of cleanup on WebTestBase, along with some other things like making the assert messages more flexible. It's probably still open, and should be in the simpletest module component if you want to look for it.
Comment #9
benjy CreditAttribution: benjy commentedClosest I could find was this #1945040: Rename assertIdentical() to assertSame() for better compatibility with PHPUnit, nothing specifically for switching the params.
Comment #10
jhodgdonWell, I can't find it either.
Comment #11
benjy CreditAttribution: benjy commentedI think this patch does what it set out todo, can we simply create a follow-up for the doc changes unless we happen to find the existing issue.
RTBC, not sure if the bot still likes it because the patch is a few weeks old.
Comment #13
benjy CreditAttribution: benjy commentedI feared as much :)
Comment #14
jhodgdonI found the other issue.
Comment #15
rpayanmComment #16
benjy CreditAttribution: benjy commentedGreat thanks.
Comment #21
vlad.n CreditAttribution: vlad.n commentedPatch "2403743-15.patch" applies cleanly.
Comment #22
alexpottMigrate and test code is not frozen. Committed c01a680 and pushed to 8.0.x. Thanks!