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. We have approximately 100 assertions such as
$this->assertTrue(isset($list['front']) && isset($list['403']) && isset($list['404']), 'Got a list with the right properties for site page data.
');
It is cleaner to assert each of these parts separately.
Steps to reproduce
Proposed resolution
Grep for >assert.*&&
(some false positives)
Split each assertion into multiple lines, improving the assertion type where possible
Remove redundant assertion messages
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff_18-20.txt | 1.62 KB | mondrake |
#20 | 3166450-20.patch | 71.21 KB | mondrake |
Comments
Comment #2
longwaveNot sure what the intent of this is, as neither ajaxPageState nor history seem to be set, never mind the child items or the part after &&, so this is largely useless.
Comment #3
longwaveWork in progress, there are many more to do.
Comment #5
mondrakeBaby steps forward. We can only convert assertTrue, since
assertFalse($a && $b);
requires OR logic, not AND.Comment #7
mondrakeStill about 40 to do.
Comment #8
mondrake15 more to go
Comment #10
mondrakeThis should do. There's 1 more instance searching with
assertTrue\(.+&&
, and that's in a deprecated method in AssertLegacyTrait, that I would leave to rest there.Comment #12
mondrakeFixes.
Comment #13
longwaveThanks for picking this up, I assume you're happy with what I did in #3 except for the test fails, then I can RTBC this once the review issues are fixed.
So much nicer!
I think $expected and $actual are the wrong way round here (and therefore LessThan should be GreaterThan and vice versa), same for a number of other instances in this file.
Comment #14
mondrake@longwave yes, I was happy with #3. Done #13.2.
Comment #15
mondrakeForgot merging with HEAD.
Comment #16
mondrakeComment #17
snehalgaikwad CreditAttribution: snehalgaikwad as a volunteer and at QED42 for Drupal India Association commentedComment #18
snehalgaikwad CreditAttribution: snehalgaikwad as a volunteer and at QED42 for Drupal India Association commentedRerolled the patch for 9.1.x.
Comment #19
longwaveI read through all changes in the patch and only have one comment that doesn't need changes, and one question:
I do wonder if it's overkill asserting that the key exists first, as the second line will fail anyway with an error if the key isn't set. I guess really it doesn't matter though, this and similar instances can be left as-is.
I think $expected and $actual are the wrong way round here?
Otherwise I think this is good to go.
Comment #20
mondrakeThanks.
#19.1 I personally agree that it seems overkill, but here it's a one-to-one conversion of previous code. One way or another, I'd leave to core committers to give input here.
#19.2 Indeed, done.
Comment #21
longwaveLooks perfect now - RTBC assuming testbot agrees.
Comment #23
Krzysztof DomańskiRandom test failure #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest). Back to RTBC.
Comment #25
catchCommitted d834847 and pushed to 9.2.x. Thanks!
Comment #28
catchAnd cherry-picked to 9.1.x
Comment #29
xjm