Misleading .property assertion behaviour with value #452

Closed
onefifth opened this Issue May 29, 2015 · 3 comments

Projects

None yet

2 participants

@onefifth

This test passes despite key not having a value of undefined.

var someObject = {key: 'value'};
expect(someObject).to.have.property('key', undefined);

I realize that the 'correct' way to test this would be to use the .undefined assertion, but there are a couple times where this has caught me off guard.
Say you've done something like .to.have.property('key', setupObject.myProprty), but you've misspelt .myProperty.
Now you have a really weird test that's passing without doing the value comparison it looks like it should be doing because you've mistakenly passed undefined.

"Offending" code: https://github.com/chaijs/chai/blob/master/lib/chai/core/assertions.js#L850

It would be nice to potentially use some internal CHAI_UNDEFINED const as the default value for val in these cases to internally differentiate between an unused optional parameter and one explicitly set to undefined. I have only encountered this using this particular assertion, but this behaviour quite possibly exists elsewhere there are optional arguments.

@onefifth

For clarification, this const could be set up by checking the length property of the arguments object to determine if the value undefined was passed in or not, assigning default values to unspecified args as needed.
The introduction of this const is just a suggestion and directly testing if (arguments.length > 1) { would suffice as a substitute for the if (undefined !== val) { line linked above.

@onefifth

The opened PR addresses this issue by checking the length of the arguments array. I attempted to add a few additional tests to cover these previously untested cases where undefined could be passed as an argument but my tests are failing with a fatal : Cannot read property 'sha' of undefined message.

If I could get help sorting that out I can update the PR with these additional tests.

Additional tests: onefifth@91f57e9
Failing TravisCI: https://travis-ci.org/onefifth/chai/builds/64526847

@onefifth

Apparently it's a weird environment thing. Pull request tests passed successfully.
If the additional tests look reasonable, consider #454 over #453.

@keithamus keithamus closed this May 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment