feature #49184 [FrameworkBundle][HttpFoundation] reduce response cons…

…traints verbosity (Nicolas Appriou, Nicals) This PR was squashed before being merged into the 7.1 branch. Discussion ---------- [FrameworkBundle][HttpFoundation] reduce response constraints verbosity | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #42948 | License | MIT | Doc PR | symfony/symfony-docs#17848 Adds a `verbose` argument to HttpFoundation response test constraints. If set to false, only the HTTP response command and headers will be displayed in case of test failure. This argument is `true` by default to keep backward compatibility, but I think it would make more sense to be `false`. <details> <summary>Before</summary> ```php $this->assertResponseIsSuccessful(); ``` ``` -- PHPUnit output: Testing App\Controller\AppControllerTest F 1 / 1 (100%) Time: 00:00.299, Memory: 42.50 MB There was 1 failure: 1) App\Controller\AppControllerTest::testSuccessfulResponse Failed asserting that the Response is successful. HTTP/1.1 404 Not Found Cache-Control: max-age=0, must-revalidate, private Content-Type: text/html; charset=UTF-8 Date: Fri, 03 Feb 2023 10:54:05 GMT Expires: Fri, 03 Feb 2023 10:54:05 GMT X-Debug-Exception: X-Debug-Exception-File: ... X-Robots-Tag: noindex  <!DOCTYPE html> <html lang="en"> <head> <meta charset="UTF-8" /> <meta name="robots" content="noindex,nofollow" />  Sfjs.createToggles(); Sfjs.createFilters(); }); } /*]]>*/ </script> </body> </html>  /home/user/project/vendor/symfony/framework-bundle/Test/BrowserKitAssertionsTrait.php:142 /home/user/project/vendor/symfony/framework-bundle/Test/BrowserKitAssertionsTrait.php:33 /home/user/project/tests/Controller/AppControllerTest.php:35 ``` </details> <details> <summary>After</summary> ```php $this->assertResponseIsSuccessful(verbose: false); ``` ``` -- PHPUnit output: Testing App\Controller\AppControllerTest F 1 / 1 (100%) Time: 00:00.300, Memory: 42.50 MB There was 1 failure: 1) App\Controller\AppControllerTest::testSuccessfulResponse Failed asserting that the Response is successful. HTTP/1.1 404 Not Found Cache-Control: max-age=0, must-revalidate, private Content-Type: text/html; charset=UTF-8 Date: Fri, 03 Feb 2023 10:53:03 GMT Expires: Fri, 03 Feb 2023 10:53:03 GMT X-Debug-Exception: X-Debug-Exception-File: ... X-Robots-Tag: noindex /home/user/project/vendor/symfony/framework-bundle/Test/BrowserKitAssertionsTrait.php:142 /home/user/project/vendor/symfony/framework-bundle/Test/BrowserKitAssertionsTrait.php:33 /home/user/project/tests/Controller/AppControllerTest.php:35 ``` </details> I added a method `Symfony\Component\HttpFoundation\Response::getCommandString()` that builds the first line of an HTTP response to avoid code duplication in constraints classes. I thought of other solutions: + Build the HTTP command line in each `Constraint`. This seems that too much code duplication. + Add an `AbstractResponseConstraint` with a utility method to build a less verbose response string. I wasn't sure we needed to add an extend layer here. + Add a `ResponseConstraintTrait` with an exposed method to build the less verbose response string. This did not *feel* right. Overall, I think the `Response` class is the more appropriate to build this string. I also had some issue with psalm complaining about the documented $response argument type of `additionalFailureDescription`: ``` ERROR: MoreSpecificImplementedParamType - src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseStatusCodeSame.php:50:53 - Argument 1 of Symfony\Component\HttpFoundation\Test\Constraint\ResponseStatusCodeSame::additionalFailureDescription has the more specific type 'Symfony\Component\HttpFoundation\Response', expecting 'mixed' as defined by PHPUnit\Framework\Constraint\Constraint::additionalFailureDescription (see https://psalm.dev/140) protected function additionalFailureDescription($other): string ``` I needed to delete the phpdoc comment to comply with this. I only changed the lines were Psalm was giving me errors, but there are still some mismatch between HttpFoundation constraints signatures and PHPUnit ones. Commits ------- cabb2dc [FrameworkBundle][HttpFoundation] reduce response constraints verbosity
symfony · Mar 17, 2024 · e491616 · e491616
2 parents c0ecee0 + cabb2dc
commit e491616
Show file tree

Hide file tree

Showing 10 changed files with 144 additions and 33 deletions.
diff --git a/src/Symfony/Bundle/FrameworkBundle/Test/BrowserKitAssertionsTrait.php b/src/Symfony/Bundle/FrameworkBundle/Test/BrowserKitAssertionsTrait.php
@@ -28,24 +28,24 @@
  */
 trait BrowserKitAssertionsTrait
 {
-    public static function assertResponseIsSuccessful(string $message = ''): void
+    public static function assertResponseIsSuccessful(string $message = '', bool $verbose = true): void
     {
-        self::assertThatForResponse(new ResponseConstraint\ResponseIsSuccessful(), $message);
+        self::assertThatForResponse(new ResponseConstraint\ResponseIsSuccessful($verbose), $message);
     }
 
-    public static function assertResponseStatusCodeSame(int $expectedCode, string $message = ''): void
+    public static function assertResponseStatusCodeSame(int $expectedCode, string $message = '', bool $verbose = true): void
     {
-        self::assertThatForResponse(new ResponseConstraint\ResponseStatusCodeSame($expectedCode), $message);
+        self::assertThatForResponse(new ResponseConstraint\ResponseStatusCodeSame($expectedCode, $verbose), $message);
     }
 
     public static function assertResponseFormatSame(?string $expectedFormat, string $message = ''): void
     {
         self::assertThatForResponse(new ResponseConstraint\ResponseFormatSame(self::getRequest(), $expectedFormat), $message);
     }
 
-    public static function assertResponseRedirects(?string $expectedLocation = null, ?int $expectedCode = null, string $message = ''): void
+    public static function assertResponseRedirects(?string $expectedLocation = null, ?int $expectedCode = null, string $message = '', bool $verbose = true): void
     {
-        $constraint = new ResponseConstraint\ResponseIsRedirected();
+        $constraint = new ResponseConstraint\ResponseIsRedirected($verbose);
         if ($expectedLocation) {
             if (class_exists(ResponseConstraint\ResponseHeaderLocationSame::class)) {
                 $locationConstraint = new ResponseConstraint\ResponseHeaderLocationSame(self::getRequest(), $expectedLocation);
@@ -100,9 +100,9 @@ public static function assertResponseCookieValueSame(string $name, string $expec
         ), $message);
     }
 
-    public static function assertResponseIsUnprocessable(string $message = ''): void
+    public static function assertResponseIsUnprocessable(string $message = '', bool $verbose = true): void
     {
-        self::assertThatForResponse(new ResponseConstraint\ResponseIsUnprocessable(), $message);
+        self::assertThatForResponse(new ResponseConstraint\ResponseIsUnprocessable($verbose), $message);
     }
 
     public static function assertBrowserHasCookie(string $name, string $path = '/', ?string $domain = null, string $message = ''): void

diff --git a/src/Symfony/Component/HttpFoundation/CHANGELOG.md b/src/Symfony/Component/HttpFoundation/CHANGELOG.md
@@ -8,6 +8,7 @@ CHANGELOG
  * Add `QueryParameterRequestMatcher`
  * Add `HeaderRequestMatcher`
  * Add support for `\SplTempFileObject` in `BinaryFileResponse`
+ * Add `verbose` argument to response test constraints
 
 7.0
 ---

diff --git a/src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseIsRedirected.php b/src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseIsRedirected.php
@@ -16,6 +16,13 @@
 
 final class ResponseIsRedirected extends Constraint
 {
+    /**
+     * @param bool $verbose If true, the entire response is printed on failure. If false, the response body is omitted.
+     */
+    public function __construct(private readonly bool $verbose = true)
+    {
+    }
+
     public function toString(): string
     {
         return 'is redirected';
@@ -37,11 +44,12 @@ protected function failureDescription($response): string
         return 'the Response '.$this->toString();
     }
 
-    /**
-     * @param Response $response
-     */
-    protected function additionalFailureDescription($response): string
+    protected function additionalFailureDescription($other): string
     {
-        return (string) $response;
+        if ($this->verbose || !($other instanceof Response)) {
+            return (string) $other;
+        } else {
+            return explode("\r\n\r\n", (string) $other)[0];
+        }
     }
 }
diff --git a/src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseIsSuccessful.php b/src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseIsSuccessful.php
@@ -16,6 +16,13 @@
 
 final class ResponseIsSuccessful extends Constraint
 {
+    /**
+     * @param bool $verbose If true, the entire response is printed on failure. If false, the response body is omitted.
+     */
+    public function __construct(private readonly bool $verbose = true)
+    {
+    }
+
     public function toString(): string
     {
         return 'is successful';
@@ -37,11 +44,12 @@ protected function failureDescription($response): string
         return 'the Response '.$this->toString();
     }
 
-    /**
-     * @param Response $response
-     */
-    protected function additionalFailureDescription($response): string
+    protected function additionalFailureDescription($other): string
     {
-        return (string) $response;
+        if ($this->verbose || !($other instanceof Response)) {
+            return (string) $other;
+        } else {
+            return explode("\r\n\r\n", (string) $other)[0];
+        }
     }
 }
diff --git a/src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseIsUnprocessable.php b/src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseIsUnprocessable.php
@@ -16,6 +16,13 @@
 
 final class ResponseIsUnprocessable extends Constraint
 {
+    /**
+     * @param bool $verbose If true, the entire response is printed on failure. If false, the response body is omitted.
+     */
+    public function __construct(private readonly bool $verbose = true)
+    {
+    }
+
     public function toString(): string
     {
         return 'is unprocessable';
@@ -37,11 +44,12 @@ protected function failureDescription($other): string
         return 'the Response '.$this->toString();
     }
 
-    /**
-     * @param Response $other
-     */
     protected function additionalFailureDescription($other): string
     {
-        return (string) $other;
+        if ($this->verbose || !($other instanceof Response)) {
+            return (string) $other;
+        } else {
+            return explode("\r\n\r\n", (string) $other)[0];
+        }
     }
 }
diff --git a/src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseStatusCodeSame.php b/src/Symfony/Component/HttpFoundation/Test/Constraint/ResponseStatusCodeSame.php
@@ -18,7 +18,7 @@ final class ResponseStatusCodeSame extends Constraint
 {
     private int $statusCode;
 
-    public function __construct(int $statusCode)
+    public function __construct(int $statusCode, private readonly bool $verbose = true)
     {
         $this->statusCode = $statusCode;
     }
@@ -44,11 +44,12 @@ protected function failureDescription($response): string
         return 'the Response '.$this->toString();
     }
 
-    /**
-     * @param Response $response
-     */
-    protected function additionalFailureDescription($response): string
+    protected function additionalFailureDescription($other): string
     {
-        return (string) $response;
+        if ($this->verbose || !($other instanceof Response)) {
+            return (string) $other;
+        } else {
+            return explode("\r\n\r\n", (string) $other)[0];
+        }
     }
 }
diff --git a/src/Symfony/Component/HttpFoundation/Tests/Test/Constraint/ResponseIsRedirectedTest.php b/src/Symfony/Component/HttpFoundation/Tests/Test/Constraint/ResponseIsRedirectedTest.php
@@ -27,9 +27,27 @@ public function testConstraint()
         $this->assertFalse($constraint->evaluate(new Response(), '', true));
 
         try {
-            $constraint->evaluate(new Response());
+            $constraint->evaluate(new Response('Body content'));
         } catch (ExpectationFailedException $e) {
-            $this->assertStringContainsString("Failed asserting that the Response is redirected.\nHTTP/1.0 200 OK", TestFailure::exceptionToString($e));
+            $exceptionMessage = TestFailure::exceptionToString($e);
+            $this->assertStringContainsString("Failed asserting that the Response is redirected.\nHTTP/1.0 200 OK", $exceptionMessage);
+            $this->assertStringContainsString('Body content', $exceptionMessage);
+
+            return;
+        }
+
+        $this->fail();
+    }
+
+    public function testReducedVerbosity()
+    {
+        $constraint = new ResponseIsRedirected(verbose: false);
+        try {
+            $constraint->evaluate(new Response('Body content'));
+        } catch (ExpectationFailedException $e) {
+            $exceptionMessage = TestFailure::exceptionToString($e);
+            $this->assertStringContainsString("Failed asserting that the Response is redirected.\nHTTP/1.0 200 OK", $exceptionMessage);
+            $this->assertStringNotContainsString('Body content', $exceptionMessage);
 
             return;
         }

diff --git a/src/Symfony/Component/HttpFoundation/Tests/Test/Constraint/ResponseIsSuccessfulTest.php b/src/Symfony/Component/HttpFoundation/Tests/Test/Constraint/ResponseIsSuccessfulTest.php
@@ -27,9 +27,28 @@ public function testConstraint()
         $this->assertFalse($constraint->evaluate(new Response('', 404), '', true));
 
         try {
-            $constraint->evaluate(new Response('', 404));
+            $constraint->evaluate(new Response('Response body', 404));
         } catch (ExpectationFailedException $e) {
-            $this->assertStringContainsString("Failed asserting that the Response is successful.\nHTTP/1.0 404 Not Found", TestFailure::exceptionToString($e));
+            $exceptionMessage = TestFailure::exceptionToString($e);
+            $this->assertStringContainsString("Failed asserting that the Response is successful.\nHTTP/1.0 404 Not Found", $exceptionMessage);
+            $this->assertStringContainsString('Response body', $exceptionMessage);
+
+            return;
+        }
+
+        $this->fail();
+    }
+
+    public function testReducedVerbosity()
+    {
+        $constraint = new ResponseIsSuccessful(verbose: false);
+
+        try {
+            $constraint->evaluate(new Response('Response body', 404));
+        } catch (ExpectationFailedException $e) {
+            $exceptionMessage = TestFailure::exceptionToString($e);
+            $this->assertStringContainsString("Failed asserting that the Response is successful.\nHTTP/1.0 404 Not Found", $exceptionMessage);
+            $this->assertStringNotContainsString('Response body', $exceptionMessage);
 
             return;
         }

diff --git a/src/Symfony/Component/HttpFoundation/Tests/Test/Constraint/ResponseIsUnprocessableTest.php b/src/Symfony/Component/HttpFoundation/Tests/Test/Constraint/ResponseIsUnprocessableTest.php
@@ -11,7 +11,9 @@
 
 namespace Symfony\Component\HttpFoundation\Tests\Test\Constraint;
 
+use PHPUnit\Framework\ExpectationFailedException;
 use PHPUnit\Framework\TestCase;
+use PHPUnit\Framework\TestFailure;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpFoundation\Test\Constraint\ResponseIsUnprocessable;
 
@@ -23,5 +25,34 @@ public function testConstraint()
 
         $this->assertTrue($constraint->evaluate(new Response('', 422), '', true));
         $this->assertFalse($constraint->evaluate(new Response(), '', true));
+
+        try {
+            $constraint->evaluate(new Response('Response body'));
+        } catch (ExpectationFailedException $e) {
+            $exceptionMessage = TestFailure::exceptionToString($e);
+            $this->assertStringContainsString("Failed asserting that the Response is unprocessable.\nHTTP/1.0 200 OK", $exceptionMessage);
+            $this->assertStringContainsString('Response body', $exceptionMessage);
+
+            return;
+        }
+
+        $this->fail();
+    }
+
+    public function testReducedVerbosity()
+    {
+        $constraint = new ResponseIsUnprocessable(verbose: false);
+
+        try {
+            $constraint->evaluate(new Response('Response body'));
+        } catch (ExpectationFailedException $e) {
+            $exceptionMessage = TestFailure::exceptionToString($e);
+            $this->assertStringContainsString("Failed asserting that the Response is unprocessable.\nHTTP/1.0 200 OK", $exceptionMessage);
+            $this->assertStringNotContainsString('Response body', $exceptionMessage);
+
+            return;
+        }
+
+        $this->fail();
     }
 }
diff --git a/src/Symfony/Component/HttpFoundation/Tests/Test/Constraint/ResponseStatusCodeSameTest.php b/src/Symfony/Component/HttpFoundation/Tests/Test/Constraint/ResponseStatusCodeSameTest.php
@@ -29,13 +29,30 @@ public function testConstraint()
 
         $constraint = new ResponseStatusCodeSame(200);
         try {
-            $constraint->evaluate(new Response('', 404));
+            $constraint->evaluate(new Response('Response body', 404));
         } catch (ExpectationFailedException $e) {
+            $exceptionMessage = TestFailure::exceptionToString($e);
             $this->assertStringContainsString("Failed asserting that the Response status code is 200.\nHTTP/1.0 404 Not Found", TestFailure::exceptionToString($e));
+            $this->assertStringContainsString('Response body', $exceptionMessage);
 
             return;
         }
 
         $this->fail();
     }
+
+    public function testReducedVerbosity()
+    {
+        $constraint = new ResponseStatusCodeSame(200, verbose: false);
+
+        try {
+            $constraint->evaluate(new Response('Response body', 404));
+        } catch (ExpectationFailedException $e) {
+            $exceptionMessage = TestFailure::exceptionToString($e);
+            $this->assertStringContainsString("Failed asserting that the Response status code is 200.\nHTTP/1.0 404 Not Found", TestFailure::exceptionToString($e));
+            $this->assertStringNotContainsString('Response body', $exceptionMessage);
+
+            return;
+        }
+    }
 }