Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FrameworkBundle][HttpFoundation] reduce response constraints verbosity #49184

Merged
merged 1 commit into from Mar 17, 2024
Merged

[FrameworkBundle][HttpFoundation] reduce response constraints verbosity #49184

merged 1 commit into from Mar 17, 2024

Conversation

Nicals
Copy link
Contributor

@Nicals Nicals commented Feb 1, 2023 •
edited

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.

Before 
$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

<!--  (404 Not Found) -->
<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="UTF-8" />
        <meta name="robots" content="noindex,nofollow" />
    <!-- undred of lines later -->
        Sfjs.createToggles();
        Sfjs.createFilters();
    });
}
/*]]>*/        </script>
    </body>
</html>
<!--  (404 Not Found) -->

/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
After 
$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

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.

@carsonbot carsonbot added this to the 6.3 milestone Feb 1, 2023
@Nicals Nicals mentioned this pull request Feb 1, 2023
Open
@Nicals Nicals force-pushed the 42948_reduce_response_constraints_verbosity branch from 78772a9 to ab0eea3 Compare February 2, 2023 07:46
@carsonbot carsonbot changed the title 42948 reduce response constraints verbosity [FrameworkBundle][HttpFoundation] 42948 reduce response constraints verbosity Feb 2, 2023
@Nicals Nicals force-pushed the 42948_reduce_response_constraints_verbosity branch from ab0eea3 to 54c1bbb Compare February 3, 2023 10:12
@carsonbot
Copy link

Hey!

I think @alli83 has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@GromNaN
Copy link
Member

GromNaN commented Mar 16, 2023 •
edited

Hi Nicolas! Thank you for submitting this pull request and for all the hard work you put into it. I appreciate your effort and dedication to improving this project.

However, after reviewing your changes, I'm not entirely sure if your solution is necessary or if using $client->catchExceptions(false); might already achieve the same result.

@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle][HttpFoundation] 42948 reduce response constraints verbosity [FrameworkBundle][HttpFoundation] reduce response constraints verbosity Mar 17, 2023
@Nicals
Copy link
Contributor Author

Nicals commented Apr 17, 2023

Catching exception effectively reduces the verbosity.

This PR intends to reduces the verbosity of test logs in all cases.

For example, a failed assertion about response status code will dump the entire response, including body.
When debugging such assertion I want to see the actual response status code, but this information is in the response header, hundreds of lines above the tail of the unit test logs.
Exception catching is not the issue in this case.

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@fabpot
Copy link
Member

fabpot commented Feb 3, 2024 •
edited

@Nicals Are you still interested in finishing this PR?

@Nicals Nicals force-pushed the 42948_reduce_response_constraints_verbosity branch from c9f5ae0 to 7d0265c Compare February 4, 2024 11:53
@Nicals
Copy link
Contributor Author

Nicals commented Feb 4, 2024

@fabpot I totally forgot about this MR. I don't work with php/symfony anymore so I did not keep track of it.

I quickly rebased this branch on up to date 7.1 and built the response headers manually.

@fabpot fabpot force-pushed the 42948_reduce_response_constraints_verbosity branch from 7d0265c to cabb2dc Compare March 17, 2024 16:31
@fabpot
Copy link
Member

fabpot commented Mar 17, 2024

Thank you @Nicals.

@fabpot fabpot merged commit e491616 into symfony:7.1 Mar 17, 2024
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature FrameworkBundle HttpFoundation Status: Reviewed
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

[HttpFoundation] Make test constraints less verbose
6 participants
@Nicals @carsonbot @GromNaN @fabpot @nicolas-grekas @xabbuh