Enable BigPipe by default in the Standard install profile

And patch :)

This fixes most failures: updates the cache context assertions in two tests.

Works for me!

8.4 is open for development now. But I think it's still too early in the cycle. Already tagging though, this should help prevent us to forget to assess this.

Thanks, and sorry!

Reuploading #5, because some of the failures were random update path test failures that have since been resolved.

And this updates the expected cache contexts for NodeTranslationUITest.

And this then finally updates StandardTest, to execute Javascript, wait for BigPipe to finish sending content before asserting a raw string, and updates the expected raw string from <br /> to <br>.

Now tests pass.

#18 failed due to a malfunctioning testbot:

00:33:21.038 Drupal\system\Tests\Module\InstallUninstallTest              3202 passes                                      
00:33:42.775 Drupal\views_ui\Tests\PreviewTest                            237 passes                                      
00:33:46.108 Drupal\views_ui\Tests\HandlerTest                            516 passes                                      
00:34:00.017 Drupal\views\Tests\Plugin\ExposedFormTest                    131 passes                                      
00:34:32.145 Drupal\views_ui\Tests\DisplayTest                            361 passes                                      
01:50:00.019 Build timed out (after 110 minutes). Marking the build as aborted.
01:50:00.024 Build was aborted
01:50:00.024 [CHECKSTYLE] Skipping publisher since build result is ABORTED
01:50:00.024 Archiving artifacts
01:50:00.034 Checking console output
01:50:00.038 Recording test results
01:50:00.056 ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?
01:50:00.062 Finished: ABORTED

Reuploading #18.

Ah, damn, I thought #18 fixed the last problem, but there's one more: StandardInstallerTest also fails. Will work on fixing that.

Well that's annoying.

The StandardInstallerTest failure is legitimate. It uncovered a new edge case, one that we had hitherto assumed could not possibly happen: a message displayed on the very first request to Drupal.

After installing Drupal, \install_finished() does this:

  $success_message = t('Congratulations, you installed @drupal!', array(
    '@drupal' => drupal_install_profile_distribution_name(),
  ));
  drupal_set_message($success_message);

It doesn't show this message on the last page of the installer… but on the first page thereafter.

That first page thereafter is the first page that is rendered by Drupal itself; everything before that is rendered by the installer, which is a special snowflake: it's a limited environment, where advanced features are not available, including BigPipe.

Because it's the first pageload, and because StandardInstallerTest uses WebTestBase which does not support executing JS, we follow the no-JS redirect. Which means that this is what happened:

1. Step 1: request the frontpage. Rendered by BigPipe, using JS BigPipe placeholders. Includes rendered messages. But includes the no-JS redirect.
2. Step 2: since this browser does not support JS, we follow the redirect.
3. Step 3: request the frontpage. Rendered by BigPipe, using no-JS BigPipe placeholders. There are no messages to render, because they were already rendered in step 1!

This assertion was added in #2458925: Screen is black and completely unreadable in Configure page after install on standard profile:

    $this->assertRaw(t('Congratulations, you installed @drupal!', array(
      '@drupal' => drupal_install_profile_distribution_name(),
    )));

… but this was picked rather arbitrarily. We could just as well verify some other text on the default installer page. So, changing that to:

$this->assertRaw('No front page content has been created yet.');

Finally: if you're wondering but shouldn't we fix this in BigPipe?, then the answer is: it's something that only ever happen in the installer, because that's the only way you can have your first page load already have a session (because we're coming from the installer, which doesn't use regular rendering system) and a status message. Without a session, no status message. Without a session, no BigPipe.

Let's re-test. Because core has changed since Feb 20, but also because the patch in #25 failed, but d.o is not linking to the testbot results for some reason…

Looks good, thank you, @Yogesh Pawar!

Exciting, a green patch!

Per #8 and #9, the only thing really preventing an RTBC 8 months ago was the fact that BigPipe should first be shipped with an entire minor cycle: Drupal 8.3. Since then, no significant bugs have been reported. In fact, since creating this issue, only 4 issues were created:

1. #2863607: Convert WebTestBaseTests of BigPipe to BrowserTestBase, which is about modernizing our test infrastructure
2. #2886657: BigPipe changes CSS evaluation order, which was a support request
3. #2903614: Race condition results in same CSS+JS being loaded twice: race between BigPipe's server-side dynamic asset loading and Quick Edit's client-side dynamic asset loading, which is at worst a normal bug, but it's not yet certain that it's actually a bug
4. #2903623: BigPipe test hardening: check that CKEDITOR exists to prevent script throws error immediately , which is about hardening a test

So I think that given that, BigPipe has proved its stability.

Therefore: RTBC'ing since this is green + a release manager decision.

01:50:00.487 Build timed out (after 110 minutes). Marking the build as aborted.

So apparently there is still a problem here.

Yet the previous testrun was timely and successful: https://dispatcher.drupalci.org/job/drupal_patches/27621/console … so it seems like this is just test infrastructure problems? Retesting…

Re-testing.

Let's get this in Drupal 8.5!

The test took 45 minutes (on Oct 13, 18:41 CEST). The closest D8 HEAD test run occurred on Oct 13, 20:36 CEST and took 48 minutes. Strangely, with different concurrency levels. AFAICT on 2 different bots. In any case, enabling BigPipe for the Standard install profile doesn't seem to make a material difference to test runs.

I don't think there's anything blocking this from becoming RTBC now. BigPipe will have been a stable module in Drupal 8 core for two release cycles (8.3 + 8.4). If material problems arise, we can still revert this. I've gotten the question Why isn't BigPipe enabled by default? many times now.

Fully agreed, of course.

But getting to the bottom of this is going to be very, very difficult, because the successful test runs are not at all slower than HEAD’s test runs. That’s what I explained and researched in #41. How one test run of the patch can be indistinguishable from a HEAD test run and another shortly thereafter can time out after taking more than twice as long is a complete mystery to me 😥

I’ll work with the testbot maintainers to get to the bottom of this.

Pinged mixologic.

😵

Wow, I'd never have been able to do such a clear root cause analysis! Thank you so much, @Mixologic! 👏

(Committers, please credit @Mixologic!)

What test coverage do we lose?

I just queued a re-test of #50, and also queued tests in every available combination of environments.

That's a lot of tests, but should help us confirm that the random timeouts are a thing of the past.

(We can then of course still look into @dawehner's concern! Just trying to confirm that Mixologic's analysis was as flawless as I think it is 😀)

PageCacheTagsIntegrationTest was moved by #2870457: Convert web tests to browser tests for page_cache module.

Rebased.

Oh, of course! Sorry for not realizing that. Will work on reroll that keeps all other tests except one in Functional rather than FunctionalJavascript.

On Feb 20, 2017, in #17, StandardTest failed. It failed to find certain elements. It looks like since then, our test dependencies/infrastructure changed, because I can now get StandardTest to pass (at least locally) without changing anything about it. That'd of course be the ideal solution to @dawehner's remark: simply being able to remove the change altogether :)

(Ironically, then @Mixologic's research in #49 also would not have been necessary.)

Let's see if this is also the case on testbot.

+++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
@@ -21,13 +21,15 @@ class NodeTranslationUITest extends ContentTranslationUITestBase {
+    'cookies:big_pipe_nojs',

+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php
@@ -76,6 +76,9 @@ public function testPageCacheTags() {
+      'cookies:big_pipe_nojs',

Any test that uses the standard install profile that also tests cache contexts will have something like this.

We already have extensive, detailed test coverage for BigPipe itself.

I think that's sufficient? Of course, the whole point about BigPipe is that it is a transparent change: if we'd have to change lots of tests, that'd prove that it wasn't transparent!

One thing I can think of that'd be a useful smoke test: we can add explicit test test coverage to verify that e.g. the comment form for an article is served via BigPipe in the Standard install profile.

Alright :) 👍

Thanks, @Bojhan, @Gábor Hojtsy, @yoroy and @webchick! Much appreciated! ❤️

I agree that having #2632750: Interface previews/skeleton screens through optional "preview" or "placeholder" templates would make things even nicer. I'll try to push that one along again too.

Right now, the only blocker here is me working on the expanded/improved test coverage that @dawehner and I discussed in #60–#63. This issue is already assigned to me for that.

I'm worried that dynamic page components - like for example an integrated Hubspot Form - would be invisible to SEO using this method.

With 100% certainty this is not the case. To convince yourself: install BigPipe, see how it uses JS if your browser supports JS, then disable JS, and see how it still streams the content, but doesn't use JS.

This very concern was already addressed before BigPipe was added to core. If it broke such use cases, it A) never would've been added to core, B) would have caused lots of bug reports in the >1.5 years it's been in core.

Also Big Pipe only applies to logged in traffic - so if your site never uses logged in users but for site content changes - Big Pipe doesn't do anything for you.

Correct. So let's make things faster for site builders :)

Implemented @dawehner's suggested additional test coverage.

Failed due to:

00:54:03.460 Fatal error: Cannot declare class Drupal\Tests\standard\Functional\StandardJavascriptTest, because the name is already in use in /var/www/html/core/profiles/standard/tests/src/FunctionalJavascript/StandardJavascriptTest.php on line 0

I think this might fix it. (I can't reproduce the failure locally.)

Content being loaded later, might cause issues with things like google analytics.

The main content is never loaded via BigPipe, for this very reason: serving an empty shell is pointless, we want to ensure that the main content is always available from the very start (i.e. the reason for the user to visit this page/URL).

Furthermore, this only affects new sites using the Standard install profile. We want to show Drupal at its best out of the box, don't we?

Product manager sign-off in #67:

Making users' sites faster out of the box sounds like a good thing to do. :) So no objections from the product management team. Makes total sense.

Patch sign-off in #74:

I think this patch is ready to go.

So I'd RTBC this again, but we still have a Needs release manager review tag.

OMG yay! 🎉

This probably merits a mention in the 8.5.0 release notes?

Thanks, Alex! 😀

WOOT! 😊