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

[BUG] assertJson() does not use strict checking #21292

Closed
sleenen opened this issue Sep 20, 2017 · 3 comments
Closed

[BUG] assertJson() does not use strict checking #21292

sleenen opened this issue Sep 20, 2017 · 3 comments

Comments

@sleenen
Copy link

sleenen commented Sep 20, 2017

  • Laravel Version: 5.5
  • PHP Version: 7.0
  • Database Driver & Version: n/a

Description:

assertJson() does not use strict mode, but I think it should. In my test (which tests API json response), I have a a piece of json that I expect to find like this:

$response->assertJson(['some_attribute' => 0]);

The actual response has a string value set for some_attribute. However the test succeeds, while imo it should fail, because the value was not found in the json response. The reason it does succeed is because it doesn't use strict checking. The evaluation of 0 == "some_string" is true. Strict checking would make this false.

Steps To Reproduce:

Created a simple test like this in the framework:

public function testAssertJsonZeroValue() {

    $response = TestResponse::fromBaseResponse(new Response(new JsonSerializableStrictResourcesStub));

    $this->expectException(ExpectationFailedException::class);

    $response->assertJson(['foo' => 0]);

}

class JsonSerializableStrictResourcesStub implements JsonSerializable
{
    public function jsonSerialize()
    {
        return [
            'foo' => 'bar'
        ];
    }
}

Possible solutions

  1. Simply changing the strict parameter to true. I feel strict checking is appropriate. I've asked in Larachat for people to come up with why/when you really want/need non-strict checking. Haven't heard a satisfiable answer there, perhaps somewhere here can give me a scenario.
    I've changed it to true, my test then passes as do all other tests.

  2. Making strict an optional parameter for the assertJson() method, defaulting to true. This might then break tests for people.

  3. Doing the same as option 2, only make the param default to false. This will make it not break things for people when they update.

  4. Introduce a new method assertStrictJson(). This is not the nicest solution imo, but I've put it here, so other people can contribute. Maybe they feel this is the best choice and can come up with good arguments then.

@Dylan-DPC-zz
Copy link

Thanks. Made a PR following step 3

@sleenen
Copy link
Author

sleenen commented Sep 21, 2017

@taylorotwell I don't understand why this does not raise any discussion.
Since if I want to see 0 in my response, I do not want to see a string. Most people will not be aware of this behaviour and then have tests that succeed while they shouldn't..

This PR itself lacks tests and the documentation should be updated if this stays in like this.

@sleenen
Copy link
Author

sleenen commented Sep 21, 2017

I'll look into something for 5.6

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

No branches or pull requests

2 participants