Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
1.03 KB

Let's see what testbot says.

sun’s picture

We should try to get rid of the resetAll() instead. — I don't even understand why that exists in the first place...

Status: Needs review » Needs work

The last submitted patch, 1: 2189411.1.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.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.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.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.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
850 bytes

Let's see what happens if we do this now.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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.

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

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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

Component: simpletest.module » phpunit

Triaging issues in simpletest.module as part of the Bug Smash Initiative to determine if they should be in the Simpletest Project or core.

This looks like it a Phpunit issue, changing component.

alexpott’s picture

Title: Remove an unnecessary container rebuild from WebTestBase » Remove an unnecessary container rebuild from FunctionalTestSetupTrait
alexpott’s picture

This is definitely still an issue - I was looking at this code a couple of days ago and thought this was silly and also that I'd filed an issue about it before - TIL - 6 years ago.

longwave’s picture

This was improved in #2795789: Do not rebuild the container so often in BTB so not sure this is still relevant?

quietone’s picture

Yes, the rebuild of the container was removed in #2795789: Do not rebuild the container so often in BTB.

But there is another code change in this small patch. I can't comment if this is needed or not.

+++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
@@ -220,6 +220,9 @@ protected function resetAll() {
+    // $this->drupalGet() will fail.
+    $this->prepareRequestForGenerator();
longwave’s picture

$this->prepareRequestForGenerator(); is already called from rebuildContainer() so I think that part of #10 is obsolete as well.

There is also an @todo linking to #2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation so I am fairly sure this issue can be closed as duplicate or obsolete.

alexpott’s picture

Version: 8.9.x-dev » 9.2.x-dev
FileSize
874 bytes

There is a rebuild that we should be able to remove.

alexpott’s picture

The last submitted patch, 19: 2189411-19.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 20: 2189411-20.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
3.43 KB

\Drupal\views\Tests\AssertViewsCacheTagsTrait::assertViewsCacheTags() is creating the request so that an unprimed url generator won't be able to create the correct absolute URL. This only works by accident at the minute :(

alexpott’s picture

I think there's one more container rebuild that we can get rid of. Container's are cached in the database and this is specific to the site.

alexpott’s picture

Here's a comparison of running the InstallerPerformanceTest with Blackfire instrumentation - https://blackfire.io/profiles/compare/7b6854ab-d5e8-49bf-95ac-f09b2f8bfe...

The change in #24 doesn't do much because we're rebuilding the container after the install anyway. This is because we don't allow the install container to be dumped - see \Drupal\Core\Installer\InstallerKernel::initializeContainer(). But I think it is good leave in because it is incorrect and unnecessary.

So this change results on one less container rebuild and one less router rebuild.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

The change in InstallerPerformanceTest proves that there is one less rebuild, and the Blackfire results back this up.

alexpott’s picture

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

I think we need to file two follow-ups before this can be RTBC.

  1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -556,11 +557,19 @@ public function installDrupal() {
    +    _drupal_flush_css_js();
    

    I think this indicates that we have a subtle installer bug. The system.css_js_query_string should be set during an install but it's not because we don't call drupal_flush_all_caches during the installer - and have not for ages. This has been hidden because we've been calling it in tests - so removing that call exposes the bug.

  2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -556,11 +557,19 @@ public function installDrupal() {
    +    Url::fromRoute('<front>')->setAbsolute()->toString();
    

    I think we should have a follow-up to remove this in Drupal 10. Tests should create request's properly - see changes to FrontPageTest here - but let's not break it in this patch.

Berdir’s picture

1. Could also be related to the memory cache backend we use during install and update, we also had to add a drush cc js+css in our deploy scripts because something is not properly cleared during the cache clear that happens during updates.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
4.61 KB

I've created the follow-ups:

@Berdir I don't think this is memory cache related see #3207893: Set system.css_js_query_string during install - would be interested in an issue outlining the problems you are seeing with updates.

longwave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

Followups filed, new @todo comments are helpful, this is ready to go.

larowlan’s picture

Crediting @longwave for reviews and forensics/triage
Crediting @quietone for forensics/triage

larowlan’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Bug Smash Initiative

Committed db3ad8f and pushed to 9.2.x. Thanks!

I think there is the possibility of disruption to contrib tests here, so erring on the side of caution and leaving it as 9.2 only at this point rather than backporting.

Happy to hear arguments for backporting.

  • larowlan committed db3ad8f on 9.2.x
    Issue #2189411 by alexpott, dawehner, longwave, quietone: Remove an...

Status: Fixed » Closed (fixed)

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