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

Scope:
Drupal\page_cache\Tests\PageCacheTagsIntegrationTest
Drupal\page_cache\Tests\PageCacheTest

Extra changes:
We introduce a helper function getHeaders() to request headers from various urls. We not use drupalGet() for this, because it works via Guzzle clients, which always normalizes URLs, eg:

That prevents testing all possible edge cases. See thorough review in #34.

CommentFileSizeAuthor
#51 interdiff-2870457-45-51.txt764 bytesApacheEx
#51 2870457-51.patch11.02 KBApacheEx
#45 interdiff-2870457-42-45.txt2.45 KBApacheEx
#45 2870457-45.patch11.05 KBApacheEx
#42 interdiff-2870457-36-42.txt975 bytesApacheEx
#42 2870457-42.patch11.57 KBApacheEx
#36 interdiff-2870457-35-36.patch1.16 KBApacheEx
#36 2870457-36.patch11.28 KBApacheEx
#35 interdiff-28-35.txt2.55 KBvaplas
#35 2870457-35.patch11.42 KBvaplas
#28 interdiff-2870457-25-28.txt36.06 KBApacheEx
#28 interdiff-2870457-21-28.txt7.35 KBApacheEx
#28 2870457-28.patch9.18 KBApacheEx
#26 interdiff-2870457-24-25.txt739 bytesApacheEx
#26 2870457-25.patch39.46 KBApacheEx
#24 2870457-24.patch39.42 KBApacheEx
#21 interdiff-2870457.txt3.1 KBdawehner
#21 2870457-21.patch10.67 KBdawehner
#19 interdiff-2870457.txt2.94 KBdawehner
#19 2870457-19.patch9.88 KBdawehner
#17 2870457-page_cache_tests-17.patch7.25 KBandypost
#17 interdiff.txt3.69 KBandypost
#15 interdiff-2870457-10-15.txt718 bytesnlisgo
#15 convert_web_tests_to-2870457-15.patch4.81 KBnlisgo
#13 interdiff-2870457-10-12.txt4.03 KBnaveenvalecha
#13 2870457-12.patch1.29 KBnaveenvalecha
#11 interdiff-2870457-7-10.txt1.8 KBnaveenvalecha
#11 2870457-10.patch4.72 KBnaveenvalecha
#7 2870457-7.patch2.92 KBandypost
#7 interdiff.txt861 bytesandypost
#3 2870457-3.patch2.39 KBandypost
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

michielnugter created an issue. See original summary.

andypost’s picture

Status: Active » Needs review
FileSize
2.39 KB

Initial conversion

Status: Needs review » Needs work

The last submitted patch, 3: 2870457-3.patch, failed testing.

dawehner’s picture

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.

michielnugter’s picture

Component: phpunit » page_cache.module
Issue tags: +phpunit initiative
andypost’s picture

Status: Needs work » Needs review
FileSize
861 bytes
2.92 KB

Inlined cacheability disabling

dawehner’s picture

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.

andypost’s picture

@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);

Status: Needs review » Needs work

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

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
1.8 KB

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

Status: Needs review » Needs work

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

naveenvalecha’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.29 KB
4.03 KB

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

Lendude’s picture

Status: Needs review » Needs work

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

nlisgo’s picture

Status: Needs work » Needs review
FileSize
4.81 KB
718 bytes

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
3.69 KB
7.25 KB

replace curl with guzzle + fix docblock

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

Status: Needs review » Needs work

The last submitted patch, 17: 2870457-page_cache_tests-17.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.88 KB
2.94 KB

This fixes one of the failures.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.67 KB
3.1 KB

I couldn't figure out testNoUrlNormalization yet.

Status: Needs review » Needs work

The last submitted patch, 21: 2870457-21.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.

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
39.42 KB

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.

Status: Needs review » Needs work

The last submitted patch, 24: 2870457-24.patch, failed testing. View results

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
39.46 KB
739 bytes

Hopefully, this fixes one of the failures.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup

👏

  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! 👌).

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
9.18 KB
7.35 KB
36.06 KB

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.

andypost’s picture

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?

ApacheEx’s picture

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

Wim Leers’s picture

  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.

ApacheEx’s picture

Status: Needs review » Needs work

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.

Wim Leers’s picture

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

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?

vaplas’s picture

Status: Needs work » Needs review
FileSize
11.42 KB
2.55 KB

Nice investigation @ApacheEx! It very useful for re-check what was happening here. Thanks!

Only one correction:

 8. GuzzleHttp\Client::requestAsync
- 9. GuzzleHttp\Psr7\Request::__construct
+ 9. $uri = $this->buildUri($uri, $options);

Because we lost /? after this operation.


all requests should be done by Client which implements GuzzleHttp\ClientInterface

This is not true. The main goal of this test is to check the caching system with different url combinations. Therefore, if the current Client is not work, we are need to look for another one. I think this is exactly what the reviewers alluded to ;)

Yes, it will not be minimal changes (one of main rule for convert to BTB), but this rule ceases to be important when regression occurs.

So, let's just use the curl request.

ApacheEx’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 36: interdiff-2870457-35-36.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Needs review

@ApacheEx++, indeed, thanks!

Wim Leers’s picture

+++ 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! 👏

ApacheEx’s picture

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) {
Wim Leers’s picture

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).
ApacheEx’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
11.05 KB
2.45 KB

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.

vaplas’s picture

Nice catches and fixes from #38 to #45! +1 to RTBC!

dawehner’s picture

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

Wim Leers’s picture

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

vaplas’s picture

Yep, @Berdir wrote:

That's the example that's problematic for redirect.module even when ignoring the query parameters.

By analogy, we can say that this example also shows the problem of phpunit, since the URL processing in Guzzle and Drupal is not equivalent.


By the way, phpunit has one more helper, which sometimes has a disservice - Behat/Mink. It is used in drupalPostForm, and as result we cann't send form with next set:
  • hidden fields
  • fields with invalid data
  • nonexistent fields

As a result, we have not simple tools for test various exploits that a user can do with a session and form tokens. See also #5.1 and #5.2 from #2809503: [PP-2] Convert AJAX part of \Drupal\file\Tests\FileFieldWidgetTest to JavascriptTestBase.

Lendude’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

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.

ApacheEx’s picture

FileSize
11.02 KB
764 bytes

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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.

vaplas’s picture

Issue summary: View changes

Short note about extra changes was added to IS.

alexpott’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 40698c6 on 8.5.x
    Issue #2870457 by ApacheEx, andypost, dawehner, naveenvalecha, vaplas,...

  • alexpott committed dc14cf8 on 8.4.x
    Issue #2870457 by ApacheEx, andypost, dawehner, naveenvalecha, vaplas,...

Status: Fixed » Closed (fixed)

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