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
We deprecated TestBase in #2847678: Deprecate WebTestBase and its required classes in favour of BrowserTestBase but
- \Drupal\simpletest\TestBase::filePreDeleteCallback
- \Drupal\simpletest\TestBase::insertAssert
are still used outside this class in run-tests.sh
And \Drupal\simpletest\TestBase::deleteAssert() exists. I don't think we should do anything about this method. It doesn't make sense - you should never delete an assert on itself - why would you do that?
Proposed resolution
Move \Drupal\simpletest\TestBase::insertAssert
somewhere else.
Use \Drupal\Tests\BrowserTestBase::filePreDeleteCallback
instead of \Drupal\simpletest\TestBase::filePreDeleteCallback
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#20 | 3056877-20.patch | 5.13 KB | Lendude |
#20 | interdiff-3056877-8-20.txt | 413 bytes | Lendude |
#17 | 3056877-8.patch | 5.2 KB | alexpott |
#11 | 3056877-11.patch | 6.49 KB | alexpott |
#11 | 8-11-interdiff.txt | 1.29 KB | alexpott |
Comments
Comment #2
alexpottComment #3
Lendudethis self:: is not going to work here :)
Other then that, this indeed seems like the most straight forward way of fixing this
Comment #4
alexpottFixing the fail... might write a test.
Comment #6
alexpottOkay cool at least we know that if this code is broken stuff breaks. So we can't properly deprecate but hey we can't properly deprecate TestBase either so /shrug.
Comment #8
alexpottSo there are situation where TestBase is loaded but not simpletest.module. Nice.
Comment #9
alexpottWe know that simpletest.module is loaded here because we inject the simpletest module into the test kernel - see \Drupal\Core\Test\TestRunnerKernel::__construct()
Comment #10
LendudeRechecked and all the fails in #6 are in the remaining Simpletest. It is of little consequence that they keep their dependence on \Drupal\simpletest\TestBase::insertAssert, so keeping \Drupal\simpletest\TestBase::insertAssert in tact for that sound like a fine work around for me.
Comment #11
alexpottI checked all the other static functions in classes that extend from TestBase and we have some usage of
\Drupal\system\Tests\Update\DbUpdatesTrait
so fixing that here.There are also some other things that should be moved or refactored out of src/Tests:
Comment #12
alexpottComment #13
alexpottCreated the followups:
#3057289: Move JsMessageTestCases out of src/Tests
#3057288: Move ViewTestData out of src/Tests
Well they're not really followups - more like things noticed along the way.
Comment #15
alexpottThe fail is something happening on testbot.
Comment #17
alexpottAh interesting. So the move of DbUpdatesTrait is not really correct. Interesting problem because this code needs to be autoloadable in the regular Drupal runtime because it is used in test module's hook_install(). So let's file a follow-up to deal with that and go back to the rtbc #8
Comment #18
alexpottPeople have been down this rabbit hole before - so no need for a follow-up - it's in hand in #2969965: Fix Drupal\system\Tests\Update\DbUpdatesTrait is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
Comment #19
catchThe comment is no longer relevant since we're not in the class with the storeAssertion() method any more.
Comment #20
LendudeRemoved the comment, changing it to say storeAssertion is protected seems like stating the obvious.
Comment #21
alexpottThanks @Lendude +1 to removing the comment
Comment #23
catchCommitted 8426bf5 and pushed to 8.8.x. Thanks!