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.
At the moment:
class UserPasswordResetTest extends PageCacheTagsTestBase
...where:
class PageCacheTagsTestBase extends WebTestBase
AFAICS there's no need for this (it seems a bit odd?) and UserPasswordResetTest
can extend WebTestBase
directly.
Comment | File | Size | Author |
---|---|---|---|
#20 | diff-12-15.txt | 336 bytes | quietone |
#16 | 2978398-9.1-15.patch | 1.14 KB | aleevas |
Comments
Comment #2
mcdruidComment #3
borisson_Sure, that makes sense. I'm not sure why this is still a webtest though. Not sure if we should do this now or we should do this when we convert the user tests to the BTB tests.
Comment #6
joachim CreditAttribution: joachim as a volunteer commented> Not sure if we should do this now or we should do this when we convert the user tests to the BTB tests.
If BTB means BrowserTestBase then user tests already seem to be using this. And the parent class in question here, Drupal\Tests\system\Functional\Cache\PageCacheTagsTestBase, extends BTB.
Patch looks sensible to me, but needs a reroll for the new base class.
Comment #7
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedAttaching new patch.
Comment #8
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedComment #9
joachim CreditAttribution: joachim as a volunteer commentedThanks for the patch! Looks good.
Comment #10
iyyappan.govind+1 applied the patch without fail. Working fine. Thanks
Comment #11
alexpottSo this results in \Drupal\Tests\system\Functional\Cache\PageCacheTagsTestBase::setUp() not being run. So there's a question as to why UserPasswordResetTest added this. Maybe there is a reason. This was added in #2461105: One-time password reset page should never be cached so by making this change we're actually breaking the test coverage added by that change.
In order to find this out I did
git log -S "UserPasswordResetTest extends PageCacheTagsTestBase"
to find out when this was added.If we want to make this change then we need to copy
to UserPasswordResetTest::setUp() - which might make sense since it's more explicit and easier to tell what's going on.
Comment #12
iyyappan.govind@Alex +1 Added the page caching in setup method. Please review the patch.
Comment #14
tvb CreditAttribution: tvb commentedPatch applies cleanly to 8.9.x-dev.
Test ran successfully so changing status to RTBC.
www-data@6ea5dd27f4c4:/app/core$ ../vendor/bin/phpunit -c phpunit.xml modules/user/tests/src/Functional/UserPasswordResetTest.php
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\user\Functional\UserPasswordResetTest
....... 7 / 7 (100%)
Time: 3.69 minutes, Memory: 6.00 MB
OK (7 tests, 176 assertions)
HTML output was generated
Comment #15
catchNeeds a re-roll against 9.1.x
Comment #16
aleevasAdded re-roll against 9.1.x
Comment #17
tvb CreditAttribution: tvb commentedNew patch applies cleanly to 9.1.x-dev.
Removing needs reroll tag.
Comment #18
iyyappan.govindComment #20
quietone CreditAttribution: quietone as a volunteer commentedI've added a diff between the patches in 12 and 15 to see the differences in the reroll. It look fine to me.
Comment #21
alexpottCommitted and pushed ed11f4f853 to 9.1.x and 7e25c77a06 to 9.0.x. Thanks!
Backported to 9.0.x since this is a test-only change.
Comment #24
xjmThis presumably affects Drupal 8 as well?
Comment #25
alexpottUsing the patch in #12 then.
Committed and pushed 1fb3c1959c to 8.9.x and da8289fdca to 8.8.x. Thanks!
Backported to 8.8.x as well.