Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Child of #3123120: Properly deprecate AssertLegacyTrait::pass to keep scope limited.
AssertLegacyTrait::pass
is meant for removal in Drupal 10.
Proposed resolution
In this issue, remove its usage from base test classes.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff_27-32.txt | 2.55 KB | mondrake |
#32 | 3123253-32.patch | 18.55 KB | mondrake |
Comments
Comment #2
mondrakeComment #3
mondrakeComment #4
mondrakeMany of the calls to
::pass
are actually are there just to log a test entry for display in Simpletest, which is no-op in PHPUnit though. Here converting to::verbose
, that is to be deprecated itself, but potentially replaceable in #2795567: Use Symfony's VarDumper for easier test debugging with dump(), so I thought not to just drop them, but kick them forward.Comment #5
mondrakeComment #7
mondrakeComment #8
longwaveNot really convinced any of these
verbose()
are actually useful, I think we should just consider removing them now. The exception-related refactoring looks fine.Comment #9
mondrakeLet's do #8 then. Good task for novice contributors.
Comment #10
prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commentedComment #11
prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commentedKindly review new patch
Comment #13
Lal_Comment #14
mondrakeDoing #8, starting from #5.
EDIT - xpost with #13
Comment #15
mondrakeForgot an interdiff.
Comment #16
mondrakeComment #17
longwaveI don't think any of these
pass()
calls add any value now we are in PHPUnit - they might have been useful in the Simpletest UI but that is long gone.Comment #18
alexpottThese changes will reduce test coverage. \Drupal\KernelTests\Core\Cache\ApcuBackendTest::testSetGet for example will end now once the exception is thrown in the call to
parent::testSetGet();
. We can't use expectException() in a base class like this - we need to catch the exception as before.This can be removed. It's pointless.
$all_tables_exist = FALSE;
will never be reached because once a phpunit test fails it ends.Comment #19
Lal_Comment #20
mondrakeThanks @Lal_
still we need to remove the calls to
pass
, which is the purpose of this issue. How about catching a\Throwable
, and within the catch block, assert the exception is an instance of\AssertionError
?Comment #21
mondrakeComment #22
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedComment #24
mondrakeInterdiff from #14.
Comment #25
mondrakeComment #26
jungleFile
core/tests/Drupal/KernelTests/KernelTestBase.php
, line9
, Unused use statementComment #27
mondrakeThanks @jungle
Comment #28
longwaveI checked what this method does and it doesn't actually perform assertions, rather it just throws an exception on failure. This means we have to use the
$this->addToAssertionCount(1);
workaround at the top of the method as there are now no assertions here. Worth opening a new issue to make it more PHPUnit-y?Otherwise this all looks good and concerns from #18 have been addressed.
Comment #29
xjmOh hey, I remember being flummoxed back in the day by all the noise that comment tests spat out.
I looked carefully over the removals here to make sure the information from the old
pass()
calls was available somewhere else (e.g., in inline comments). Just a couple points to fix:Very-nit: Comma splice. (The comma should be replaced by a semicolon in each case.)
The assertion message looks wrong here. If the assertion passes, the installation of the table was successful, right? Then The message should be "Successfully installed entity type table for..."
Thanks!
Comment #30
xjmFor #28.
Comment #31
mondrakeOn this.
Comment #32
mondrakeThanks.
#28: filed #3133137: Convert AssertConfigTrait::assertConfigDiff() into a real assertion.
#29:
1. Done.
2. There is #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages where the format of to-be remaining custom $messages is being discussed and hopefully normed. Here, adjusting the message to the latest suggestion in that thread,
{context} should {verb} {subject}
.Comment #33
longwaveAll concerns addressed, back to RTBC.
Comment #38
xjmCommitted to all four branches. Thanks!
Comment #40
jungle