Assert: Exclude `assert.step()` calls from `assert.expect()` count

[Krinkle]

This example is now treated as having one assertion rather than three.

QUnit.test('example', function (assert) {
  // assert.expect(1);
  // Previously:
  // assert.expect(3);

  assert.step('x');
  assert.step('y');

  assert.verifySteps(['x', 'y']);
});

This better matches the mental model of building up a buffer of values and then comparing them, just like the following would be:

QUnit.test('example', function (assert) {
  // assert.expect(1);

  var data = [];
  data.push('x');
  data.push('y');

  assert.deepEqual(data, ['x', 'y']);
});

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 https://github.com/qunitjs/qunit/issues/1226.

/cc @getify

[getify]

any chance that...?

1. a deprecation-type warning could be added to the 2.x branch, to start warning about this change (so only when someone uses expect() and verifySteps() in the same test)
2. the 3.x could have logic that if there's an error related to a count mismatch (again, when expect() and verifySteps() were used together), that it adds a note hinting that this counting-algorithm has changed and that's likely the culprit?

[Krinkle]

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

• Change in default (how we count).
• Mutually exclusive. One way in 2.x, one way in 3.0.
• We can detect if a wrong count is likely due to expecting old behaviour.

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 expect() failures that explains the change and includes the correct number?

I also wonder if eslint-plugin-qunit could autofix some of this. It's probably a stretch, but for simple cases it seems plausible? To avoid disruption you'd want to limit the autofix to cases where it is confident in understanding your code (e.g. only if the number is a literal and matches what it belives the old number should have been for your test function), and when QUnit 3.0 is installed. It'd analyse the test fn and count assert steps and other asserts (sans known uncounted methods: timeout, step, expect, async, pushResult; but include unknowns/plugins?).

[Krinkle]

The last revision emits the following error when expected && steps && (expected === (assertions + steps)):

Expected 9 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/

[getify]

I don't see a good way to emit an (actionable) warning for this in 2.x.

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?

---

...adding a config option...

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 false (default), then the step counting still works as it always has, BUT it warns about it changing in the next 3.x major release. But if it's then set to true, then it keeps working as current and keeps quiet about it.

In 3.x, this would still default to false, but now the step counting would be changed. While false, if the inferable mistake has been made, then the error notice (as above) would be issued. But if it's set to true, then the counting should go back to working as it did in 2.x ("legacy" mode). You can note in the 3.x docs that this legacy mode has been deprecated as of 3.x and is subject to removal in future major releases (4.x or later).

[Krinkle]

@getify I see. That sounds good to me. I learn toward a slight variation, where a 2.x release would:

• Add a new option QUnit.config.countStepsAsOne (false by default). When enabled, we count the steps (without changing the fact that steps are internally stored as assertions in 2.x), and require the count to be assertions - steps. Akin to how this PR counts steps for the improved error message, but reversed.
• Emit a deprecation warning when countStepsAsOne=false (default), and expect() is used, and verifySteps() is used, indicating that the count will change in 3.0. You can resolve the warning by enabling countStepsAsOne=true and update the numbers. In 3.0 the option wouldn't exist, as it matches the same behaviour.

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.

[getify]

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. :)