Convert web tests to browser tests for page_cache module

Initial conversion

For me it would be totally fine to inline these 3 lines of code needed for \Drupal\simpletest\WebTestBase::setHttpResponseDebugCacheabilityHeaders
It is used twice in the entirety of core.

Inlined cacheability disabling

Thank you @andypost
I strongly hope this passes it now :) Maybe still open up an issue to add such a method to something, it can't hurt.

@dawehner do you mean to extend AssertLegacyTrait or separate trait cos usage after patch is

core8$ git grep setHttpResponseDebugCacheabilityHeaders
core/modules/simpletest/src/WebTestBase.php:2184:  protected function setHttpResponseDebugCacheabilityHeaders($value = TRUE) {
core/modules/system/src/Tests/Routing/RouterTest.php:96:    $this->setHttpResponseDebugCacheabilityHeaders(FALSE);

Here's the patch which added the drupalHead function to the BTB base class to get it's passed. However, the tests are failing on local due to the headers response code in testConditionalRequests. The request is expecting not modified(304) but the drupalGet function in BTB is returning 200 which I'm not sure why it's happening

//Naveen

PageCacheTest module requires the couple of assertions which needs to be added before converting them.

BTB drupalGetHeader function is calling getResponseHeader of Mink Session class which internally changing the string to lowercase which making couple of tests fail.

    public function getResponseHeader($name)
    {
        $headers = $this->driver->getResponseHeaders();

        $name = strtolower($name);
        $headers = array_change_key_case($headers, CASE_LOWER);

        if (!isset($headers[$name])) {
            return null;
        }

        return is_array($headers[$name]) ? $headers[$name][0] : $headers[$name];
    }

How about overriding this method in BTB? Let's continue this discussion in follow-up issue and let keep this issue for minimal disruption.

//Naveen

@naveenvalecha hmmm not sure if it's worth splitting this off, since that just leaves 1 test in this patch. I would opt for trying to get PageCacheTest to pass here. If it really needs new methods to be added, lets make an issue for that, then we just postpone on that issue.

replace curl with guzzle + fix docblock

btw it makes sense to split this out of the test to separate unit

This fixes one of the failures.

I couldn't figure out testNoUrlNormalization yet.

Here is a patch. There are some things which I want to highlight:

1. All deprected things were replaced.
2. A drupalHead was removed in favour of drupalGet.
3. There were 3 cases in testNoUrlNormalization. The second one is:
   

     [
       $url . '?',
       $url . '',
     ],
   

   I removed it because guzzle client (which is default for BrowserTestBase) uses parse_url for building the URL and in this case
   parse_url('http://drupal.org/?') will be exactly the same as parse_url('http://drupal.org/'). Hope, it's clear.

Hopefully, this fixes one of the failures.

👏

1. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php
   @@ -79,7 +77,7 @@ public function testPageCacheTags() {
   -    $this->assertPageCacheContextsAndTags($node_1->urlInfo(), $cache_contexts, [
   +    $this->assertPageCacheContextsAndTags($node_1->toUrl(), $cache_contexts, [
   
   @@ -120,7 +118,7 @@ public function testPageCacheTags() {
   -    $this->assertPageCacheContextsAndTags($node_2->urlInfo(), $cache_contexts, [
   +    $this->assertPageCacheContextsAndTags($node_2->toUrl(), $cache_contexts, [
   

   These changes are out of scope.

2. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -15,7 +16,9 @@
   +  use AssertPageCacheContextsAndTagsTrait;
   
   @@ -45,19 +48,20 @@ protected function setUp() {
   -    $config = $this->config('system.performance');
   -    $config->set('cache.page.max_age', 300);
   -    $config->save();
   +    $this->enablePageCaching();
   

   Also out of scope.

3. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -45,19 +48,20 @@ protected function setUp() {
   -    $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS');
   +    $this->assertEquals('MISS', $this->drupalGetHeader('X-Drupal-Cache'));
   

   Same.

4. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -45,19 +48,20 @@ protected function setUp() {
   -    $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT');
   -    $cid_parts = [\Drupal::url('system_test.cache_tags_page', [], ['absolute' => TRUE]), 'html'];
   +    $this->assertEquals('HIT', $this->drupalGetHeader('X-Drupal-Cache'));
   +    $cid_parts = [
   +      Url::fromRoute('system_test.cache_tags_page')->setAbsolute()->toString(),
   +      'html',
   +    ];
   

   Again.

5. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -68,28 +72,34 @@ public function testPageCacheTags() {
   -    $this->assertIdentical($cache_entry->tags, $expected_tags);
   +    $this->assertSame($expected_tags, $cache_entry->tags);
   ...
   -    $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS');
   +    $this->assertEquals('MISS', $this->drupalGetHeader('X-Drupal-Cache'));
   

   Same and same.

Basically, the patch seems to be doing lots of great improvements, but they're out of scope here. The point of these "convert to BrowserTestBase issues" is to keep the set of changes as small as possible: the bare minimum to switch from WebTestBase to BrowserTestBase.

I see that in #21, the patch was 10.67 KB. And in #24, it was 39.42 KB. So it seems that @ApacheEx added all of these. Which is great! But those changes should be made in another issue, after this issue lands. We call that a "followup issue".

@ApacheEx, thanks for your great work :) But could you split off the non-essential changes to a separate patch that you post in a new followup issue? Once you've done that, I'll be able to RTBC this issue (and then once this is committed, I'll be able to RTBC the followup issue — because all of your changes in #24 look wonderful! 👌).

Thank you for your great feedback. Yeah, I was a bit carried away :)
Here is a patch with minimal changes (WebTestBase => BrowserTestBase) to pass all tests.
p.s. I'll create a follow-up issue and put #24 there.
p.s.s I've also attached two interdiffs (from #21 and #24) to see differences.

Changes looks great except nitpick

+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
@@ -553,10 +571,6 @@ public function testNoUrlNormalization() {
-      [
-        $url . '?',
-        $url . '',
-      ],

why that removed?

@andypost,
I can explain:

- in WebTestBase we have curl as default client.
- in BrowserTestBase we have guzzle as default client.

Let's imagine that:

$url = 'http://drupal.org/';

So, curl makes a request to that URL as is without any transformations.
The requests will look like:
- http://drupal.org/? - MISS
- http://drupal.org/? - HIT
- http://drupal.org/ - MISS

it means all tests passed.

But guzzle uses \GuzzleHttp\Psr7\Uri which composes a URI string from its various components.
The requests will look like:
- http://drupal.org/? - MISS (will be transformed to http://drupal.org/ by \GuzzleHttp\Psr7\Uri::__toString)
- http://drupal.org/? - HIT (will be transformed to http://drupal.org/ by \GuzzleHttp\Psr7\Uri::__toString)
- http://drupal.org/ - HIT (Failed, the expected result is MISS).

it means, one of the tests failed

I've tried to solve this, but it looks like impossible with Guzzle.

Hope, it's clear now.

1. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -15,7 +16,9 @@
   +  use AssertPageCacheContextsAndTagsTrait;
   

   This can be removed too, it also belongs in the followup. :)

2. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -553,10 +571,6 @@ public function testNoUrlNormalization() {
   -      [
   -        $url . '?',
   -        $url . '',
   -      ],
   

   I agree with andypost about this. This is removing test coverage. It was added in #2761639: PageCache should not use $request->getUri(). We should be careful when removing test coverage.

   Are you sure that Guzzle is getting in the way for \Drupal\page_cache\Tests\PageCacheTest::testNoUrlNormalization()? That test method is not explicitly using Guzzle, it's not constructing a URL explicitly.

1.

+class PageCacheTest extends BrowserTestBase {
+
+  use AssertPageCacheContextsAndTagsTrait;

It's used, here is a proof after re-testing:

2) Drupal\Tests\page_cache\Functional\PageCacheTest::testPageCacheAnonymousRolePermissions
Error: Call to undefined method Drupal\Tests\page_cache\Functional\PageCacheTest::assertCacheContext()

2. Guys, I agree with you that removing of test coverage is not a good idea. I'll try to figure out if this can be solved in another way.

1. Oh, sorry to have missed that!
2. ❤️ I know this is frustrating, thanks for your understanding!

ok, there is my investigation:

How BrowserWebBase::drupalGet does a request for testNoUrlNormalization

Sequence of calls:

1. Drupal\Tests\page_cache\Functional\PageCacheTest::testNoUrlNormalization
2. Drupal\Tests\BrowserTestBase::drupalGet
3. Behat\Mink\Session::visit
4. Behat\Mink\Driver\BrowserKitDriver::visit
5. Symfony\Component\BrowserKit\Client::request
6. Goutte\Client::doRequest
7. GuzzleHttp\Client::request
8. GuzzleHttp\Client::requestAsync
9. GuzzleHttp\Psr7\Request::__construct

- here is how GuzzleHttp\Psr7\Request handles an URL ($uri is http://d8.docksal/?):

if (!($uri instanceof UriInterface)) {
  $uri = new Uri($uri);
}
$this->uri = $uri; // Now, it looks like http://d8.docksal/ without "?"

- here is how GuzzleHttp\Psr7\Uri constructs an URL ($uri is http://d8.docksal/?):

if ($uri != '') {
  $parts = parse_url($uri);
  if ($parts === false) {
    throw new \InvalidArgumentException("Unable to parse URI: $uri");
  }
  $this->applyParts($parts);
}

- here is how GuzzleHttp\Psr7\Uri outputs an URL (and now we have http://d8.docksal/ instead of http://d8.docksal/?):

public function __toString() {
  return self::composeComponents(
    $this->scheme,
    $this->getAuthority(),
    $this->path,
    $this->query,
    $this->fragment
  );
}

- Here is how parse_url works with this test case (see #29):

>>> parse_url('http://d8.docksal/?');
=> [
     "scheme" => "http",
     "host" => "d8.docksal",
     "path" => "/",
   ]

- Here is how tests work without test case (see #29). BTW, I've modified (a bit) tests to show how some things work.
code: https://gist.github.com/ApacheEx/65ad1d016062a106bad6778e01dbaaed
logs:

docker@cli:/var/www/docroot/core$ sudo -u www-data -E ../vendor/phpunit/phpunit/phpunit --filter=testNoUrlNormalization --verbose
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Runtime:        PHP 7.0.23-1~dotdeb+8.1
Configuration:  /var/www/docroot/core/phpunit.xml

Testing 
.

Time: 13.6 seconds, Memory: 186.00MB

OK (1 test, 11 assertions)

- Here is how test case works (see #29). BTW, I've modified (a bit) tests to show how some things work.
code: https://gist.github.com/ApacheEx/b140c4fec21e2af8327adebad1d16c79
logs:

docker@cli:/var/www/docroot/core$ sudo -u www-data -E ../vendor/phpunit/phpunit/phpunit --filter=testNoUrlNormalization --verbose
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Runtime:        PHP 7.0.23-1~dotdeb+8.1
Configuration:  /var/www/docroot/core/phpunit.xml

Testing 
F

Time: 13.67 seconds, Memory: 186.00MB

There was 1 failure:

1) Drupal\Tests\page_cache\Functional\PageCacheTest::testNoUrlNormalization
Failed #1
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://d8.docksal/?'
+'http://d8.docksal/'

/var/www/docroot/core/tests/Drupal/Tests/BrowserTestBase.php:1240
/var/www/docroot/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:57
/var/www/docroot/core/modules/page_cache/tests/src/Functional/PageCacheTest.php:593

FAILURES!
Tests: 1, Assertions: 6, Failures: 1.

You can notice that URL before drupalGet and after drupalGet was modified from http://d8.docksal/? to http://d8.docksal/

That's why we get HIT instead of MISS in this assert:

// Check if the normalized URL is not cached.
$this->drupalGet($url_normalized);
$this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', "Cache is missing for {$url_normalized} URL.");

Why all test cases from testNoUrlNormalization have passed (WebTestBase)

Heh, drupalGet uses curlExec which builds an URL in Drupal way with WebTestBase::buildUrl
for example http://d8.docksal/? will be the same as WebTestBase::buildUrl('http://d8.docksal/?'), what I can't say about GuzzleHttp\Psr7\Uri ;(

How to fix it?

To be honest, I have no idea, all requests should be done by Client which implements GuzzleHttp\ClientInterface. It's not a solution to rewrite something there. What should we do then? Any ideas? Does it mean we should remove #29 from test coverage and use a patch from #28?

Only one correction:

8. GuzzleHttp\Client::requestAsync
- 9. GuzzleHttp\Psr7\Request::__construct
+ 9. $uri = $this->buildUri($uri, $options);
Because we lost /? after this operation.

yeah, that's true. But anyway, after this operation it goes to GuzzleHttp\Psr7\Request::__construct where again we have new Uri()

and here is a patch with some minimal corrections of your changes if you don't mind:
- I have added the test case which was removed by me in #28 :)
- Some CS fixes.

thank you for your feedback.

+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
@@ -564,18 +582,60 @@ public function testNoUrlNormalization() {
+   * Helper to get headers from pure request without any url modifications.

This is lacking an explanation for why this helper method exists.

Once that's done, this is RTBC. Great work, @vaplas and especially @ApacheEx! 👏

Thanks,
what about this?

  /**
   * Retrieves only the headers for an absolute path.
   *
   * This is a simplified re-implementation of a drupalHead() that
   * executes a cURL request without any URL modifications.
   *
   * @param string $url
   *   The URL to load into internal browser.
   *
   * @return array
   *   Array of headers.
   *
   * @see \Drupal\simpletest\WebTestBase::drupalHead()
   */
  protected function getHeaders($url) {

that executes a cURL request without any URL modifications.

is still not entirely clear.

What about:

executes a cURL request without any URL modifications (Guzzle always normalizes URLs and therefore doesn't allow to test all possible edge cases).

Yeah, I agree.
Here is updated patch (hopefully the final version).

1. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -564,18 +582,67 @@ public function testNoUrlNormalization() {
   +  /**
   +   * Retrieves only the headers for an absolute path.
   +   *
   +   * This is a simplified re-implementation of a drupalHead() that
   +   * executes a cURL request without any URL modifications (Guzzle always
   +   * normalizes URLs and therefore doesn't allow to test all possible edge
   +   * cases).
   +   *
   +   * @param string $url
   +   *   The URL to load into internal browser.
   +   *
   +   * @return array
   +   *   Array of headers.
   +   *
   +   * @see \Drupal\simpletest\WebTestBase::drupalHead()
   +   */
   +  protected function getHeaders($url) {
   +    $ch = curl_init();
   +    curl_setopt($ch, CURLOPT_URL, $url);
   +    curl_setopt($ch, CURLOPT_HEADER, TRUE);
   +    curl_setopt($ch, CURLOPT_NOBODY, TRUE);
   +    curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
   +    curl_setopt($ch, CURLOPT_USERAGENT, drupal_generate_test_ua($this->databasePrefix));
   +    $output = curl_exec($ch);
   +    curl_close($ch);
   +
   +    $headers = [];
   +    foreach (explode("\n", $output) as $header) {
   +      if (strpos($header, ':')) {
   +        list($key, $value) = explode(':', $header, 2);
   +        $headers[trim($key)] = trim($value);
   +      }
   +    }
   +
   +    return $headers;
   +  }
   

   I don't think we should mention drupalHead() here because eventually that code will disappear and this comment will be obsolete. The bit in brackets is the important thing - that we can't use Guzzle because it is going to normalise the request. (So it probably shouldn't be in brackets). I.e. maybe:

   Executes a cURL request without any modifications to the given URL. Guzzle always normalizes URLs which prevents testing all possible edge cases.
   

   Also, I'm wondering if we should think about adding a general trait as drupalHead() is used in DownloadTest and contrib's redirect module tests. Not sure about that as the code added here is not general. Probably not worth it.

2. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -564,18 +582,67 @@ public function testNoUrlNormalization() {
   +      $this->assertResponseHeader('X-Drupal-Cache', 'MISS', $url_raw);
   ...
   +  protected function assertResponseHeader($header, $expected, $url, $message = '') {
   

   $expected comes first and I think I would actually bother with this. I'd just do (for example)

   $headers = $this->getHeaders($url_raw);
   $this->assertEquals('MISS', $headers['X-Drupal-Cache'];
   

   in the loop.

   There's less indirection and less to think about.

thank you for your review :)
1. I don't think that we need the drupalHead. It can be replaced by drupalGet in most cases I guess or even $client->request('HEAD'). Btw, I'm a bit modified text if you don't mind.
2. Fixed.

+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
@@ -564,18 +582,52 @@ public function testNoUrlNormalization() {
+   *
+   * Executes a cURL request without any modifications to the given URL.
+   * Note that the Guzzle always normalizes URLs which prevents testing all
...
+   */
+  protected function getHeaders($url) {
+    $ch = curl_init();

I'm not convinced that testing foo/? vs foo/ is actually worth testing, maybe its just me, but at least for me this is something a functional test coverage should not care about. If you care about that level of testing, better write a integration/kernel test.

#47: That test was explicitly added in #2761639: PageCache should not use $request->getUri(), at @Berdir's request in #2761639-29: PageCache should not use $request->getUri(). I think it kinda makes sense to have in a functional test.

About #47, I think it's worth discussing if this is the right way to test this (because my initial reaction is that I agree that this is the wrong type of test for this), and I think core tests should serve as examples of how to test certain scenarios, but I'd opt to do so in a follow up.

The test exists currently, this issue is about converting existing test coverage with minimal changes. The changes here look as minimal as possible given all the previous findings. So I'd go with just leaving it as is for now.

On the patch:
Great work on this, and great research! Bit of nitpicking:

1. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -564,18 +582,52 @@ public function testNoUrlNormalization() {
   +   * Note that the Guzzle always normalizes URLs which prevents testing all
   

   'the Guzzle'? Just 'Guzzle' would do here.

2. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -564,18 +582,52 @@ public function testNoUrlNormalization() {
   +   *   The URL to load into internal browser.
   

   We are not using the internal browser, we are using cURL, that's the whole point of this method right?

More importantly: can we get some of the research from #34/#35 into the IS? It's a little minimal at the moment.

many thanks for reviewing, here is the patch.
Will try to update IS later or somebody else will do it.

Thank you @Lendude for providing an answer. I agree now, as long we clearly document why we are doing things a certain way.

+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
@@ -577,18 +595,52 @@ public function testNoUrlNormalization() {
+   *
+   * Executes a cURL request without any modifications to the given URL.
+   * Note that Guzzle always normalizes URLs which prevents testing all
+   * possible edge cases.
+   *

It is great that we document why we need this.

Giving credit to @Wim Leers, @Lendude and myself for reviews that influenced the direction of the patch and detailed why the approach taken is correct.

I've removed the "needs followup" tag because it was added #27 because the changes that were out-of-scope were all about removing deprecated usages or usages of the legacy layer in browser test base. These tasks should not be limited to fixing just one test but should be automated and done on a per method level.

Committed and pushed 40698c6c8b to 8.5.x and dc14cf8150 to 8.4.x. Thanks!

Backported to 8.4.x as this is a test-only change.