Introduce PHPUnit-compliant WebAssert::responseHeaderExists() method, and its negative

Looks great! two nitpicks:

1. +++ b/core/tests/Drupal/FunctionalTests/WebAssertTest.php
   @@ -0,0 +1,52 @@
   +   * Modules to enable.
   +   *
   +   * @var array
   

   Use {@inheritdoc} ?

2. +++ b/core/tests/Drupal/FunctionalTests/WebAssertTest.php
   @@ -0,0 +1,52 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $defaultTheme = 'classy';
   

   Replace with stark? see [#3083055], classy is not recommended to use anymore.

To me, the patch for 9.x is an RTBC and this should be eligible backporting to 8.x.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -55,6 +58,38 @@ protected function cleanUrl($url) {
+    $constraint = new ArrayHasKey($name);

Is it worth the effort of creating brand new Constraint classes? If this was a new ResponseHeaderExists class then we can customise the "Failed asserting that an array has the key" message. Unfortunately ArrayHasKey is final so we can't just extend it, but Constraint is quite straighforward to implement.

Trying #13, unsure if this is overkill though - comments welcome.

This is ineligible for backport to 8.x due to the void return type not being compatible with PHP 7.0.

Fixed a couple of minor errors in #14. Interdiff from #11.

Even better, besides 3 coding standards messages

core/tests/Drupal/Tests/Constraints/ResponseHeaderExists.php
- line 52 Expected 1 blank line after function; 0 found
- line 53 The closing brace for the class must have an empty line before it
core/tests/Drupal/Tests/WebAssert.php
- line 15 Unused use statement

This is ineligible for backport to 8.x due to the void return type not being compatible with PHP 7.0.

You are right.

Thank you @longwave!

Here I have added a patch which might address comment #16.

Thanks, @ravi.shankar! No more CS violations.

#17 is the one RTBCed, And this is ineligible to 8.x as @longwave said "due to the void return type not being compatible with PHP 7.0."

@mondrake, thanks for your review!

Should proper design pattern(s) be taken into account here for reusing existed code? Such as: builder, decorator, composite etc.

Just spotted that Symfony ships with some very similar constraints - e.g. https://github.com/symfony/http-foundation/blob/4.4/Test/Constraint/Resp... - but we can't use them directly because they expect a Symfony Response object which Mink doesn't use.

1. So in #3131186-16: Replace assertions involving calls to drupalGetHeader() with session-based assertions, where possible, part of the reason we're introducing these methods is that a response header may exist but have a null value.

   Based on that, I think that we should have test coverage that responseHeaderExists() passes if it exists and is set to NULL, and that responseHeaderNotExists() fails in the same circumstance.

2. +++ b/core/tests/Drupal/FunctionalTests/WebAssertTest.php
   @@ -0,0 +1,50 @@
   + * Tests WebAssert functionality.
   ...
   +class WebAssertTest extends BrowserTestBase {
   

   It tests quite a bit less than that. :) Let's give it a more specific name and document it?

3. +++ b/core/tests/Drupal/FunctionalTests/WebAssertTest.php
   @@ -0,0 +1,50 @@
   +  /**
   +   * @covers ::responseHeaderExists
   +   */
   +  public function testResponseHeaderExists() {
   ...
   +  /**
   +   * @covers ::responseHeaderDoesNotExist
   +   */
   +  public function testResponseHeaderDoesNotExist() {
   

   Technically, these should both still have one-line summaries.

4. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -55,6 +58,38 @@ protected function cleanUrl($url) {
   +   * @param string $name
   +   *   The name of the header entry to check existence of.
   +   */
   +  public function responseHeaderExists(string $name, string $message = ''): void {
   ...
   +   * @param string $name
   +   *   The name of the header entry to check existence of.
   +   */
   +  public function responseHeaderDoesNotExist(string $name, string $message = ''): void {
   

   Documentation of $message is missing.

5. Finally, can we include with the updated patch an updated test-only patch to expose the fails of the final version? Thanks!

#26.2 None of WebAssert has tests. I think this class should exist with the aim to add further WebAssert tests to it in the future.

#26.3 There is an exception in our PHPunit coding standards: "PHPUnit tests MAY skip the PHPDoc summary line if their PHPDoc specifies a @coversDefaultClass" Actually does this only apply to the *class* comment and not the method comment?

Addressed #26.1, #26.3, #26.4, Not sure what a test-only patch adds in this case, but uploading one anyway.

@longwave, I hear all the time that such-and-such class is intended to be extended with more tests for X, and then it isn't. :) If the test is to be expanded, we'll want a followup issue that specifies adding other coverage to the test in order for that to be likely to happen. Thanks!

Couple questions from reviewing this from an architectural standpoint rather than just "is this committable" tunnel vision:

1. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -55,6 +58,42 @@ protected function cleanUrl($url) {
   +   * @param string $message
   +   *   The failure message.
   

   Interesting, I did not notice before that this is a "Failure message" rather than the "Pass" message. Definitely upside-down from our historical tests, at least maybe the SimpleTest ones. Is that the convention for PHPUnit assertions and I just never realized?

2. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -55,6 +58,42 @@ protected function cleanUrl($url) {
   +    if ($message === '') {
   +      $message = "Failed asserting that the response does not have a '$name' header.";
   +    }
   

   Something I opened an issue for many, many years ago was to concatenate the user-supplied message (if any) with the default message, so that (a) people did not obscure real debugging with less specific or misleading messages but also (b) They could add more information without having to also ensure that the generic message details were included. That was back in pre-PHPUnit days, but it's still relevant to this patch.

   I'm not sure how much we're trying to closely follow specific upstream conventions here, but it's worth considering at least whether that's something we'd want to do.

We still need the followup to add tests for the rest of our methods in WebAssert - it would be great to add similar test coverage for them.

Also the questions in #31 have not been addressed

All the feedback has been addressed, so I think this is ready to go in.

Committed 6e7d0d9 and pushed to 9.1.x. Thanks!