-
-
Notifications
You must be signed in to change notification settings - Fork 696
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
should undefined be classed as empty in expect empty #472
Comments
Hi @Daveloper87 thanks for the issue! You're right on the latter - it should be As for |
Just to clarify - I'm marking this as pr wanted only for the |
Hi @keithamus , Thanks for responding. I have opened a pull request for that fix here: In regards to the .empty on undefined checks. I only actually noticed it because I was writing my tests first, and was slightly surprised that my empty function passed, but I very much agree with your point about checking the return type and it not being worth a breaking change, I just did not expect undefined to pass the empty check. One little thing I wanted to note as well just in case someone else comes across this issue.
only fails because the line: https://github.com/chaijs/chai/blob/master/lib/chai/core/assertions.js#L381
because typeof null is === 'object', it just fails with a TypeError trying to Object.keys on null, otherwise that would pass too. Obviously not a problem because it fails, but I am just adding it to this issue in case someone else highlights it. Thanks again |
The fact that |
@Daveloper87 I think given the messy There is a |
I did think after I sent that last message that its probably not good having it fail from an error.. |
👍 thanks @Daveloper87 |
@keithamus Happy to close this based on pull #475. Would you like me to reopen the fix for 'object' being correctly named to 'obj' #473 in master since 4.x.x will not be released just yet? |
Sure, if you're willing to put the work in for the PR I'll gladly accept and roll it out as a bugfix for chai 3.x 😄 |
Great! I will get that sorted tomorrow |
@Daveloper87 Did you ever get this working? If not, I'd be willing to take a look at it too! 😁 |
@bradcypert Ah sorry, I thought I had pushed it.. Its on my other computer so will have to get it pushed later |
@keithamus Sorry for the delay, I thought I had already reopened this pull. It is here now if you would like to review #499. If you are happy to merge I will close off this issue as well 😀 |
Merged #499, so this can be closed |
I have found a potential issue with the .empty assertion in expect. Running in a node.js context on Mac OSX
If you run :
This will happily work as a passing test assertion. Is this an intended behavior?
My only problem would be if you are creating a collection dynamically and it comes through as undefined, you may assume your code is working when it is just returning undefined.
Obviously you could add something like:
But I feel it should fail if it receives undefined to avoid assertions passing when no collection, object or string was passed.
Also as a side note, the string check here does not execute:
https://github.com/chaijs/chai/blob/master/lib/chai/core/assertions.js#L379
It still works because the code runs
which would return true for an empty string. It looks like it supposed to be obj instead of object
The 'string' check could be removed based on !expected, but it is probably nicer to be explicit anyway.
The text was updated successfully, but these errors were encountered: