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.

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

[HttpFoundation] Constraint ResponseHeaderSame now shows the actual header value #49227

Open
wants to merge 2 commits into
base: 7.1
Choose a base branch
from

Conversation

jschaedl
Copy link
Contributor

@jschaedl jschaedl commented Feb 3, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? small adjustment
Deprecations? no
Tickets -
License MIT
Doc PR -

I am using the BrowserKitAssertionsTrait::assertResponseHeaderSame(...) quite a lot in my functional tests and it would be nice to see the actual header value in case the expected one doesn't match. So instead of just seeing:

Failed asserting that the Response has header "Cache-Control" with value "public".

it would be helpful to see:

Failed asserting that the Response has header "Cache-Control" with value "public", value of header "Cache-Control" is "no-cache, private".

@carsonbot carsonbot added this to the 6.3 milestone Feb 3, 2023
@jschaedl jschaedl force-pushed the more-talkative-response-header-same-constraint branch from 621b10e to 2cab626 Compare February 3, 2023 16:31
@javiereguiluz
Copy link
Member

I like this! A quick question: what happens if the header doesn't exist? Should we expect a message error like this?

Failed asserting that the Response has header "Cache-Control" with value "public",
the header "Cache-Control" was not included in the response.

@jschaedl
Copy link
Contributor Author

jschaedl commented Feb 3, 2023

Hi @javiereguiluz,

I guess most people check for header existence before they assert the header value, but I think it makes sense to add the null case, yes.

@jschaedl jschaedl force-pushed the more-talkative-response-header-same-constraint branch from 2cab626 to 29ae98d Compare February 6, 2023 11:36
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@jschaedl jschaedl force-pushed the more-talkative-response-header-same-constraint branch from 29ae98d to 39bfb97 Compare October 11, 2023 19:57
@jschaedl jschaedl force-pushed the more-talkative-response-header-same-constraint branch from 39bfb97 to b421a7f Compare November 12, 2023 09:27
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@jschaedl jschaedl force-pushed the more-talkative-response-header-same-constraint branch from b421a7f to 90e11a4 Compare November 26, 2023 18:38

public function __construct(string $headerName, string $expectedValue)
public function __construct(string $headerName, string $expectedValue, bool $logicalNot = false)
Copy link
Contributor Author

@jschaedl jschaedl Nov 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Let me try to explain:

The test case for the assertResponseHeaderNotSame

public function testAssertResponseHeaderNotSame()
{
    $this->getResponseTester(new Response())->assertResponseHeaderNotSame('Cache-Control', 'public');
    $this->expectException(AssertionFailedError::class);
    $this->expectExceptionMessage('Failed asserting that the Response does not have header "Cache-Control" with value "no-cache, private", value of header "Cache-Control" is "no-cache, private".');
    $this->getResponseTester(new Response())->assertResponseHeaderNotSame('Cache-Control', 'no-cache, private');
}

failed with:

Failed asserting that exception message

'Failed asserting that the Response does not have header "Cache-Control" with value "no-cache, private", value of header "Cache-Control" is not "no-cache, private".'

contains

'Failed asserting that the Response does not have header "Cache-Control" with value "no-cache, private", value of header "Cache-Control" is "no-cache, private".'.

  • Please notice the not in the actual exception message.

This is happening because the assertResponseHeaderNotSame assertion uses a ResponseHeaderSame constraint internally. This constraint is wrapped by a LogicalNot which is in fact the reason all messages for this constraint are negated.

So the best idea I had so far is to "ignore" the logical not case.

@jschaedl jschaedl force-pushed the more-talkative-response-header-same-constraint branch from 90e11a4 to 8854686 Compare November 26, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants