WIP: Overhaul .throw
#1220
WIP: Overhaul .throw
#1220
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1220 +/- ##
==========================================
+ Coverage 94.51% 94.63% +0.11%
==========================================
Files 32 32
Lines 1676 1676
Branches 404 394 -10
==========================================
+ Hits 1584 1586 +2
+ Misses 92 90 -2
Continue to review full report at Codecov.
|
lib/chai/core/assertions.js
Outdated
| * | ||
| * expect(badFn).to.throw; |
There was a problem hiding this comment.
This is an interesting addition - but it is also one we tried very much in vain to do back in Chai@2 days. The main issue here is that property assertions that assert and also return a function to assert messes a lot of stuff up. There were some terrible nasty bugs that came from this.
Aside from my concerns of implementation complexity, I also don't think this is actually internally consistent with the property-vs-assertion rule we have: throw takes arguments, and so having it as a property sometimes, and other times not, is something that does not have existing precedent.
Further to your point:
I'm opposed to jumping to the opposite extreme of removing all property-based assertions from Chai; despite these types of problems, I think property-based assertions are a critical part of Chai's identity and appeal for many users.
I'm of the opposite mind set. For one, we see a lot of issues around implicitly passing tests because people often mistake function-assertion for a property-assertion, and get many bug reports about this. For another it makes the learning cliff a lot harder, as we're probably the only library in the users tool-belt that has this behaviour. If it were a common idiom I'd be more behind it, but it is not. Another reason I'd like to see this pattern disappear is that it makes plugin development more complex, both from the API complexity but also from a philosophical perspective where not every plugin strictly adheres to the property-vs-method rubric, which of course in turn decreases usability because Chai's own rules aren't consistent.
Currently, with chai@5 there exists no way to handle property assertions, although I plan to add them in but via an optional interface, and that is forced based on something we can introspect in code (e.g. property assertions are made from any unary assertions - i.e one that only takes only an actual and not any additional arguments).
There was a problem hiding this comment.
My two main arguments against making property assertions "opt-in" in chai@5 are:
- The history & popularity of Chai undermine the effectiveness of dropping property-based assertions this late in the game. Chai has had 7 years worth of blog posts and tutorials published on the Internet with assertions like
expect(blah).to.be.true, so if a user unknowingly follows one of those outdated tutorials in chai@5, they now have an assertion that looks legit and has increased legitimacy from being a style that's demonstrated across many tutorials but now in chai@5 passes no matter what. - This type of bug report dropped drastically in chai@4 with the introduction of proxy protection. There's now only a couple of remaining assertions in Chai core that could be reasonably written such that they look legit but pass no matter what.
expect(fn).to.throw;andexpect('meow').to.be.a.string;. I'm not familiar with the implementation complexity and bugs that popped up when trying to fix this in chai@2; I guess I'd have to experience that pain myself to understand!
Good points about increased complexity for plugin authors, and consistency issues across both the Chai plugin ecosystem as well as JavaScript as a whole. I'm convinced that dropping property-based assertions would've been the right decision at the start of the project, but I'm not so sure it is now given Chai's history & popularity. That's a tough one.
| * invokes the target function and asserts that an error is thrown that's an | ||
| * instance of that error constructor. | ||
| * When invoked with one argument, and it's a function (presumably a | ||
| * constructor), `.throw` invokes the target function and asserts that it |
There was a problem hiding this comment.
Nit: s/presumably/typically/. presumably suggests we don't know what it will be, whereas typically suggests that is the common pattern.
lib/chai/core/assertions.js
Outdated
| * constructor, and the second is a string or regular expression, `.throw` | ||
| * invokes the function and asserts that an error is thrown that fulfills both | ||
| * conditions as described above. | ||
| * When invoked with two arguments, and the first is a function (presumably a |
There was a problem hiding this comment.
Same nit: s/presumably/typically/
| * | ||
| * expect(goodFn).to.not.throw(); | ||
| * expect(badFn).to.throw(String, 'salmon'); | ||
| * expect(badFn).to.throw(String, /salmon/); |
| * var badFn = function () { throw nonErrorObject; }; | ||
| * | ||
| * expect(badFn).to.throw(NonErrorConstructor, 'salmon'); | ||
| * expect(badFn).to.throw(new NonErrorConstructor('Illegal salmon!')); |
There was a problem hiding this comment.
If I am to understand the proposed algorithm, the only reason this does this is because the logic falls through to using deep-eql. We should mention that explicitly, if it is the case, as there's plenty of edge cases around deep-eql which might trip a user up when making this assertion.
BREAKING CHANGE: Drop support for versions of Node below v6, and update the Travis config accordingly.
BREAKING CHANGE: Update the deep equality algorithm used to compare `Error` objects.
meeber
force-pushed
the
overhaul-throw
branch
3 times, most recently
from
b1c9713
to
7f41fc9
Compare
|
@chaijs/chai Here's what I have in mind for solving problems 1, 2, and 4 that I described in my original post, while simultaneously simplifying code. Other assertions (e.g., This is still a draft, and I haven't updated the And here is what it'd look like if |
meeber
force-pushed
the
overhaul-throw
branch
from
7f41fc9
to
435b363
Compare
| } | ||
|
|
||
| return cri; | ||
| } |
There was a problem hiding this comment.
I'd be interested to see these functions move into https://github.com/chaijs/check-error where they can be tested for their surface area, and can be reviewed+merged+shipped easier there, thereby making this PR smaller.
There was a problem hiding this comment.
I thought about that. createErrCriteria could probably be moved to https://github.com/chaijs/check-error, but it'd be the only function there, without any real non-Chai applicability. assertErrCriteria is using a bunch of other Chai assertions internally, so I don't think it can be cleanly moved to a separate module until v5 when assertions are better modularized.
|
I'm still not convinced this is the right implementation. Gonna close for now and revisit post v5 release. |
I've been thinking lately about chaijs/check-error#18. I think some of my decisions in that PR were wrong; in particular, dropping support for checking non-
Errorinstances. Although only throwing instances ofError(including subclassed errors, likeTypeError) is a best practice, it's not an important enough issue to justify enforcing that best practice on users in the form of a breaking change. However, there are other issues that are important enough to justify breaking changes:Eliminating the ambiguity of
.throw('waffles'); currently it asserts that the target function throws either a string that contains "waffles", or an object that has a.messageproperty that contains "waffles". Users typically have only one of those scenarios in mind when writing the assertion, and it's an unwelcome surprise for the assertion to pass given the other scenario.Eliminating the ambiguity of
.not.throw(someCriteria); currently it asserts that the target function either doesn't throw, or it throws but the thrown value doesn't match one or more of the given criteria. Again, users typically have only one of those scenarios in mind when writing the assertion.Eliminating a reasonable-looking but actually-meaningless assertion from always passing; currently
expect(myFn).to.throwsilently passes, regardless of whether or notmyFnthrows, because.throwis only a method-based assertion. This is an unwelcome surprise for users of an assertion library that makes use of property-based assertions (e.g.,.true). (I'm opposed to jumping to the opposite extreme of removing all property-based assertions from Chai; despite these types of problems, I think property-based assertions are a critical part of Chai's identity and appeal for many users.)Replacing the seldom-used
.throw(ErrorInstance)behavior, which currently checks if the thrown value is strictly (===) equal toErrorInstance, with more useful behavior of checking that the thrown value is deeply equal toErrorInstance. This change was enabled by feat: change error comparison algorithm again deep-eql#59.This PR demonstrates the changes I have in mind in the form of an update to the docs for
.throw. I believe that implementing these changes would significantly simplify the code (and possibly eliminate the need for a separatecheck-errormodule altogether), as most of the complexity of the current implementation is caused by handling the ambiguity of the first two issues described above. It'd also lead to the creation of a similar.errorassertion.Questions:
new-chairepo?new-chairepo?