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

[5.8] Fix assertJsonMissingValidationErrors() on empty response #28595

Merged

Conversation

shadoWalker89
Copy link
Contributor

I have an endpoint where i update a resource and return a http 204 (no content) if the update was successful.

Calling ->assertJsonMissingValidationErrors('some_field') to check that there is no errors should pass the test, but i get this error.
Invalid JSON was returned from the route.

That is because the TestResponse::json() method will throw this error when trying to decode a response that is not a valid json.

@shadoWalker89 shadoWalker89 force-pushed the no_json_errors_on_empty_response branch from b9e45c9 to 313a2a0 Compare May 23, 2019 13:31
@taylorotwell
Copy link
Member

I guess I don't understand. If you already asserted that the response code is a 204 it feels redundant and unnecessary to even check this?

@shadoWalker89
Copy link
Contributor Author

@taylorotwell No i don't check the status code.
This is one of my tests, i added comments in the code to explain.

    /** @test */
    public function validates_name_is_unique()
    {
        $cluster = factory(Cluster::class)->create();
        $otherCluster = factory(Cluster::class)->create(['name' => "Existing name"]);
        
        $this->signIn();

        // So here i check that cannot update using another cluster name
        $this->putJson(route('clusters.update', $cluster), $this->validPayload([
            'name' => $otherCluster->name,
        ]))
        ->assertJsonValidationErrors('name');

        // Here i check i can update the cluster without changing the name
        // means the validation unique rule is ignoring the name of the model being update
        $this->putJson(route('clusters.update', $cluster), $this->validPayload([
            'name' => $cluster->name,
        ]))
        // ------------------------------> This why i made the PR <---------------------------------
        // Before the PR i get the error : Invalid JSON was returned from the route.
        ->assertJsonMissingValidationErrors('name');
    }

Here i'm not interested in the status code, but in the absence of the error on the name field

@SjorsO
Copy link
Contributor

SjorsO commented May 27, 2019

@taylorotwell Could you reconsider this PR? I've run in to this problem before too.

Some api endpoints don't have to return anything, just a 200 status code so the front-end knows the call was successful. If the controller includes validation then you can never use the assertJsonMissingValidationErrors assertion, because that assertion always fails when the response doesn't contain valid json (since an empty response is not valid json).

The reason for adding the assertJsonMissingValidationErrors is so you get useful output when the test fails, instead of just a "200 does not equal 422" message.

If you do plan to merge this PR, i would remove the $this->getStatusCode() == 204 check, since returning nothing is automatically a 200 response. If you're already adding code to return a 204, you might as well return valid json too.

@driesvints
Copy link
Member

@shadoWalker89 you're performing multiple HTTP requests in a single test method which isn't supported.

@shadoWalker89
Copy link
Contributor Author

shadoWalker89 commented May 27, 2019

@driesvints Why do you think it's not supported ?
It actually is supported, and i do it all the time.
This is not a problem related to multiple http requests at the same time.
The problem is that the assertJsonMissingValidationErrors() method uses json() which will throw the Invalid JSON was returned from the route error if the response is not a valid json content.
And that is why i made this PR.

Btw, assertSessionDoesntHaveErrors('field') will work if the session has no errors at all.
I think the same should work with assertJsonMissingValidationErrors('field') when the response is empty.

@SjorsO
Copy link
Contributor

SjorsO commented May 27, 2019

@shadoWalker89 You can usually make multiple calls in one test, but it can lead to some weird problems. Take a look at this thread: #27060

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants