See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Converting these tests revealed a bug in BrowserTestBase where certain redirects where not followed. The reason this is a problem: \Symfony\Component\HttpFoundation\RedirectResponse uses the lower-case variant of meta[http-equiv="refresh"]… which means any Symfony redirect response will not be followed correctly by the current check for meta[http-equiv="Refresh"]

See #2905818: Make sure \Drupal\Tests\BrowserTestBase::checkForMetaRefresh is case insensitive

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Jo Fitzgerald created an issue. See original summary.

Jo Fitzgerald’s picture

Component: dblog.module » big_pipe.module
Assigned: Jo Fitzgerald » Unassigned
Status: Active » Needs review
FileSize
6.02 KB

Kicked-off the conversion by:

  • Moving both of the files.
  • Changing the namespace.
  • Updating references.

(and corrected the component on the issue)

Status: Needs review » Needs work

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

dawehner’s picture

I think you can replace

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

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

Wim Leers’s picture

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

Wim Leers’s picture

Title: Convert web tests of big_pipe » Convert WebTestBaseTests of BigPipe to BrowserTestBase
michielnugter’s picture

Issue tags: +phpunit initiative
Lendude’s picture

Status: Needs work » Postponed

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

naveenvalecha’s picture

Status: Postponed » Needs review
FileSize
6.07 KB

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

Status: Needs review » Needs work

The last submitted patch, 9: 2863607-9.patch, failed testing. View results

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
8.71 KB

Here we go with another hit.

Status: Needs review » Needs work

The last submitted patch, 11: 2863607-11.patch, failed testing. View results

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
4.31 KB

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

//Naveen

Status: Needs review » Needs work

The last submitted patch, 13: 2863607-14.patch, failed testing. View results

naveenvalecha’s picture

naveenvalecha’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: 2863607-15.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
11.15 KB
11.56 KB

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.

Status: Needs review » Needs work

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

Lendude’s picture

Status: Needs work » Needs review
FileSize
734 bytes
11.18 KB

just this?

Lendude’s picture

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

Lendude’s picture

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

Wim Leers’s picture

Status: Needs review » Needs work

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.

Lendude’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
14.24 KB

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

Wim Leers’s picture

  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!
  1. 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 :)
  2. See prior point.
  1. Hm 🤔 this makes the test more complex and harder to understand than it was before. Trying to come up with a simpler alternative…
  2. See prior point.

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

Wim Leers’s picture

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

jibran’s picture

  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?

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
4.99 KB
15.45 KB

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 :)

Wim Leers’s picture

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

Lendude’s picture

Title: Convert WebTestBaseTests of BigPipe to BrowserTestBase » Convert WebTestBaseTests of BigPipe to BrowserTestBase and make the redirect following in BrowserTestBase work for lower case refresh
Category: Task » Bug report
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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

Wim Leers’s picture

Category: Bug report » Task

/me high-fives Lendude 🖐

Great teamwork!

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

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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

larowlan’s picture

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

:-)

Wim Leers’s picture

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.

yoroy’s picture

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

larowlan’s picture

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

Lendude’s picture

Title: Convert WebTestBaseTests of BigPipe to BrowserTestBase and make the redirect following in BrowserTestBase work for lower case refresh » Convert WebTestBaseTests of BigPipe to BrowserTestBase
Issue summary: View changes
Lendude’s picture

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

Wim Leers’s picture

Title: Convert WebTestBaseTests of BigPipe to BrowserTestBase » [PP-1] Convert WebTestBaseTests of BigPipe to BrowserTestBase
Status: Needs review » Postponed
Related issues: +#2905818: Make sure \Drupal\Tests\BrowserTestBase::checkForMetaRefresh is case insensitive
larowlan’s picture

Title: [PP-1] Convert WebTestBaseTests of BigPipe to BrowserTestBase » Convert WebTestBaseTests of BigPipe to BrowserTestBase
Status: Postponed » Needs review

Blocker is in - thanks

larowlan’s picture

Retested

Wim Leers’s picture

@larowlan is operating at lightning speed! 🏎

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

And green again, so back to RTBC :)

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#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 :)

  • larowlan committed 8b52ba0 on 8.5.x
    Issue #2863607 by Lendude, naveenvalecha, Wim Leers, Jo Fitzgerald:...

  • larowlan committed 2e02c13 on 8.4.x
    Issue #2863607 by Lendude, naveenvalecha, Wim Leers, Jo Fitzgerald:...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

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

Wim Leers’s picture

Thanks!

larowlan’s picture

Version: 8.5.x-dev » 8.4.x-dev

Fixing version given backport

Status: Fixed » Closed (fixed)

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