Assert: Exclude assert.step() calls from assert.expect() count#1775
Assert: Exclude assert.step() calls from assert.expect() count#1775Krinkle merged 1 commit intoqunitjs:mainfrom
assert.step() calls from assert.expect() count#1775Conversation
|
any chance that...?
|
|
@getify I considered that, but couldn't think of a satisfying approach that seemed worthwhile. Open to ideas! I generally try to limit semver-major to removing/changing already-deprecated behaviour. This means adding new features in previous releases first. The idea being that if you're warning-free on the last 2.x, you can expect to be fine on 3.0. There may be cases where a change is mutually exclusive (e.g. change default from X to Y), in which case we would not want to emit a warning for everyone that hasn't specified a preference. Such change would need to be very compelling, e.g. believed to be best for most new projects, and not break most existing projects. Then there are cases where a change is both mutually exclusive, and with only one available in a given release. In that case warning pre-upgrade would be disruptive if there is no way to satisfy said warning. This could inform a change in direction so that we don't create such situation (e.g. fork the method, add a parameter, add a config option, ..). But that comes at its own cost. In this case we have:
I don't see a good way to emit an (actionable) warning for this in 2.x. But, I very much like the idea of including a "You probably get this because..." in 3.0 when we detect that the wrong number is equal to the old expected number. I'll make sure we do that if nothing else. Is there an alternative approach here for 2.x that you think might be worthwhile? E.g. forking the method, adding a config option, etc. Or is it small enough to be tolerable to discover post-upgrade, and then fixup, so long that we have an improved error message for I also wonder if |
9e43e2f to
0b3e67b
Compare
If the old count is detected, we emit: > Expected 10 assertions, but 4 were run. > It looks like you are upgrading from QUnit 2. Steps no longer > count as separate assertions. https://qunitjs.com/api/assert/expect/
0b3e67b to
9f5c584
Compare
|
The last revision emits the following error when
|
I wasn't initially thinking the 2.x warning needed to be actionable, because it's just a warning (as opposed to a failing error). An error would of course need to be actionable. Maybe "warning" is the wrong term, perhaps it's some sort of info notice, so that it's not normally being printed but is printed for those who are doing any debugging with verbose logging level on, or whatever something like that?
This could work, in that it could default to printing the warning/notice/whatever in 2.x, BUT then the notice could say "do ___ to suppress this warning", and that ___ could be the actionable step to add a simple config option, to silence the warning. For example: QUnit.config.legacyStepCounting = true; // defaults to `false`In 2.x, if it's In 3.x, this would still default to |
|
@getify I see. That sounds good to me. I learn toward a slight variation, where a 2.x release would:
I only suggest this because the migration seems trivial. I prefer this over bringing the old behaviour to QUnit 3.0, and maintaining it over the long run. If we worry people might stay behind, I'd happily absorb the cost and let them upgrade with a simple switch like you suggest. |
|
That option seems fine to me. I was suggesting a slightly "softer" landing where it gets officially deprecated in 3.x and removed in 4.x or later. But if you think we can rip the bandaid off in 3.x, I'm in support. :) |
This example is now treated as having one assertion rather than three.
This better matches the mental model of building up a buffer of values and then comparing them, just like the following would be:
In addition to being less surprising to new developers and more intuitive to experienced ones, it also removes the need for busywork in the form of keeping the assertion count up-to-date when merely changing the number of values in the list, before performing the assertion. Updating the verification array should suffice.
I've also taken this moment to turn obvious program errors into exceptions rather than assertion failures, in line with how other QUnit assertion methods validate their arguments.
Fixes #1226.
/cc @getify