Convert WebTestBaseTests of BigPipe to BrowserTestBase

I think you can replace

    $this->curlClose();
    $this->curlCookies = [];
    $this->cookies = [];

inside BigPipeTest with $this->getSession()->reset().

I had started doing this weeks/months ago. It's mighty difficult.

This needs #2862885: Batch: Convert system functional tests to phpunit because of checkForMetaRefresh() use in BigPipeTest

This issue landed #2862885: Batch: Convert system functional tests to phpunit and it unblocked this one. However, this still needs to replace the curlOpen(); and curlClose();
Will link if I will find any follow-up issue for the same otherwise will open a new one.
//Naveen

Here we go with another hit.

Only this file core/modules/big_pipe/src/Tests/BigPipeTest.php is left for conversion. So this is the TODO left.

//Naveen

Managed to get some methods to come back green.

\Drupal\big_pipe\Tests\BigPipePlaceholderTestCases wasn't happy when moved, and didn't feel like finding out why.

\Drupal\Tests\big_pipe\Functional\BigPipeTest::assertBigPipeNoJsMetaRefreshRedirect needed a extensive rewrite since there where more redirects that needed to be blocked and since getResponseHeaders returns something completely different in BrowserTestBase then WebTestBase.

Interdiff is against #2 since that seemed like a good path forward, I don't think \Drupal\Tests\big_pipe\Functional\BigPipeTest should be out of scope.

just this?

Moved BigPipePlaceholderTestCases to a discoverable place. This cleans out the /core/modules/big_pipe/src/Tests dir.

+++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
@@ -446,33 +446,34 @@ protected function assertSessionCookieExists($expected) {
+    $this->drupalGet($original_url);
+    $this->assertEquals($original_url, $this->getSession()->getCurrentUrl(), 'Redirected back to the original location.');

Not happy with this. I couldn't find a way to make the redirect go one step at a time. Once I blocked it, it didn't matter what URL I called, it never seemed to redirect.

First off: thank you so much for pushing this forward! I'm so tempted to mark this RTBC, but I have a few small remarks, and a few bigger ones. This is one of the trickiest tests in Drupal core, because it's what gives us all strong assurances (and hence peace of mind) that BigPipe is actually working correctly. So I am reviewing in the strictest possible way. The fact that I can find so few things to remark upon, says a lot — great job! 👏

1. similarity index 97%
   rename from core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
   
   rename from core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
   rename to core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipePlaceholderTestCases.php
   

   I strongly disagree with this move, because it's also used by BigPipeTest.

2. +++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipePlaceholderTestCases.php
   @@ -90,7 +90,7 @@ public static function cases(ContainerInterface $container = NULL, AccountInterf
   -          'data' => '    <div role="contentinfo" aria-label="Status message" class="messages messages--status">' . "\n" . '                  <h2 class="visually-hidden">Status message</h2>' . "\n" . '                    Hello from BigPipe!' . "\n" . '            </div>' . "\n    ",
   +          'data' => ' <div role="contentinfo" aria-label="Status message" class="messages messages--status">' . "\n" . ' <h2 class="visually-hidden">Status message</h2>' . "\n" . ' Hello from BigPipe!' . "\n" . ' </div>' . "\n ",
   

   Why these changes?

3. +++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php
   similarity index 90%
   rename from core/modules/big_pipe/src/Tests/BigPipeTest.php
   
   rename from core/modules/big_pipe/src/Tests/BigPipeTest.php
   rename to core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   

   Yay!

4. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -59,10 +61,12 @@ protected function setUp() {
   +    $this->getSession()->getDriver()->getClient()->followRedirects(FALSE);
   ...
   +    $this->getSession()->getDriver()->getClient()->followRedirects(TRUE);
   

   This is the magic thing I hadn't found yet in my attempt I mentioned in #5.

5. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -59,10 +61,12 @@ protected function setUp() {
        $this->maximumMetaRefreshCount = 1;
   ...
        $this->maximumMetaRefreshCount = 0;
   

   This was overriding WebTestBase::$maximumMetaRefreshCount — so this can now be deleted.

6. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -59,10 +61,12 @@ protected function setUp() {
        $this->metaRefreshCount = 0;
   

   Same for this.

7. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -95,9 +99,7 @@ public function testNoJsDetection() {
   -    $this->curlClose();
   -    $this->curlCookies = [];
   -    $this->cookies = [];
   +    $this->getSession()->reset();
   

   This is 100x more elegant!

8. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -369,14 +369,14 @@ protected function assertBigPipePlaceholders(array $expected_big_pipe_placeholde
   -    $placeholders = array_map(function(\SimpleXMLElement $element) { return (string) $element['data-big-pipe-placeholder-id']; }, $this->cssSelect('[data-big-pipe-placeholder-id]'));
   +    $placeholders = array_map(function(NodeElement $element) { return $element->getAttribute('data-big-pipe-placeholder-id'); }, $this->cssSelect('[data-big-pipe-placeholder-id]'));
   
   @@ -409,13 +409,13 @@ protected function assertBigPipePlaceholders(array $expected_big_pipe_placeholde
   -    $session_data = $this->container->get('session_handler.write_safe')->read($this->cookies[$this->getSessionName()]['value']);
   +    $session_data = $this->container->get('session_handler.write_safe')->read($this->getSession()->getCookie($this->getSessionName()));
   

   Wonderfully elegant updates here!

9. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -446,33 +446,34 @@ protected function assertSessionCookieExists($expected) {
   -    $original_url = $this->url;
   -    $this->performMetaRefresh();
   -
   -    $this->assertEqual($original_url, $this->url, 'Redirected back to the original location.');
   +    $original_url = $this->getSession()->getCurrentUrl();
    
   -    $headers = $this->drupalGetHeaders(TRUE);
   -    $this->assertEqual(2, count($headers), 'Two requests were made upon following the <meta> refresh, there are headers for two responses.');
   +    // Perform the MetaRefresh without following any additional redirects.
   +    $this->performMetaRefresh();
   

   The changes here are confusing. Removing the newly added comment would make it easier to understand.

   Then the other thing that's noteworthy is that the "Two requests were made upon following…" assertion was removed. Why?

10. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
    @@ -446,33 +446,34 @@ protected function assertSessionCookieExists($expected) {
    +    $this->drupalGet($original_url);
    +    $this->assertEquals($original_url, $this->getSession()->getCurrentUrl(), 'Redirected back to the original location.');
    +    $headers[1] = $this->getSession()->getResponseHeaders();
    

    Why do we need to perform this request separately?

    That means performMetaRefresh() is no longer working in the same way as it was before. Which also means we're not longer actually able to test that the redirect actually works correctly, since we're doing something manually here that wasn't being done manually before.

@Wim Leers thanks so much for the review! And strict reviews are welcome.

#24.1 My goal was to clear out the /core/modules/big_pipe/src/Tests dir, hence the move. But it needed to be in a spot where it could be discovered and this worked (/core/modules/big_pipe/tests/src didn't), really wasn't any additional logic to that, so if you can think of a better place (or just want to leave it where it was), I'm fine with that.

#24.2 Because that made the test pass :) Seems like NodeElement->getText() strips some white space that SimpleXML (string) casts leave in.

#24.3 :)

#24.4 :)

#24.5 That is also a thing in BrowserTestBase, so that's still needed I think

#24.6 That is also a thing in BrowserTestBase, so that's still needed I think

#24.7 :)

#24.8 :)

#24.9 Took the comment out, that was left over from earlier attempt I made at making it pass. And the test was taken out because there doesn't seem to be way to check this in BrowserTestBase. WebTestBase returned all the headers for all the request, and that could be used to determine the number of redirects that were done. Made an attempt at that in this version of the patch.

#24.10 Wasn't happy with that (see my comment in #23). But it seems that once the cookie is set, there is no way to continue the request and follow all the redirects to the end. So new attempt to simulate this: Do the first step of the redirect, check stuff, unset the cookie, go to the original URL and don't block the redirects, and see if at the end of it we end up in the right spot with the right cookie.

1. Reproduced. That's weird. It seems like a bug in \Drupal\simpletest\TestDiscovery::scanDirectory() or something related.
   Fixing that is out of scope here. I tried both Drupal\Tests\big_pipe\BigPipePlaceholderTestCases and Drupal\Tests\big_pipe\Unit\BigPipePlaceholderTestCases and could not get it to work. Very strange. So… then your solution definitely seems the next best alternative! 👍
2. Hm, interesting! It looks like Mink is only keeping unique whitespace characters in sequences. So multiple spaces are merged into a single one. That's fine then!

5. Oh, I didn't notice that! This was fixed only 2 months ago, in #2862885: Batch: Convert system functional tests to phpunit, before then, checkForMetaRefresh() didn't exist yet. So that is why many of the problems I was having are no longer there, yay! :) (I was re-implementing that myself.) Great, then the patch is fine as-is :)
6. See prior point.

9. Hm 🤔 this makes the test more complex and harder to understand than it was before. Trying to come up with a simpler alternative…
10. See prior point.

The only other thing I found was a few comments with now outdated FQCN references: nitpicks. Fixed those already.

To be clear: points 9+10, the performMetaRefresh() changes is the only thing I'm concerned about, the rest looks RTBC-worthy already! :)

1. +++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipePlaceholderTestCases.php
   @@ -2,10 +2,10 @@
   - * Contains \Drupal\Tests\big_pipe\Unit\Render\Placeholder\BigPipePlaceholderTestCases.
   + * Contains \Drupal\big_pipe_test\BigPipePlaceholderTestCases.
   

   Why do we still have this line?

I found the root cause for the chaos!

\Drupal\simpletest\WebTestBase::checkForMetaRefresh() does $refresh = $this->xpath('//meta[@http-equiv="Refresh"]');, which matches both <meta http-equiv="Refresh"… and <meta http-equiv="refresh"…

\Drupal\Tests\BrowserTestBase::checkForMetaRefresh() does $refresh = $this->cssSelect('meta[http-equiv="Refresh"], which matches <meta http-equiv="Refresh"… but NOT <meta http-equiv="refresh"….

The solution is to change the latter to

$refresh = $this->cssSelect('meta[http-equiv="Refresh"], meta[http-equiv="refresh"]');

(This is a regression caused by #2862885: Batch: Convert system functional tests to phpunit.)

The reason this is a problem: \Symfony\Component\HttpFoundation\RedirectResponse uses the lower-case variant… which means any Symfony redirect response will not be followed correctly.

With that bug out of the way, the solution becomes much simpler. We can simply not change anything about \Drupal\Tests\big_pipe\Functional\BigPipeTest::performMetaRefresh(), and the change to assertBigPipeNoJsMetaRefreshRedirect() becomes much simpler: from

    $original_url = $this->url;
    $this->performMetaRefresh();

    $this->assertEqual($original_url, $this->url, 'Redirected back to the original location.');

    $headers = $this->drupalGetHeaders(TRUE);
    $this->assertEqual(2, count($headers), 'Two requests were made upon following the <meta> refresh, there are headers for two responses.');

to

    $original_url = $this->getSession()->getCurrentUrl();

    // Disable automatic following of redirects by the HTTP client, so that this
    // test can analyze the response headers of each redirect response.
    $this->getSession()->getDriver()->getClient()->followRedirects(FALSE);
    $this->performMetaRefresh();
    $headers[0] = $this->getSession()->getResponseHeaders();
    $statuses[0] = $this->getSession()->getStatusCode();
    $this->performMetaRefresh();
    $headers[1] = $this->getSession()->getResponseHeaders();
    $statuses[1] = $this->getSession()->getStatusCode();
    $this->getSession()->getDriver()->getClient()->followRedirects(TRUE);

    $this->assertEqual($original_url, $this->getSession()->getCurrentUrl(), 'Redirected back to the original location.');

… which IMHO is very easy to understand :)

If you agree with #29, Lendude, feel free to RTBC!

@Wim Leers yes this makes it sooooo much cleaner! I tried the double $this->performMetaRefresh(); and couldn't get it to work, great find.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1407,7 +1407,7 @@ protected function getTestMethodCaller() {
+    $refresh = $this->cssSelect('meta[http-equiv="Refresh"], meta[http-equiv="refresh"]');

With this fix, bumping this to a bug, test coverage is a little indirect, but does the trick I feel.

Updated the IS to include the fix.

/me high-fives Lendude 🖐

Great teamwork!

P.S.: just realized this isn't a bug, but a task :)

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1407,7 +1407,7 @@ protected function getTestMethodCaller() {
-    $refresh = $this->cssSelect('meta[http-equiv="Refresh"]');
+    $refresh = $this->cssSelect('meta[http-equiv="Refresh"], meta[http-equiv="refresh"]');

Sorry to do this, but I think this is scope creep. We should open a separate issue for this, and postpone this one on it.

Our guidelines are pretty clear in this regard https://www.drupal.org/core/scope

Also, the title of the issue was the red-flag for scope creep

Convert WebTestBaseTests of BigPipe to BrowserTestBase and make the redirect following in BrowserTestBase work for lower case refresh

:-)

Ugh … really? :(

Clearly it was case-insensitive in WebTestBase. We're converting one more test from WebTestBase to BrowserTestBase. In doing so, we found something in BrowserTestBase that was not forgiving enough, so we're fixing it.

Doesn't that make perfect sense? Do we really want explicit test coverage for this in BrowserTestBaseTest?

If you still think that's what we want, I'll do it, but I personally think it's overkill.

As far as I can tell: yeah, really :)

Yes please create a new issue, and ping me with it, we'll get it in quick

Here is the split issue #2905818: Make sure \Drupal\Tests\BrowserTestBase::checkForMetaRefresh is case insensitive

This is the patch without the change to BrowserTestBase, should be green once #2905818: Make sure \Drupal\Tests\BrowserTestBase::checkForMetaRefresh is case insensitive lands.

Postponing this on #2905818: Make sure \Drupal\Tests\BrowserTestBase::checkForMetaRefresh is case insensitive. #39 should fail until that's committed.

Blocker is in - thanks

Retested

@larowlan is operating at lightning speed! 🏎

And green again, so back to RTBC :)

+++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipePlaceholderTestCases.php
@@ -2,10 +2,10 @@
- * Contains \Drupal\Tests\big_pipe\Unit\Render\Placeholder\BigPipePlaceholderTestCases.
+ * Contains \Drupal\big_pipe_test\BigPipePlaceholderTestCases.

I think we can just remove these Contains now? Not there for a particular reason?

Can be fixed on commit.

#45: Correct — that class is just being moved, so we didn't do that to minimize changes. But you're right, it can be removed per new coding standards.

Is indeed safe to fix on commit :)

Committed as 8b52ba0 and pushed to 8.5.x.
Cherry-picked as 2e02c13 and pushed to 8.4.x.

Thanks!

Fixing version given backport