should undefined be classed as empty in expect empty

[Daio-io]

I have found a potential issue with the .empty assertion in expect. Running in a node.js context on Mac OSX
If you run :

expect().to.be.empty
expect(undefined).to.be.empty;

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:

expect(someData).to.be.an('object');

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

!expected

which would return true for an empty string. It looks like it supposed to be obj instead of object

 if (Array.isArray(obj) || 'string' === typeof obj)

The 'string' check could be removed based on !expected, but it is probably nicer to be explicit anyway.

[keithamus]

Hi @Daveloper87 thanks for the issue!

You're right on the latter - it should be obj. I welcome a pr fixing this.

As for .empty, changing it would be a breaking change for something that is philosophically/arguably wrong, rather than objectively. Therefore I'm not as convinced. If you're concerned that your code may be returning undefined then add some more assertions to test this, as you mentioned. Personally I always assert return types for all my tests unless it's obvious (e.g through equality checks).

[keithamus]

Just to clarify - I'm marking this as pr wanted only for the obj fix 😄.

[Daio-io]

Hi @keithamus , Thanks for responding. I have opened a pull request for that fix here:
#473

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.

expect(null).to.be.empty;

only fails because the line: https://github.com/chaijs/chai/blob/master/lib/chai/core/assertions.js#L381

else if (typeof obj === 'object') {

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

[keithamus]

The fact that null throws badly is somewhat changing my mind about this situation...

[keithamus]

@Daveloper87 I think given the messy null situation this becomes a good candidate for making a breaking change.

There is a 4.x.x branch available on Chai - please feel free to make a PR against that branch to change the behaviour of .empty to throw on undefined and null. Of course this will need extra tests in place to explicitly test those behaviours (tests can be found in expect.jsL356-400).

[Daio-io]

I did think after I sent that last message that its probably not good having it fail from an error..
Ok great, I will get a PR together this week

[keithamus]

👍 thanks @Daveloper87

[Daio-io]

@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?

[keithamus]

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 😄

[Daio-io]

Great! I will get that sorted tomorrow

[bradcypert]

@Daveloper87 Did you ever get this working? If not, I'd be willing to take a look at it too! 😁

[Daio-io]

@bradcypert Ah sorry, I thought I had pushed it.. Its on my other computer so will have to get it pushed later

[Daio-io]

@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 😀

[keithamus]

Merged #499, so this can be closed