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
AssertLegacyTrait::pass
is meant for removal in Drupal 10.
Proposed resolution
Deprecate the method and remove its usage in core.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#48 | rawdiff_42-47.txt | 76.83 KB | mondrake |
#48 | rawdiff_42-46.txt | 71.25 KB | mondrake |
#47 | 3123120-47-8.9.x_branch.patch | 148.08 KB | mrinalini9 |
#46 | 3123120-46-9.0.x_branch.patch | 148.24 KB | mrinalini9 |
Comments
Comment #2
mondrakeLet's start with a deprecation trigger and see what neeeds to be fixed.
Comment #3
mondrakeComment #5
mondrakeIt's not straightforward to replace these calls. Those that are there just to log a test entry for display in Simpletest, can be just removed since they have no point in PHPUnit. More complicated is the case when they are part of a
try...catch
construct to assert an exception is thrown. Sometimes they can be replaced byexpectException
, but need to be careful.For the time being, making changes to the test *base* classes. Maybe this needs to be spinned off in sub-issues.
Comment #7
mondrakePostponed on #3123253: Remove usage of AssertLegacyTrait::pass() from base test classes, so we can do a limited size patch there and discuss.
Comment #8
mondrakeComment #9
mondrake#3123253: Remove usage of AssertLegacyTrait::pass() from base test classes got committed, but I'd still suggest to keep this postponed on #3114617: Properly deprecate methods in Drupal\FunctionalTests\AssertLegacyTrait and Drupal\KernelTests\AssertLegacyTrait, while still keeping deprecations silenced. Once that is in, this could be a reference type of issue to remove the silenced deprecation, all the usages, and add in a deprecation test.
Comment #10
mondrakeComment #11
mondrakeThere are uses in traits, too, that generate most of the failures.
Comment #12
mondrakeFiled #3135077: Remove usage of AssertLegacyTrait::pass() from traits
Comment #13
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #14
mondrakeTesting #3135077: Remove usage of AssertLegacyTrait::pass() from traits with the deprecation silencer removed to see how many are left out.
Comment #15
mondrakeUnpostponing, #3135077: Remove usage of AssertLegacyTrait::pass() from traits got committed.
Comment #16
mondrakeOn this.
Comment #17
mondrakeRemoving deprecation silencer, adding deprecation test, started removing usage (complete for Database kernel tests).
Comment #18
mondrakeComment #19
longwaveLet's fix upset -> upsert while we are here
Does ' need escaping in the annotation?
Comment #20
mondrakeThanks @longwave, will take #19 in next patch.
Comment #22
mondrake#19 fixed, continued removing usages, more to do.
Comment #23
mondrakeComment #24
mondrakeComment #26
mondrakeThis may turn out to be green?
Comment #28
mondrakeNever, never say #26... :)
Comment #29
mondrakeFor review.
Comment #30
longwaveQuite a few places where the failure message can be merged straight into the fail() method call to improve test readability, a couple of other questions.
Do you think it's worth opening a followup to refactor some of "Expected exception; just continue testing." into individual test methods that test only one thing and so can use expectException?
Here we can refactor away $message.
Here we can refactor away $msg.
Same here
Same here
pass() returns void but this actually fixes this method to match the docblock, so this is better!
Much nicer :)
$message can be refactored away here.
Same here.
Same here.
And again.
$message can be refactored away here.
And again.
And again.
Much cleaner!
When wouldn't $e be an Exception inside this catch block?
Can this use expectedException and expectedExceptionMessage?
All the $message variables can be refactored away here too.
Comment #31
mondrakeThanks @longwave, yes, a follow up could be opened for most of that.
In general, the tricky part here is that in some tests, if you just remove the
$this->pass
call, you end up with a test with no assertions, hence 'risky' in PHPUnit world, and failure of the test. That's the reason for #30.15, for example.On another note, there is a subtlety in using
$this->expectException
. Using this method means that the test execution STOPS after the exception is raised and intercepted. For this reason, we use thetry...catch
block instead in some cases, that allow keeping the test running. But also, this means that any method that extends a core test and expect continuing execution after callingparent::testXXX
, will silently not executing if we turn its parent to use expectException.This to say this probably requires more discussion in a follow up.
Comment #32
mondrakewill take some of #30 here, on it
Comment #33
mondrake#30:
1, 2, 3, 4, 7, 8, 9, 10, 11, 12, 13, 17 - Done
5 - see #3131900: Refactor assertions that assign return values to variables and #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods for follow-ups
6, 14 - :)
15 - Changed to test the appropriate exception
16 - see #31
Comment #34
longwaveThanks! The expectedException vs try/catch stuff is tricky and I wish we could just continue after an exception but I also see why we can't actually do that, so let's leave that all alone for now. Otherwise all issues look to be resolved so this is RTBC for me.
Comment #35
mondrakeRerolled, I thought some recent commits were impacting the patch, in fact they didn't, still I have this one freshest.
Comment #36
mondrakeReroll after commit of #3135538: Replace remaining assert* involving use of count() where relevant, only patch context changes solved by 3-way merge.
Comment #37
catchNeeds another re-roll.
Comment #38
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedRerolling for 9.1.x!
Comment #39
longwaveThanks for rerolling!
Comment #40
catchNeeds yet another re-roll.
On reviewing this - I agree we should open the follow-up to refactor tests that could potentially use expectException() - the current ::pass() calls inside catch are giving us the illusion of test coverage and we don't lose anything by removing them, but refactoring the tests would improve things.
Comment #41
mondrake#40 filed follow-up meta #3158997: [meta] Refactor tests that use try...catch blocks to use expectException instead. I think we'll need a meta there since each case of try...catch will likely need a refactoring of its own, big work.
Comment #42
mondrakeRerolled.
Comment #43
mondrakeComment #45
catchFixed this typo (via cspell) on commit.
It would be good to backport the test changes (but not the deprecation itself) to 9.0.x and 8.9.x
Comment #46
mrinalini9 CreditAttribution: mrinalini9 commentedAdded patch for 9.0.x and 8.9.x branch, please review.
Comment #47
mrinalini9 CreditAttribution: mrinalini9 commentedPlease ignore previous patch for 8.9.x branch. Here is the updated patch after fixing PHPLint issue in #46, please review.
Comment #48
mondrakeThanks, rawdiffs look good.
Comment #49
catchCommitted/pushed the respective patches to 9.0.x and 8.9.x, thanks!
Comment #52
mondrake