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

Matcher properties in expect mode are dangerous #788

Closed
ralfstx opened this issue Sep 8, 2016 · 3 comments
Closed

Matcher properties in expect mode are dangerous #788

ralfstx opened this issue Sep 8, 2016 · 3 comments

Comments

@ralfstx
Copy link

ralfstx commented Sep 8, 2016

I have some concerns about the use of properties in the matcher API that I would like to bring it up here. It's not meant as criticism, I just don't understand why it has been done this way and would like to understand your point of view.

After switching several projects from Jasmine to Mocha/Chai, we've discovered a number of tests that falsely succeeded. For example, it appeared obvious to transform Jasmine's toBeDefined() to

expect(some.thing).to.be.defined;

which always succeed, even though some.thing does not exist at all. This behavior is obvious, given that defined is not part of Chai's API. However, not getting an error when calling non-existent API by mistake is a pitfall, especially in a test framework, isn't it?

At another place, using sinon-chai, someone mixed up the API and wrote

expect(some.method).to.have.been.called.once;

instead of the correct

expect(some.method).to.have.been.calledOnce;

which also doesn't do any meaningful but all the green tests lulled us into a false sense of security.

Typos like to.be.fasle would go unnoticed as well.

Granted, if you follow the red-green-refactor mantra, this should not happen, however, tests evolve, they are copied, and those things actually do happen. If Chai used functions instead of properties (as Jasmine does), we would have run into errors:

expect(some.thing).to.be.null();

would succeed only if the method null exists, otherwise the test would run into the infamous "undefined is not a function".

Another problem may be API evolution. If at some point, one of those fields is not supported anymore, nobody will notice, as all tests would continue to succeed (as false positives).

I'm sure there were good reasons for this kind of API. Do you see this problem too? What's your take on this? Would you consider it an option to add functions like null(), true(), etc.?

@meeber
Copy link
Contributor

meeber commented Sep 8, 2016

Gonna close this as a duplicate of #726. The ES6 proxy solution mentioned in that thread is already live on master branch and will be released in Chai 4.0. Other measures mentioned in that thread are on our road map.

@meeber meeber closed this as completed Sep 8, 2016
@lucasfcosta
Copy link
Member

Hi @ralfstx, thanks for the issue!
Just complementing what @meeber said, you will be able to see how it works and more details about how to use and configure it when #781 gets done!
Hopefully this will solve the problem you're currently having!

@ralfstx
Copy link
Author

ralfstx commented Sep 9, 2016

Thanks for the pointer. The proxy solution will certainly help. Still I don't see the advantage of using properties instead of functions (except compatibility).

I find it confusing that for example to.throw() is a function while to.be.true is not, and I'd prefer to use functions consistently. I hope that providing functions next to the properties as suggested in #726 (comment) is still on the road map.

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

3 participants