Skip to content

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

Merged
Krinkle merged 1 commit intoqunitjs:mainfrom
Krinkle:assert-step-count
Jul 13, 2024
Merged

Assert: Exclude assert.step() calls from assert.expect() count#1775
Krinkle merged 1 commit intoqunitjs:mainfrom
Krinkle:assert-step-count

Conversation

@Krinkle
Copy link
Copy Markdown
Member

@Krinkle Krinkle commented Jul 5, 2024

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 #1226.

/cc @getify

@Krinkle Krinkle requested review from gibson042 and mgol July 5, 2024 03:56
Copy link
Copy Markdown
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

@getify
Copy link
Copy Markdown

getify commented Jul 10, 2024

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
Copy link
Copy Markdown
Member Author

Krinkle commented Jul 10, 2024

@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 Krinkle force-pushed the assert-step-count branch from 9e43e2f to 0b3e67b Compare July 10, 2024 15:00
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/
@Krinkle Krinkle force-pushed the assert-step-count branch from 0b3e67b to 9f5c584 Compare July 10, 2024 15:02
@Krinkle
Copy link
Copy Markdown
Member Author

Krinkle commented Jul 10, 2024

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
Copy link
Copy Markdown

getify commented Jul 10, 2024

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
Copy link
Copy Markdown
Member Author

Krinkle commented Jul 12, 2024

@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
Copy link
Copy Markdown

getify commented Jul 12, 2024

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

@Krinkle Krinkle merged commit 5ee5ae0 into qunitjs:main Jul 13, 2024
@Krinkle Krinkle deleted the assert-step-count branch July 13, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Counter for expected assertions when using step(..) / verifySteps(..) is surprising (and undocumented!)

3 participants