Replace assertions involving calls to drupalGetHeader() with session-based assertions, where possible

Perhaps we can even deprecate drupalGetHeader() after this?

Conversions here all look good. I agree with pushing the rest to a followup to keep this one simple.

+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
@@ -246,44 +246,44 @@ public function testPageCache() {
+    $this->assertNull($this->getSession()->getResponseHeader('X-Drupal-Cache'));
+    $this->assertSession()->responseHeaderNotContains('Vary', 'cookie');
+    $this->assertSession()->responseHeaderEquals('Cache-Control', 'must-revalidate, no-cache, private');
+    $this->assertSession()->responseHeaderEquals('Expires', 'Sun, 19 Nov 1978 05:00:00 GMT');
+    $this->assertSession()->responseHeaderEquals('Foo', 'bar');

For lines like this, is it necessary to introduce a variable for $this->assertSession()? It gets called many times.

Continue with #11, if we are going to doing so, I think it should be done in a separate issue. As it does not just happen here.

+++ b/core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php
@@ -46,7 +45,7 @@ public function testBasicAuth() {
-    $this->assertNull($this->drupalGetHeader('X-Drupal-Cache'));
+    $this->assertNull($this->getSession()->getResponseHeader('X-Drupal-Cache'));

Looked at the code behind this and I think we might be able to use this instead of assertNull?

$this->assertSession()->responseHeaderEquals('X-Drupal-Cache', NULL);

NW for #14, apologies for not doing this before - set back to RTBC if you disagree!

And responseHeaderEquals() checks that return value using === so $actual NULL will only check a NULL response and not pass for e.g. the empty string.

        $actual = $this->session->getResponseHeader($name);
        $message = sprintf('Current response header "%s" is "%s", but "%s" expected.', $name, $actual, $value);

        $this->assert($value === $actual, $message);

Wouldn't getResponseHeader() also return NULL if the header value was NULL instead of missing? Can a header value even be NULL, isn't it always a string or not set?

Reviewing.

1. +++ b/core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php
   @@ -73,9 +72,9 @@ public function testBasicAuth() {
   -    $this->assertNull($this->drupalGetHeader('X-Drupal-Cache'));
   +    $this->assertNull($this->getSession()->getResponseHeader('X-Drupal-Cache'));
   

   Why are we retaining assertNull() here as the assertion? ... I see #14 - #18 are about this.

2. +++ b/core/modules/config/tests/src/Functional/ConfigExportUITest.php
   @@ -58,9 +58,7 @@ public function testExport() {
   -    $header_match = (boolean) preg_match('/attachment; filename="config-' . preg_quote($hostname) . '-\d{4}-\d{2}-\d{2}-\d{2}-\d{2}\.tar\.gz"/', $header_content_disposition);
   ...
   +    $this->assertSession()->responseHeaderMatches('content-disposition', '/attachment; filename="config-' . preg_quote($hostname) . '-\d{4}-\d{2}-\d{2}-\d{2}-\d{2}\.tar\.gz"/');
   

   Nice one.

3. +++ b/core/modules/dynamic_page_cache/tests/src/Functional/DynamicPageCacheIntegrationTest.php
   @@ -56,23 +56,23 @@ public function testDynamicPageCache() {
   -    $this->assertNull($this->drupalGetHeader(DynamicPageCacheSubscriber::HEADER), 'Response object returned: Dynamic Page Cache is ignoring.');
   +    $this->assertNull($this->getSession()->getResponseHeader(DynamicPageCacheSubscriber::HEADER));
   
   @@ -93,39 +93,39 @@ public function testDynamicPageCache() {
   -    $this->assertNull($this->drupalGetHeader(DynamicPageCacheSubscriber::HEADER), 'Response returned, rendered as HTML response, admin route: Dynamic Page Cache is ignoring');
   ...
   -    $this->assertNull($this->drupalGetHeader(DynamicPageCacheSubscriber::HEADER), 'Response returned, plain response, admin route: Dynamic Page Cache is ignoring');
   ...
   -    $this->assertNull($this->drupalGetHeader(DynamicPageCacheSubscriber::HEADER), 'Response returned, cacheable response, admin route: Dynamic Page Cache is ignoring');
   ...
   -    $this->assertEqual('UNCACHEABLE', $this->drupalGetHeader(DynamicPageCacheSubscriber::HEADER), 'Render array returned, rendered as HTML response, but uncacheable: Dynamic Page Cache is running, but not caching.');
   ...
   -    $this->assertEqual('UNCACHEABLE', $this->drupalGetHeader(DynamicPageCacheSubscriber::HEADER), 'Render array returned, rendered as HTML response, but uncacheable: Dynamic Page Cache is running, but not caching.');
   ...
   -    $this->assertEqual('MISS', $this->drupalGetHeader(DynamicPageCacheSubscriber::HEADER), 'By default, Drupal has no auto-placeholdering cache tags.');
   

   This assertion (and the rest in this test) is adding debugging information about the type of the response (Response object, cacheable response object, render array as HTML, render array as AJAX, dialog, etc.). Since there's many similar assertions in the test, it is making debugging a little harder to go back to the exact line number, but it's manageable. In most cases in the test, the inline comments already include the needed information. However, there are a few where the information isn't included directly in the inline comments. I've highlighted those here. In most cases, these are giving additional information about the fixture being tested at that point.

4. +++ b/core/modules/file/tests/src/Functional/DownloadTest.php
   @@ -75,8 +75,8 @@ protected function doPrivateFileTransferTest() {
   -    $this->assertEqual($this->drupalGetHeader('x-foo'), 'Bar', 'Found header set by file_test module on private download.');
   -    $this->assertNull($this->drupalGetHeader('x-drupal-cache'), 'Page cache is disabled on private file download.');
   

   Let's put this in the inline comments as well.

I got about halfway through the patch. In terms of scope management, I'm wondering if it might make sense to separate assertions that already don't have assertion messages from those that do?

I think we should file a followup for #14 - #18 perhaps.

Nice! Patch #30 looks good to me.
Set to RTBC.

May be we will need to re-roll patch from issue #3164589: Replace assertions that use $this->getSession()->getResponseHeader() with WebAssert methods after this issue is committed or the opposite. Both patches fixes the class BasicAuthTest.php. Let's see what will be committed first.

Cheers, Paulo.

Rerolled.

Oops, missed a change from responseHeaderEquals(..., NULL) to responseHeaderDoesNotExist()

+++ b/core/modules/dynamic_page_cache/tests/src/Functional/DynamicPageCacheIntegrationTest.php
@@ -56,23 +56,32 @@ public function testDynamicPageCache() {
     $url = Url::fromUri('route:dynamic_page_cache_test.html');
     $this->drupalGet($url);
-    $this->assertEqual('MISS', $this->drupalGetHeader(DynamicPageCacheSubscriber::HEADER), 'Render array returned, rendered as HTML response: Dynamic Page Cache is active, Dynamic Page Cache MISS.');
+    // Verify that 'render array is returned, rendered as HTML response:
+    // Dynamic Page Cache is active, Dynamic Page Cache MISS.

@@ -82,10 +91,14 @@ public function testDynamicPageCache() {
       $this->drupalGet($url);

Not sure if there is a script issue, but the opening single quote from the assertion message is being reproduced in the code comment (first time I read it I was looking for the closing single quote).

This happens several times in the patch.

I'm also not sure the comment is correct as written - we're not asserting anything about a render array, only the cache headers. So I think we could remove these comments since the assertion is fairly self-documenting.

Changes are done mentioned by #36.

@mondrake I was also thinking the same but got confused so only removed the single quote comments mentioned in #36. Added a new patch and removed the line space error as well.

Thanks for adding. I forgot to add the interdiff.

Interdiff between patch #37 and #40.
Also RTBC+1

Cheers, Paulo.

#40 looks great, but needs a re-roll for 9.1.x

@mondrake I've already created the patch and worked on so many file. Put efforts and time on that but now its not worth.

     // GET a URL that was cached by Page Cache before, it should not be now.
-    $this->drupalGet('/respond-cacheable-response');
-    $this->assertNull($this->drupalGetHeader('X-Drupal-Cache'), 'Drupal page cache header not found.');
+    $this->drupalGet('/respond-cacheable-reponse');
+    $this->assertSession()->responseHeaderDoesNotExist('X-Drupal-Cache');
   }
 

The patch introduced a typo here, but still passed tests because the 403 response is also not cached. Out of scope to improve the test here, but we should open a follow-up to add an additional assertion to make sure the page we get back is valid.

Committed 3e0f015 and pushed to 9.1.x. Thanks!