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 title
Proposed resolution
Regex for searching: >assert.*\(.*( == | === | != | !== )
example:
- $this->assertTrue($output == $expected, new FormattableMarkup('Token recognized in string %string', ['%string' => $input]));
+ $this->assertEquals($expected, $output, "Token is expected to be replaced in string $input");
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#60 | 3132919-60.patch | 79.8 KB | jungle |
#60 | raw-interdiff-57-60.txt | 1.65 KB | jungle |
Comments
Comment #2
mondrakeComment #3
mondrakeStarted.
Comment #4
mondrakeComment #5
mondrakeHere's a patch for review. I deliberately left the comments in, it looks like it's better postpone removals/updates to when #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages will be more mature.
Comment #6
mondrakeComment #8
mondrakeFixing failures.
Comment #9
mondrakeComment #10
mondrakeComment #11
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled patch. Please review!
Comment #12
mondrake#11 failed to apply.
Comment #13
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedRerolled patch to 9.1.x as #11 failed to apply.
Comment #14
jofitz CreditAttribution: jofitz at jofitz commentedRemoved "Needs reroll" tag
Comment #15
jungleThanks all for working on this.
There are 52 matches matching the regex
>assert.*\(.*( == | === | != | !== )
, but none of them meets the scope there. RTBC +1.Leave NR expecting another review or a joint RTBC.
Comment #16
mondrakeNeeds reroll, there is a CS fix required, the @todo in AssertMailTrait can point to #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods
Comment #17
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #18
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented@mondrake Here is the re-rolled patch for 9.1.x could you please review.
There us no setUp function in AssertMailTrait
Thanks
Pavnish
Comment #20
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #21
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented@mondrake Fixed #18 test failed issue here could you please review.
Thanks
Pavnish
Comment #22
mondrakeThanks. Let's see where we can use more strict
assertSame
.Comment #24
msutharsComment #25
mondrakeComment #26
msutharsMade some changes in the patch.
Comment #27
mondrakeComment #28
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch #26, please review.
Comment #29
tvb CreditAttribution: tvb commentedPatches from #28 and #26 failed to apply.
The attached patch is rerolled from #25.
$ egrep -Ril ">assert.*\(.*( == | === | != | !== )" core/ | wc -l
106
$ git apply 3132919-29.patch
$ egrep -Ril ">assert.*\(.*( == | === | != | !== )" core/ | wc -l
49
Comment #30
mondrakeComment #31
mondrakeComment #32
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedFixing Php-lint error in #29
Comment #33
samiullah CreditAttribution: samiullah at Salsa Digital commented@ridhimaabrol. When applying the patch I saw some Hunk Failed messages
Please let me know if this needs to be fixed
Comment #34
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commented@samiullah Please update your branch to the latest head. There are no such issues on the latest head.
Comment #35
abhisekmazumdarThe patch on comment #32 can be applied in 9.1.x-dev. Not moving to RTBC yet. Might need more detailed review.
Comment #36
tvb CreditAttribution: tvb commentedRerolled patch from #32 due to a recent change in core/modules/editor/tests/src/Functional/EditorAdminTest.php (commit a44a9b1cd044d56e7b0c3a7412b38c9292fc83f2).
Comment #37
longwaveI reviewed the patch with git diff --color-words and I think it is looking close. However, should we also be removing useless assertion messages here? There are quite a few we could get rid of.
The old assertions need removing here.
I also grepped for remaining assertions. There are a lot of false positives but a few that I think we should look at here:
This should be split into two assertions.
The ternary operator in each of these cases is a bit odd. Probably better to check $node is an object?
Unsure if this is out of scope, but is this sort of thing better written out like the below? Not sure.
Comment #38
mondrake#37 I suggest to postpone this to #3166450: Split assertTrue using && into multiple assertions where we can better address the cases that need splitting.
Comment #40
mondrake#3166450: Split assertTrue using && into multiple assertions is in, this can be worked on again.
Comment #41
mondrakeOn this.
Comment #42
mondrakePlain reroll of #36.
Comment #43
mondrakeComment #44
mondrakeswap arguments?
same?
same?
seems duplicated code, see lines above
should be reverted, entity query are used now
swap arguments?
assertEquals returns void, so return TRUE should remain
should be reverted?
Comment #45
mondrake#37 seems all self-solved already in other issues.
Comment #46
mondrakeOn this
Comment #47
mondrake#44
.1 - no the order is OK, made a change to make clear what is the actual
.2 - done
.3 - no it's ok
.4 - done
.5 - done
.6 - no it's ok
.7 - done
.8 - done
Comment #48
mondrakeComment #49
mondrakeRerolled, and reverted #47.1 that seems oos here.
Comment #50
longwaveLooking pretty good, just a couple of comments:
Should this be assertStringContainsString()?
Should this be tightened to assertSame()?
Comment #51
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAddressed comment #50, please review.
Comment #52
mondrakeThanks!
#50.1 - ok, I think we can do even better
#50.2 - done, let's see
edit- xposted with #51, sorry
Comment #55
longwaveAlright, I was wrong about #50.2, sorry!
Comment #56
longwave$page is unused now.
Comment #57
mondrake#56 it's needed a bit below
Comment #58
longwaveAh, OK. That looks like it could be $this instead but that's enough scope creep already.
All review points addressed - RTBC if bot agrees.
Comment #59
mondrake#58 you’re right... maybe follow ups to cleanup where we have this type of dance?
Comment #60
jungleRerolled, stay RTBC
Comment #62
catchCommitted cf64091 and pushed to 9.2.x. Thanks!