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

[6.x] Add assertNoContent to TestResponse #30125

Merged
merged 2 commits into from Sep 30, 2019

Conversation

@jasonmccreary
Copy link
Contributor

commented Sep 27, 2019

This assertion is similar to the recently added assertForbidden and assertUnauthorized methods. However, its compound assertion provides an easier way to test for "no content" responses while also specifying an optional status code.

Before

$response = $this->get('api/endpoint');

$response->assertStatus(409);
$this->assertEmpty($response->content());

After

$response = $this->get('api/endpoint');

$response->assertNoContent(409);
@jasonmccreary jasonmccreary force-pushed the jasonmccreary:assert-no-content branch 2 times, most recently from 6fad51c to 448157d Sep 27, 2019
@driesvints driesvints changed the title Add assertNoContent to TestResponse [6.x] Add assertNoContent to TestResponse Sep 27, 2019
@jasonmccreary jasonmccreary force-pushed the jasonmccreary:assert-no-content branch from 448157d to 43ee394 Sep 27, 2019
@dwightwatson

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

This feels a little odd to me in the sense that all the other assert{$httpStatus} helper methods literally only assert the status - making this one an outlier in that it can take an argument, and that it has an additional expectation.

Would it be better to also introduce assertEmpty so could be used separately (and also be a bit more explicit)?

$response->assertStatus(409)->assertEmpty();

$response->assertNoContent()->assertEmpty();
@jasonmccreary

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2019

@dwightwatson this is the exact same behavior of response()->noContent() which would be used in the implementation. It too takes an optional argument of the status code.

It's important to remember a "no content" response by default is not just a 204 status code, but also an a response with "no body". As such, the compound expression is what makes this assertion improve the developer experience.

With that said, a $response->assertEmpty() might be helpful depending on if this is merged or not.

@driesvints driesvints dismissed their stale review Sep 30, 2019

Changes made.

@taylorotwell taylorotwell merged commit 567d374 into laravel:6.x Sep 30, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/styleci/pr The analysis has passed
Details
@jasonmccreary jasonmccreary deleted the jasonmccreary:assert-no-content branch Sep 30, 2019
@ahinkle ahinkle referenced this pull request Oct 1, 2019
i-bajrai added a commit to i-bajrai/framework that referenced this pull request Oct 4, 2019
* Add assertNoContent to TestResponse

* Update TestResponse.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.