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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcdruid created an issue. See original summary.

mcdruid’s picture

Status: Active » Needs review
FileSize
711 bytes
borisson_’s picture

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.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

Status: Needs review » Needs work
Issue tags: +Novice

> 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.

ilya.no’s picture

ilya.no’s picture

Status: Needs work » Needs review
joachim’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the patch! Looks good.

iyyappan.govind’s picture

+1 applied the patch without fail. Working fine. Thanks

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So 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

    // Enable page caching.
    $config = $this->config('system.performance');
    $config->set('cache.page.max_age', 3600);
    $config->save();

to UserPasswordResetTest::setUp() - which might make sense since it's more explicit and easier to tell what's going on.

iyyappan.govind’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

@Alex +1 Added the page caching in setup method. Please review the patch.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tvb’s picture

Status: Needs review » Reviewed & tested by the community

Patch 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

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a re-roll against 9.1.x

aleevas’s picture

Added re-roll against 9.1.x

tvb’s picture

Issue tags: -Needs reroll

New patch applies cleanly to 9.1.x-dev.

Removing needs reroll tag.

iyyappan.govind’s picture

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
336 bytes

I've added a diff between the patches in 12 and 15 to see the differences in the reroll. It look fine to me.

alexpott’s picture

Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 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.

  • alexpott committed ed11f4f on 9.1.x
    Issue #2978398 by ilya.no, mcdruid, iyyappan.govind, aleevas, quietone,...

  • alexpott committed 7e25c77 on 9.0.x
    Issue #2978398 by ilya.no, mcdruid, iyyappan.govind, aleevas, quietone,...
xjm’s picture

Version: 9.0.x-dev » 8.8.x-dev
Status: Fixed » Patch (to be ported)

This presumably affects Drupal 8 as well?

alexpott’s picture

Status: Patch (to be ported) » Fixed

Using 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.

  • alexpott committed 1fb3c19 on 8.9.x
    Issue #2978398 by ilya.no, iyyappan.govind, mcdruid, aleevas, quietone,...

  • alexpott committed da8289f on 8.8.x
    Issue #2978398 by ilya.no, iyyappan.govind, mcdruid, aleevas, quietone,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.