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
Recommended regex for searching
assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\)
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#78 | raw-interdiff-72-78.txt | 2.08 KB | jungle |
#78 | 3126965-78.patch | 410.15 KB | jungle |
Comments
Comment #2
jungleComment #3
jungleComment #4
mondrakeNice, but I think the scope should be larger: try
assert(?:Same|Identical|Equal|Equals)\(.*count\(
and there's much more to convert.
At least here, let's convert assertSame and assertIdentical, which are equivalent. Note also that the order of expected and actual may be swapped.
Comment #5
jungleComment #6
jungle@mondrake, Thank you! let's enlarge the scope.
Comment #7
jungleComment #8
longwaveDone a few more of these, but a rough grep shows there are about another 1000 to check, most of which need changing. Bumped the version to 9.1.x as I think this will have to be done there first and then backported, and there are several differences between the branches now.
Discovered #3128773: AssertButtonsTrait is unused while looking at this as well, going to leave that one alone as we can't test it.
Comment #9
longwaveActually I realise I checked more than the regex in #4 - there are a lot of
assertTrue(count(...) == ...)
as well, and then things like this:Should we narrow this down a bit to make it easier to review?
Comment #10
jungleThen let's break this into 4 subtasks.
assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\)
, examples:assert.+\(count\(.+\), count\(.+\)(|.+)
, examples:assert.+\(count\(.+\) (===|==) \d+(|.+)\)
, examples:assert.+\(count\(.+\) (>=|<=) \d+\)
, example:Rescope this one to focus on replacing
assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\)
Comment #11
jungle3 more issues created
Comment #12
jungleComment #13
dwwBased on Slack thread, giving this a human-readable title.
Comment #14
dwwThere, that makes more sense:
#3128813: Replace assertEqual() or assertSame() on two calls to count() with assertCount()
#3128814: Replace assert* involving count() and an equality operator with assertCount()
#3128815: Replace assert*() involving greater/less comparison operators with assert(Greater|Less)Than(OrEqual)
;)
Enjoy,
-Derek
Comment #15
jungleComment #16
jungleComing a big patch
Comment #17
jungleFixed PHPLint error but setting back to NW for removing messages.
Comment #18
jungleFixing failed CI run in #17
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedNeeds a reroll.
Comment #20
quietone CreditAttribution: quietone as a volunteer commentedNW for removing message
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedI've had a few interruptions while working on this so I won't be surprised if there are errors.
Messages were left in this files because there were in a for loop.
Messages were left in these files as well, on a first look the message contains a variable and looks useful.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedOops, lost a semicolon.
Comment #23
quietone CreditAttribution: quietone as a volunteer commentedAnother missing semi colon. (In my defense since I updated PhpStorm the editor hasn't been the same.) Lets try one more time.
Comment #24
jungleWell, I am inserting a CONTINUE statement for the next iteration, good luck! 💪
Comment #25
jungleComment #26
quietone CreditAttribution: quietone as a volunteer commented@jungle, thanks. Must be the price I pay for upgrading PhpStorm and not taking a walk today.
And again!! This time I didn't rely on phpStorm.
Comment #27
jungleThanks @quietone! Taking over to continue, found things to do while reviewing.
Comment #28
quietone CreditAttribution: quietone as a volunteer commented@jungle, thanks. I mean to unassign myself before I went to bed and forgot.
Comment #29
jungleComment #30
jungleContinue to #29, reverted out of scope changes, and removed more redundant ones. By reviewing #26, confirmed that assertion messages in loops have not been touched -- that's good.
Thanks!
Comment #31
longwaveIncomplete review as the patch is so big, but some thoughts so far:
There are a lot of
assertCount(0, ...)
in here. Would these be better asassertEmpty()
? This would give more helpful error messages on failure.This one feels better as assertEquals, as we are comparing two counts?
Same here.
Comment #32
dww@longwave Re: #31:
1. +1. assertEmpty() indeed seems better than assertCount(0)
2 + 3: Indeed. I thought we already had that discussion at #3128813: Replace assertEqual() or assertSame() on two calls to count() with assertCount() and agreed to leave them as assertEqual().
However, #NeedsFollowup?
$this->assertEqual(count($options), count($expected_nids));
It's supposed to be:
assertEqual($expected, $actual)
So really the above should be:
$this->assertEqual(count($expected_nids), count($options));
Seems that'll be impossible to grep for, and therefore, it'll be a huge PITA to actually fix those. :/
Thoughts?
Thanks/sorry,
-Derek
Comment #33
jungleOn this
Comment #34
jungleRerolled a patch from #29 first.
Comment #35
jungleThanks @longwave and @dww for reviewing!
$this->assert*(count(), count());
ones. there is an issue filed for that, closed it because of the original one has a better readability, and it's out of the rescoped scope here, I will check it to see if there are any changes like that in the patch and revert them. Reopen/rescope #3128813 if necessaryWhat I am going to do next:
Comment #36
mondrake#35 let's file a follow up to convert assertCount(0, ...) to assertEmpty(...) - it should be checked case by case.
Comment #37
jungleReplaced more, and reverted
$this->assert*(count(), count());
ones foundComment #38
TR CreditAttribution: TR commentedActually, you got that backwards.
The old Simpletest
assertEqual()
uses($actual, $expected)
. That method now lives inAssertLegacyTrait
.The new
assertEquals()
with an 's' has the arguments reversed -($expected, $actual)
I think the confusion over this and the need to reverse the arguments is why core bailed on removing the old
assertEqual()
from D9.0. That change is now deferred until D10.0.AssertLegacyTrait::assertEqual()
looks like this:Comment #39
longwaveRaised #3133291: Add XPath existence assertions as a lot of these are looking for the existence of XPath elements, and I think we can do better.
Comment #40
dww@TR re: #38 wow, confusing! ;) Thanks for clarifying. I didn't even notice the Equal vs. Equals. I'm just used to PHPUnit style assertions, where expected is always first. Too bad we did it backwards in SimpleTestLandia. What a mess. I'll be sure to pay more attention to Equal vs. Equals in the future. ;)
Comment #41
mondrakeNeeds a reroll :(
Comment #42
mondrakeSome review:
This is very hard to read. I suggest to split in two,
This makes us lose the context in the message.
This makes us lose the context in the message.
This makes us lose the context in the message.
This makes us lose the context in the message.
This makes us lose the context in the message.
Need to have a comment why we expect 5.
Needs comments to replace the messages removed.
This makes us lose the context in the message.
This makes us lose the context in the message.
Needs comments to replace the messages removed.
This is hard to read. I suggest to split
This makes us lose the context in the message.
This makes us lose the context in the message.
This makes us lose the context in the message.
This makes us lose the context in the message.
This makes us lose the context in the message.
This makes us lose the context in the message.
Comment #43
jungle@mondrake, thanks for your review.
But to me, this is too frustrating, especially, back and forth many times.
#42 is too long to follow one by one. I have a small 13-inch screen and no dual monitors. I need switching between the browser and the editor frequently to get all points addressed. Similar to #3133162: Replace the start verb Test with Tests in method comments of tests I've worked on yesterday, frankly, it drove me crazy. 41 revision points listed, what a long list!
I would suggest sticking with the scope as the title indicated "Replace assert* involving count() and an integer literal with assertCount()", STRICKLY. leaving the task of removing redundant assertion messages to a separate issue.
If we can agree on that. What to do next is to revert all assertion messages removed.
Comment #44
jungle> If we can agree on that. What to do next is to revert all assertion messages removed.
Actually, If we can agree on that, we can continue with #19 which is a reroll of #18 without messages removed.
Comment #45
jungleAbout redundant assertion messages, so far, there is no feasible definition of what assertion message is redundant or not. Would be great to conduct an agreement before proceeding.
"Removed all assertion messages, except when they're used in a loop" is the one we follow lately, but this rule is fragile. It's not applicable to all cases.
Comment #46
mondrakeHi @jungle yes, I agree with #43 and #44. Sorry with that. Let's focus on the important thing (converting to assertCount), and leave the messages only as a very last step if core committers require so, or for a follow up.
Comment #47
jungle@mondrake, thanks for your agreement!
> Fixed PHPLint error but setting back to NW for removing messages.
I started this in #17, actually. 🤦
First, rerolled a patch from #19
Comment #48
jungleReverting out of scope changes by repeating #30 and #35.2
Comment #49
jungleDue to rescoping, apologize to all for your efforts wasted.
Comment #50
jungleAs there are two meta-issues already, #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages and this one's parent. One is about assertion message removal, one is to replace assertion with more appropriate ones, So I won't put a needs follow-up tag, and I do not think it's necessary.
One more thing that,
assertCount()
is available in PHPUnit 6 and 7, that says it is applicable to all branches down to 8.8.x. So I will work on patch(es) for other branches next.Comment #51
jungleA patch made against 8.8.x
Comment #52
jungleComment #54
jungleComment #55
mondrake@jungle practical suggestion: unless specifically required by a committer, I would suggest to avoid working on backport patches at the moment. You will just spend a lot of time trying to keep 4 versions aligned, better do it when it makes sense. BTW I am not seeing any of these chain of issues backported D8.8, really.
I'll try to review the D9.1 one - it's a 400kb+ patch, it will take some time - but if that's RTBCed and committed then you will have a clear reference for the backport, not sooner.
Any reviewers, patch to review is at #48.
Comment #56
jungleThanks for your suggestion! #3131474: Replace assertions involving calls to array_search() with assertContains()/assertNotContains() is the only one in this series of issues backported to 8.8.x so far
Updating the recommended regex to IS:
assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\)
Comment #57
mondrakeReviewing, just realized #3133291: Add XPath existence assertions is a do-do for follow up... apart from the many
assertCount(n, $this->xpath(...));
, there sre probably even more that assign the xpath result to a variable and assert on the variable itself.A few points:
(and others) just drop the 4th argument instead of changing to NULL, it has no purpose in assertCount
for readibility, I would split this in two statements, one assigning the result of array_filter to a variable, the second to assertCount on that variable
I'd keep these as one-liners if possible
Comment #58
jungleThanks for your review again! On it.
Comment #59
darrenwh CreditAttribution: darrenwh as a volunteer and at Investis Digital commentedSome observations that could be updated.
can use single quotes
exists
The field has gone
The field has gone
AJAX
There is one parent...
There is one parent...
OK
is in the secondary
TID perhaps?
SQL
SQL
with NOT
JavaScript
JavaScript
JavaScript
JavaScript
Comment #60
mondrakeActually, regex in #56 seems too tight.I tried bothassert.*\(count\(
andassert.*\(.*, count\(
and I can see several more that slipped between the cracks... amidst many false positives though, I admit.EDIT - disregard, this comment was not in line with the scope declared.
Comment #61
mondrake#59 - please note we suggest not to touch any message in this issue. What we had before, we'll have after this patch. See #43 and following. The patch is already 400kb+ and if we start questioning messages we will never do it.
Let's address messages separately in one or more follow ups. Actually many of them could be just removed, probably. Please refer to #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages for more discussion.
Comment #62
jungleAddressed #57 only.
@darrenwh, much appreciated for your review. but I'd say sorry. as #61 commented, we now choose to stick with the current scope strictly.
Comment #63
mondrake@jungle thanks. Now it LGTM. The size of the patch is significant, so we propose not to address changes/removals to custom assertion messages here, but to defer that to other issues, to be spin offs from #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages. Grepping the codebase with the regex in IS yields no results. There are potentially more conversions to be done, but those will not involve literal integers, so out of scope here.
Comment #66
catchCommitted 40a5bd7 and pushed to 9.1.x, cherry-picked to 9.0.x. Thanks!
Will need a re-roll for 8.9.x
Comment #67
jungleComment #69
jungleRerolled the patch first, fixing tests next.
Comment #70
jungleReverting the changes to simpletest related assertions as assertCount() is not available in Drupal\simpletest\WebTestBase
Comment #72
jungleTwo more to revert in core/modules/simpletest/src/Tests/BrowserTest.php
Comment #74
jungleA random failure. Queued again
Comment #75
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedOn this.
Comment #76
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedI have reviewed the patch #72 for the 8.9.x version.
Interdiffs submitted in #67, #70, #72 looks good.
The scope of this issue is covered in the patch.
Comment #77
xjmNo longer applies (already). This is another one you can ping me once it's RTBC again. Thanks!
Comment #78
jungleThank you, @xjm, I will ping you once the testing passes.
Comment #79
xjmGreat, thanks! First thing tomorrow I guess then for me, but I'll ping other committers and let them know I'm going to revert anything else that conflicts with this between now and then.
FWIW given the size of this patch I might have scoped it into two issues, one where the
count()
was around the first parameter in the original code, and another where it was around the second. Too late now since it's in D9 already, but just as an example for future reference. :) (It's still maybe in--color-words
territory rather than--porcelain
, although I might also have written a--porcelain
script if I'd been reviewing it on all four branches, or I still might if my eyes start to glaze over too much on it even after sleeping.)Comment #80
alexpottI've scanned every change using git diff --color-words and all the changes are the expected change to assertCount() and nothing stood out as wonky.
Comment #82
catchCommitted 8090ecf and pushed to 8.9.x. Thanks!