Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
$this->assertSame(FALSE, ...
and $this->assertSame(TRUE, ...
is no longer necessary and can be replace with assertTrue/False()
.
Also $this->assertSame(..., FALSE/TRUE)
.
Proposed resolution
Replace assertSame(TRUE/FALSE,...
with assertTrue/False()
.
Comment | File | Size | Author |
---|---|---|---|
#16 | 3082415-16.patch | 110.02 KB | mondrake |
#16 | interdiff_12-16.txt | 67.64 KB | mondrake |
#12 | interdiff-10-12.txt | 730 bytes | Krzysztof Domański |
#12 | 3082415-12.patch | 45.33 KB | Krzysztof Domański |
Comments
Comment #2
Krzysztof Domański1. #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests should be fixed first.
2. See also #3082287: \Drupal\user\Plugin\views\access\Role::access() does not conform to the base class documentation.
Comment #3
Krzysztof DomańskiComment #4
Krzysztof DomańskiComment #5
mondrakeComment #7
mondrakeParent committed, unpostponing.
Comment #8
mondrakeThere are also assertEquals(true|false... and assertEqual(true|false... calls that probably need love, not sure if here or in a separate issue.
Comment #9
mondrakeComment #10
Krzysztof Domański1. Re-rolled and fixed additional changes added in the meantime (see #2).
2. I agree that we can also change
assertEqual(s)
(see #8) but we need a separate issue.Replacing
assertEqual
withassertFalse/True
is not the same as replacingassertSame
withassertFalse/True
.assertEqual
->$value == FALSE
assertSame
->$value === FALSE
assertFalse
->$value === FALSE
Comment #11
mondrakeCouple nits:
1.
The comment here should be changed according to the change in the assert.
2. There are a couple of hacky use of assertSame remaining, in rest/test/src/Functional/ResourceTestBase.php and same in jsonapi/test/src/Functional/ResourceTestBase.php:
this could be changed to
assertNotFalse
Comment #12
Krzysztof Domański11.1. Deleted redundant comment.
11.2. The
$expected_cache_tag
is a function parameter.I think we should not change anything here.
The code above is a shorter form of this:
Comment #13
mondrakeThanks for #12.2 - I'd rather go for your longer form then because I feel it's not good to combine conditional logic with assertions, but that's definitely not for here. RTBC for me.
Comment #14
alexpottI think we should only do this on Drupal 9 once we've removed the overrides. This is because if the test is marked @legacy then the deprecations won't matter and we might not be testing what we think we're testing.
Let's make this a Drupal 9 issue that removes the deprecations and overridden methods and then we're good.
Comment #15
mondrakeIt's not just
assertSame
, but actuallyassertSame, assertNotSame, assertIdentical, assertNotIdentical
. Also, not nuking them, since something likeassertSame(FALSE, strpos($x, $y))
should be replaced byassertStringNotContainsString
for example - separate issues for that.Comment #16
mondrakeRerolled and addressing #15.
Comment #17
longwaveI applied the patch and can only find a couple of stragglers with grep - I guess these probably want to be assertNotNull() anyway, which is out of scope? If so these can be done elsewhere and this is RTBC.
Comment #18
mondrake#17 exactly, there are cases where more appropriate assertions can be used.
Comment #19
catchCould we open a follow-up for #17?
Comment #20
jungleAn issue created for #17, and added a new meta issues for it and for issue like #3126965: [backport] Replace assert* involving count() and an integer literal with assertCount()
Comment #22
catchCommitted 5132c83 and pushed to 9.1.x, cherry-picked to 9.0.x. Thanks!