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
Per title. Regex for searching: >assert(Sa|Eq|Id|Tr|Fa).*\((count\(|.*, count\()
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#20 | 3135538-20.patch | 57.27 KB | mondrake |
#20 | interdiff_16-20.txt | 776 bytes | mondrake |
Comments
Comment #2
mondrakeWorking on this.
Comment #3
mondrakeLet's see. Each case is kind of its own, here. In general, I made comparison stricter using assertSame when both expected and actual use a count(). In such cases I preferred to keep the comparison rather than converting to assertCount and then have the expected value in a count(). Readibility, but I guess it can be seen as a matter of preference.
Comment #5
mondrakeAddressing failures.
Comment #6
jungleThanks, @mondrake! I was about to review this, but the patch did not apply on my local.
Comment #7
nitesh624Assigning to me. I will be working on this
Comment #8
mondrake@jungle the patch is applying, and tests run on DrupalCI for 9.1.x. Maybe your git HEAD was not clean?
Comment #9
jungleSorry! @mondrake! NW for 4 coding standards messages, see https://www.drupal.org/pift-ci-job/1697536
And it looks like that it's hard to check whether there are remaining ones to fix.
Comment #10
mondrakeThanks @jungle. Fixed CS. It's good if it's hard to find more to fix... it means we did most - and if others pop up fine, will probably deserve their own issue.
Comment #11
nitesh624Comment #12
nitesh624#10 patch applied successfully on my local and looks good to me. RTBC1 from my side
Comment #13
xjmThanks @mondrake.
@nitesh624, The automated testing infrastructure tells us whether the patch applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community". "Looks good to me" also isn't enough information for a review credit. Next time, I'd suggest specifically describing your thoughts about the patch, including exactly what you did to review it and what you think of the kinds of changes in the patch. Thanks!
So I'm "hmmm" on this change set.
assertEquals()
toassertSame()
.assertEquals()
.assertCount()
.Can we clarify the "why" in each of the three cases cases?
I do see for point 1 that
assertCount()
is not sufficient for cases where we're comparing the count of two items. In those places it looks like anassertSameCount()
method might be nice, actually. What do you think? We could split the cases where we're comparing two counts out into their own patch to explore that.Comment #14
mondrakeIntroducing an
assertSameCount
would be a good idea, actually. The correct way would be toassertSameCount
andassertNotSameCount
, that invoke the Constraint, à la #3133355: Introduce PHPUnit-compliant WebAssert::responseHeaderExists() method, and its negative.Personally I will hold until those two issues are in, since they will make a reference to all future implementations. Would be lovely to see what @longwave thinks of this.
Also, see #3139405: Replace usages of AssertLegacyTrait::assert(No)UniqueText, that is deprecated.
Comment #15
longwaveNot sure whether we really need assertSameCount() or not. Would we even need a custom constraint or could we just wrap assertSame? And if we did add it, would people discover and use it in the future, or would we just be refactoring for the sake of it and then it would never get used again? The header one is useful because there is no existing easy method for it; assertSameCount() can be emulated by writing the assertion slightly differently, as already seen here.
Also, some of these cases feel like the tests could be refactored to be simpler, but doing that here is out of scope:
This seems like it would be better done as a single assertEquals().
Same.
Is the count check redundant here? It feels like IdenticalResultSet should also check the counts!
Again, can this just be done as a single assertEquals()?
Comment #16
mondrake@longwave thank you. In fact I see #15 in scope here: after #3126965: [backport] Replace assert* involving count() and an integer literal with assertCount() and #3128814: Replace assert* involving count() and an equality operator with assertCount() that did the bulk of the changes, this is to try and address the longtail. So, not just mere string replacements but rather thought over refactors. Then whether this one has to be split further, let's discuss.
I take yours as a -1 for
assert(Not)SameCount
methods, fair enough.Done all of #15 points here.
Comment #17
longwaveI would call it a -0.5 for
assertSameCount
, I might be convinced the other way but I am leaning towards "no" for now :)Comment #18
mondrake#17 Ha! It should be an
int
, not afloat
:PComment #20
mondrakeAddressing the test failure of #16 using
assertEqualsCanonicalizing
that presorts the arrays for comparison.Comment #21
longwaveAll these changes look sensible now.
Comment #22
catchI've opened #3156396: Use assertSameSize() to check same size of two countable variables for assertSameCount() - IMO this would be fine to add, but also using more appropriate assertions in this patch seems good in its own right too. Don't think it hurts to do this in two steps.
Committed d1033a9 and pushed to 9.1.x. Thanks!