Planned assertion documentation issue (or planned assertion bug)

[ArtemGovorov]

Readme says:

And this will result in an error because the code called more assertions than planned:

test(t => {
    t.plan(1);

    t.pass();

    setTimeout(() => {
        t.pass();
    }, 100);
});

however running the test gives:

✔ [anonymous]

  1 test passed

[vadimdemedes]

This is happening, because when first t.pass() is executed, AVA exits the test and treats it as finished. We had a TODO test for this case before, but removed it because we could not find a good & stable solution for this one.

[rmg]

I think the docs could use a little more clarifying. A common problem with test plans is forgetting to update them when new assertions are added. In cases where the assertions happen asynchronously (the primary use case for test plans), they are not seen even if they fail. The suite will continue to pass, even though failing tests have been added.

While this is less of a problem if one is disciplined about verifying their failing tests before fixing them, it would be nice if the tools helped. And if the tools can't help, then at least the readme should offer a warning about this scenario.

I suspect that if this was "fixed" in the code there would be a number of bug reports claiming AVA suddenly broke but really it just stopped hiding broken tests. This is a problem we have had to address in node-tap and it "broke" a lot of test suites when we did.

[sindresorhus]

@rmg As far as I know there's no way to actually fix this (would be so happy to be proven wrong), as we would never know whether an async planned assertion will happen at some point. Currently, we end the test in the next event loop when all planned assertions are fulfilled.

I removed the test in 7a3f444 when I realized it's not really possible. I had this idea of checking process._getActiveHandles().length === 1, but I don't think that'll work when tests run concurrently and there might be a lot of other async things going on in AVA itself.

How do you think we could further improve the docs about this? Would you be interested in doing a documentation pull request?

[rmg]

I'm planning on submitting a docs PR if I can remember to when I'm on a real computer. Meant to mention that in my comment above.

[jamestalmage]

If we are killing the process because we think we are done that's one thing, but the following should detect an issue:

test('a', t => {
    t.plan(1);
    t.ok(true);

    setTimeout(() => t.ok(true), 1000);  // test-b will still be running when this happens.
});

test('b', t => {
    setTimeout(() => t.end(), 2000);
});

Also, in the case of multiple test files A, B, C (in order of increasing execution time), how undesirable would it be to leave the forked process for A running until B and C have also finished (just to increase the chance of catching a plan error). Node forks are pretty lightweight as I recall.

[jamestalmage]

A setting that required you to always call t.end() might be helpful. In that case t.plan(x) just becomes another assertion for ... how many assertions you will have (meta).

Perhaps just t.expect(3), means "expect 3 assertions before t.done() is called"

[jamestalmage]

Or t.end(4) meaning - I'm done and should have 4 previous assertions

[rmg]

I'm not sure I see the usefulness of t.end(4).

I think the root of the conflict is that using t.plan(4) for auto-end and using t.plan(4) to assert than only 4 assertions were made are two someone contradictory goals. I'm not sure how to do both well at the same time.

[jamestalmage]

t.end(4) is the assertion without the auto-ending that comes with t.plan(4).

I don't think there is a great way to do both, and auto-ending is problematic for exactly the reasons you describe.

[vadimdemedes]

I agree with @rmg, t.end(4) does not really address this issue.

There's simply no way to detect, that more assertions are coming. We could just go mocha-way and display an error, if more assertions are happened after ending the test. That's the best we can do here.