Must verify:

  1. an automatic redirect is performed, if and only if JS is disabled (i.e. using <noscript>)
  2. only one redirect is ever performed
  3. the presence of the BigPipe no-JS cookie after the redirect was followed
  4. the redirect redirects back to the original location
  5. the redirect response must have the cookies:big_pipe and session.exists cache contexts, because it depends on those
  6. all of the above must occur for any user with a session, be it an authenticated or an anonymous user
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
5.73 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2674890-2.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.37 KB
2.66 KB

So, this is the very first patch that actually tests entire pages generated by BigPipe! It doesn't test how those pages are delivered overall, but it's the first time BigPipe (in its no-JS form) is delivering a (very simple) page.

And in doing so, this is causing two PHP errors:

Undefined index: ajaxPageState
Unsupported operand types

Why haven't these been discovered before? Because before, we were testing on actual sites, whereas this test is using the testing install profile. That install profile only installs required modules. Which has as an indirect consequence that the core/ajax asset library is not being loaded on all pages anymore.


Steps to reproduce:

  1. Install BigPipe
  2. Ensure the core/ajax asset library is not loaded on the test page
  3. Disable JavaScript
  4. Either log in or trigger a session for the anonymous user
  5. BAM, those two errors

The solution is quite simple: don't assume core/ajax, and hence also the ajaxPageState JS setting in BigPipe's no-JS code paths.

Status: Needs review » Needs work

The last submitted patch, 4: 2674890-3.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.59 KB
970 bytes

This failure:

The first response varies by the "cookies:big_pipe_nojs" and "session.exists" cache contexts.

is a test expectation for point 5 in the IS.

BigPipeController does not yet do this. So let's do that.

Wim Leers’s picture

FileSize
4.82 KB
3.9 KB

And just some clean-up.

Status: Needs review » Needs work

The last submitted patch, 7: 2674890-7.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.4 KB

Oops, the patch equalled the interdiff.

Here's what #7 should've looked like.

Status: Needs review » Needs work

The last submitted patch, 9: 2674890-7.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.31 KB
1.89 KB

The two remaining failing assertions I cannot reproduce locally:

fail: [Other] Line 61 of modules/big_pipe/src/Tests/BigPipeNoJsDetectionTest.php:
Raw "
" found

fail: [Other] Line 75 of modules/big_pipe/src/Tests/BigPipeNoJsDetectionTest.php:
Raw "
" found

(Bonus: Either DrupalCI or run-tests.sh are apparently horribly broken, because any HTML syntax is apparently escaped away completely. What is actually being asserted there is: $this->assertRaw('<noscript><meta http-equiv="Refresh" content="0; URL=' . base_path() . 'big_pipe/no-js?destination=' . str_replace($base_url, '', $this->url) . '" />' . "\n" . '</noscript>');.)

Which means this is most likely due to DrupalCI installing Drupal in a subdirectory. Or actually, that destination query string is not a relative URL, it's a Drupal path. My bad. Then this should work.

Status: Needs review » Needs work

The last submitted patch, 11: 2674890-11.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.4 KB
819 bytes

Still same output. I talked to both @catch and @drumm, neither know how to get more useful information out of DrupalCI. Note that it runs with --keep-results, but you can't access those results anywhere…

So, I have to resort to adding explicit test failures that contain debug output that is HTML-escaped, so that I hopefully will be able to read it. Fingers crossed.

If I run ($ sudo -u _www php core/scripts/run-tests.sh --keep-results --verbose --color --url http://tres/ --directory modules/big_pipe) this locally, I get this output

Fail      Other      BigPipeNoJsDetect   61 Drupal\big_pipe\Tests\BigPipeNoJsDe
    &lt;!DOCTYPE html&gt;
    &lt;html lang=&quot;en&quot; dir=&quot;ltr&quot;&gt;
      &lt;head&gt;
        &lt;meta charset=&quot;utf-8&quot; /&gt;
    &lt;noscript&gt;&lt;meta http-equiv=&quot;Refresh&quot; content=&quot;0;
    URL=/big_pipe/no-js?destination=/user/1&quot; /&gt;
    &lt;/noscript&gt;&lt;meta name=&quot;Generator&quot; content=&quot;Drupal 8
    (https://www.drupal.org)&quot; /&gt;
    &lt;meta name=&quot;MobileOptimized&quot; content=&quot;width&quot; /&gt;
    &lt;meta name=&quot;HandheldFriendly&quot; content=&quot;true&quot; /&gt;
    &lt;meta name=&quot;viewport&quot; content=&quot;width=device-width,
    initial-scale=1.0&quot; /&gt;
    &lt;link rel=&quot;shortcut icon&quot;
    href=&quot;/core/misc/favicon.ico&quot; type=&quot;image/vnd.micr

Status: Needs review » Needs work

The last submitted patch, 13: 2674890-13-dbg.patch, failed testing.

Wim Leers’s picture

Testbot/DrupalCI/Jenkins/run-test.sh === epicfail, because even #13 yields exactly zero useful information:

fail: [Other] Line 60 of modules/big_pipe/src/Tests/BigPipeNoJsDetectionTest.php:
Raw "
" found

fail: [Other] Line 61 of modules/big_pipe/src/Tests/BigPipeNoJsDetectionTest.php:


  
    







fail: [Other] Line 75 of modules/big_pipe/src/Tests/BigPipeNoJsDetectionTest.php:
Raw "
" found

… once again more stuff being replaced by whitespace. Sigh.

I'm out of ideas.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.4 KB
930 bytes

I've got one more absolutely crazy/silly trick up my sleeves, which should result in output like this:

Fail      Other      BigPipeNoJsDetect   61 Drupal\big_pipe\Tests\BigPipeNoJsDe
    [!DOCTYPE html]
    [html lang="en" dir="ltr"]
      [head]
        [meta charset="utf-8" /]
    [noscript][meta http-equiv="Refresh" content="0;
    URL=/big_pipe/no-js?destination=/user/1" /]
    [/noscript][meta name="Generator" content="Drupal 8
    (https://www.drupal.org)" /]
    [meta name="MobileOptimized" content="width" /]
    [meta name="HandheldFriendly" content="true" /]
    [meta name="viewport" content="width=device-width, initial-scale=1.0" /]
    [link rel="shortcut icon" href="/core/misc/favicon.ico"
    type="image/vnd.micr

Status: Needs review » Needs work

The last submitted patch, 16: 2674890-16-dbg.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.59 KB
1006 bytes

That worked, LOL. So now let's add a bit more output so I can really debug this.

Status: Needs review » Needs work

The last submitted patch, 18: 2674890-18-dbg.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
10.04 KB
2.71 KB

The output shows that #11 was a step in the wrong direction. Reverting #11 and adding debug output for the second failing assertion.

Status: Needs review » Needs work

The last submitted patch, 20: 2674890-20-dbg.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.94 KB
2.97 KB

I think this is the solution.

Status: Needs review » Needs work

The last submitted patch, 22: 2674890-22-dbg.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.34 KB
1.77 KB
1.92 KB

Indeed it was.

Whew.

Wim Leers’s picture

Status: Needs review » Fixed

This still looks good to me. Let's get this in.

  • Wim Leers committed 337064b on 8.x-1.x
    Issue #2674890 by Wim Leers: BigPipe no-JS detection test coverage (...
Fabianx’s picture

RTBC - in retrospective.

Wim Leers’s picture

Excellent, thanks!

Status: Fixed » Closed (fixed)

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