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

breaking change in 2.0 (null, undefined, arguments, ok, true, false, empty, exist) #371

Closed
JamesMessinger opened this issue Feb 18, 2015 · 10 comments

Comments

@JamesMessinger
Copy link
Member

Before the 2.0 release of Chai, these assertions could be written as properties or as functions. For example:

expect(foo).to.be.true;
expect(foo).to.be.true();

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

expect(foo).to.exist;
expect(foo).to.exist();

expect(foo).to.be.null;
expect(foo).to.be.null();

The documentation on chaijs.com even explained that this dual-syntax was there to play nice with some IDEs and linting tools that flag the non-function syntax as an error. But in 2.0, the function syntax no longer exists. Was it removed by accident, or was there a reason for it?

@adrianblynch
Copy link

I see the same. Our test suite was updated to use function call syntax and under 2.0.0 they all fail.

Include undefined() and false() in that list.

@adrianblynch
Copy link

Look at the bottom of the 2.0.0 change log.

Revert "Allows writing lint-friendly tests"

Was this intentional?

@keithamus
Copy link
Member

Thanks for the issue @BigstickCarpet.

This was fully intentional. The new method assertion style (.to.be.true()) broke the old property assertion style (.to.be.true), meaning it was a case of using one or the other. We had to pick one single interface to stick to, so we went back to the old property assertion style (.to.be.true).

Because developers have been using the new style, and dropping the new style would break their code, we pushed the change as a Version 2.0.0. According to SemVer (which I'd like us to start following for future releases), major versions mean breaking changes - which is to say that the contract of "you wont have to refactor your test code" is invalidated.

If you'd like to keep the method assertion syntax (.to.be.true()), feel free to keep using Chai 1.10.0. Alternatively, if you feel the need to upgrade to 2.0.0 then you have a few options:

  • Refactor your tests to the new (actually old) property assertions; .to.be.true, to.be.empty, etc.
  • Refactor your code so as not to avoid these assertions (.to.be.true becomes .to.equal(true)) and avoid the problem altogether
  • Use the dirty-chai plugin, which changes all end property assertions to be method assertions (so .to.be.true becomes .to.be.true()). Of course, this may break plugins or tests that use the old property assertions.

Ultimately, if we made the wrong decision in settling on one style, and the majority of the Chai community wants to solely use the method assertions (.to.be.true()) then we can flip-flop on this decision. But given we've flip-flopped once already it will have to take a lot of convincing, and would basically have to be a unanimous decision from the community. Personally I'm in favour of method assertions (.to.be.true()) but my opinion nugatory in the face of the community - who get the deciding vote.

@keithamus
Copy link
Member

For more context on all of this:

@feugy
Copy link

feugy commented Feb 19, 2015

Hi, thank you for this explanation.
It's been a long time we're using chai, and we started with the old syntax. We relaxed JSHint's configuration for test files.

But as we are not english natives, it turns out that we mispelled some expectation (especially exist()):

expect(err).not.to.exists;

In the old style, exists isn't defined, and the expectation is always true, whatever the value or err.

Use the new style exists() has a significant advantage: the test ends with something like exists is undefined which allow the developper to discover that his assertion was broken.

Is it possible with [email protected] that this edge case does not happen anymore ?

@keithamus
Copy link
Member

Thanks @feugy for your use case, this is definitely one advantage of using method assertions.

@feugy I suggest for now you use dirty-chai

@KrisSiegel
Copy link

May I suggest placing some sort of notice or link at the top of the documentation saying "Upgrading to 2.0? Look here to see the breaking changes" or something similar. I was confused why things were not working until I found this thread and then the changelog.

For what it's worth, at least in my projects, this was an easy global find and place so it wasn't so bad.

@lo1tuma
Copy link
Contributor

lo1tuma commented Feb 23, 2015

@keithamus Couldn't a lib like funstance make those properties callable in a backwards compatible way?

@keithamus
Copy link
Member

@lo1tuma thanks for the suggestion, but we already went down this road with the initial PRs. I'd be incredibly weary of trying to support both styles as it was pretty effectively proven to be buggy. The only real solution is to go all in with one style, which we've done.

@KrisSiegel sounds like a great idea 😄. Love to see a PR for better documentation across the project!

@keithamus
Copy link
Member

I think the dust has settled on this now. I'm going to close this one, as I'm pretty sure we're sticking with the property style of assertions. dirty-chai exists and is a perfectly suitable plugin for this - and is compatible with all other plugins.

solsson added a commit to solsson/chai that referenced this issue May 5, 2015
…otentially harmful in particular when troubleshooting through tests and adding .to.be.defined because you see a .to.be.undefined somehwere. There is .to.exist but after 2.0.0 chaijs#371 assertions are more prone to false positives.
lacolaco pushed a commit to lacolaco/kuromoji.js that referenced this issue Jul 31, 2015
And fix some test for breaking changes of chai.
 [ chaijs/chai#371 ]
tignear pushed a commit to guild-utils/bot that referenced this issue Jul 23, 2020
And fix some test for breaking changes of chai.
 [ chaijs/chai#371 ]
tignear pushed a commit to guild-utils/bot that referenced this issue Jul 23, 2020
And fix some test for breaking changes of chai.
 [ chaijs/chai#371 ]
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

6 participants