-
Notifications
You must be signed in to change notification settings - Fork 783
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
Counter for expected assertions when using step(..)
/ verifySteps(..)
is surprising (and undocumented!)
#1226
Comments
Thanks for the detailed explanation! When we originally implemented this feature, I didn't think much about the impact on expected assertion counts, so we just went with the default which is to increment the internal counter for any assertion calls. I agree that the reasoning in (2) makes sense, however, since this would now be a breaking change we will have to wait for QUnit 3.0 to be released before making the change. In the interim, we will add a note the documentation that calls out the behavior. @qunitjs/qunit-team if any of you all have thoughts on how we should count these assertions, please chime in 😄 |
I've been thinking about what API change could help here. The wider problem, I think, is that when an Assert method internally uses (multiple) other assertions, the count can be confusing. I too agree that approach (2) seems preferable here. Perhaps an (invisible) abstraction over the Assert class would help? A proxy to differentiate between direct calls on the assert parameter, and internal calls. This subclass would increment the counter for certain methods and then forward the call. Once inside, subsequent calls bypass the counter. However, for the particular issue with QUnit.test('bad example', function (assert) {
var obj = new Example();
obj.here(function (val) {
assert.ok(val);
});
obj.there(function (val) {
assert.ok(val);
});
}); We don't even check in which order things happen., how often they happen, what value is received. We don't even know if one of them is skipped entirely because the assertion is not under our control anymore, but under the subject's control. expect()Firstly, I'd say assertions never belong in a nested callback[1]. Moving assertions out avoids assertions being skipped and naturally produces an assertion failure if the assignment didn't happen. In addition, not having nested assertions also makes If you keep assertions to the top-level, the only way an assertion can be skipped is by an early abort, which QUnit has always caught. QUnit.test('example', function (assert) {
var x = false, y = false, obj = new Example();
obj.here(function (val) {
x = val;
});
obj.there(function (val) {
y = val;
});
assert.strictEqual('x', x);
assert.strictEqual('y', y);
}); This still doesn't assert the order or frequency of events. One could use step()A more elegant solution than QUnit.test('example', function (assert) {
var log = [];
var obj = new Example();
obj.here(function (val) {
log.push({ here: val });
});
obj.there(function (val) {
log.push({ there: val });
});
assert.deepEqual(
[ { here: 'x' }, { there: 'y' } ],
log
);
}); I absolutely love this pattern and can't recommend it enough. It's simple and yet very strict. And is in fact what I do acknowledge the wider problem of Assert methods making other assertions, and that definitely calls for a solution. But, while we might find other use cases for such abstraction, it seems right now the only use case is [1]: Except in a promise callback, which can be verified by QUnit if returned from the test function. One can also use async-await instead in modern code which avoids nested assertions even for async code. |
FWIW, I have never used Now that I've found the But as to the bigger point of if |
BTW, in case it's useful context, here's my general pattern for my asynchronous tests: QUnit.test( "something", async function test(assert){
var done = assert.async();
var somethingExpected = whatever;
try {
var somethingActual = whateverAsyncPromise();
somethingActual = await somethingActual;
}
catch (err) {
assert.expect(1);
assert.pushResult({ result: false, message: (err.stack ? err.stack : err.toString()) });
done();
return;
}
assert.expect(1);
assert.strictEqual(somethingActual,somethingExpected,"hello!");
done();
} ); BTW, I dunno if I was doing something wrong, but I added the If that's supposed to work, it wasn't for me. If that's not supposed to work, I think that could be a useful feature to consider. :) |
@getify Yep, I would also expect that to work. The following should do it: QUnit.test("something", async function (assert) {
var something = await whateverAsyncPromise();
assert.strictEqual(something, "whatever", "hello!");
}); I haven't confirmed it myself. But we already support explicitly returning a Thenable from the test callback. Using an async function should behave the same way. It's even documented at https://api.qunitjs.com/QUnit/test. QUnit will wait for the returned promise (async function) to settle. It will also assert that it gets resolved. It will detect a rejection (async exception) and reports its details. I would assume no async |
Pleased to report that by removing the QUnit.test( "something", async function test(assert){
var somethingExpected = whatever;
var somethingActual = whateverAsyncPromise();
somethingActual = await somethingActual;
assert.expect(1);
assert.strictEqual(somethingActual,somethingExpected,"hello!");
} ); Thanks for the suggestion, @Krinkle! |
I agree of the two directions, option 2 seems preferred. The selling point for me personally is that it means you could use I've deprioritised this so far because the need for |
I've authored many hundreds of test cases (with assert.expect( 6 ); // note: 1 assertions + 5 `step(..)` calls So, with the code comment, it's clear any time I read the tests where the count is coming from. It's an additional maintenance burden and I occasionally forget to update the code comment, but... it works. I still think this counting strategy could be changed some day, but I've managed fine without the change, so far. |
Tip
Migration guide has been published at: https://qunitjs.com/api/assert/expect/
Tell us about your runtime:
What are you trying to do?
I'm trying to use the
step(..)
/verifySteps(..)
API, but I had a failure related to the number of expected assertions.The documentation for this feature doesn't mention the impact on expected assertion count at all. So at a minimum, the docs need to be updated. But I also think the current behavior is counter-intuitive.
Code that reproduces the problem:
What did you expect to happen?
I assumed the number of assertions to expect would either correlate to the number of
step(..)
calls (2), OR to the number ofverifySteps(..)
calls (1).What actually happened?
The failure error message says 3, not 1 or 2. So clearly the counter is incrementing with both the
step(..)
calls and theverifySteps(..)
calls.This feels very strange and surprising to me. Even if it had been documented that way, I think it leads to more confusion with test authoring. It should consider only one or the other, not both.
The argument for using only the
step(..)
calls in the counter:step(..)
is kinda like anok( true, .. )
call, so each timestep(..)
happens, make sure it's counted. If you know there are 5 steps to some algorithm, it makes intuitive sense to increase your expected count by 5.Moreover, it doesn't make sense to include
verifySteps(..)
in this count in the same way that the call toassert.expect(..)
doesn't itself get included in the count.The argument for using only the
verifySteps(..)
calls in the counter:step(..)
is conceptually like just pushing an entry into an array. We haven't verified anything yet. There's notrue
orfalse
passing that's happening at that point. The assertion doesn't happen until you call theverifySteps(..)
call, which is conceptually like callingdeepEqual(..)
on the array thatstep(..)
is pushing into.Usually you make lots of
step(..)
calls, but only oneverifySteps(..)
call. So the counter should only increment once with that call, regardless of how manystep(..)
s there are.Moreover, you're already implicitly counting the number of
step(..)
calls that happened, because the only way entires get into the internal array you're comparing to is by callingstep(..)
, so theverifySteps(..)
is already checking that the number -- not just the order! -- ofstep(..)
calls is correct. No need for that to be included in the count.I think either of these lines of argument is compelling. Personally, I think (2) is how my brain works. The style of how I lay out my tests, my assertions are all collected together at the end, so I expect to be able to see the same number of assertions listed, line-by-line, as what I pass to
expect(..)
. Ifstep(..)
is included in that count, I have to look all over other parts of the test code to verify that my number matches. This is clunky.The text was updated successfully, but these errors were encountered: