Make assertions throw

[sindresorhus]

Currently, the AVA assertions don't throw. This has a lot of downsides:

• It's surprising behavior for new users, as other test frameworks don't do it like this. (Relevant: document that ava assertions don't throw #2624)
• It's surprising behavior for existing users too (I get surprised by it sometimes).
• It means that you have to guard anything coming after an assertion as the execution of the test would continue even after the assertion fails. It simply complicates writing tests.
• It means AVA cannot easily support third-party assertion frameworks.

Lets make them throw again. We can start by making it opt-in and then change the default in the next major version.

---

Related: #1485

[ZachHaber]

I'm thinking of trying to implement this after #3233 is merged in.

Is there a preferred way of setting up options for this? The only thing I'm worried about is that it'll be difficult to make the typescript declarations swap with an option.

What would you think about making a separate import for tests with throwing? It would be a bit more involved for users to make that the default in the next major version: a regex search and replace ava/throws to ava. Or to not break anything too bad for users who opt-in, we could use the exports field in the package.json to re-export the same file for another major version.

import test from 'ava/throws';

[sindresorhus]

I would make the breaking change now and release a new major AVA version that also targets Node.js 16. I don't think the work to make it opt-in first and duplicate a lot of code is worth it. But it's up to @novemberborn.

[novemberborn]

There’s a couple things around errors I want to get into AVA 6, and actually I’d then drop Node.js 16 support before release.

If this lands in time we can include it without a config option.

Implementation wise I think failed assertions should throw a TestFailure error. If it is caught the test should still fail. We don’t need to expose that error constructor, having a name field should suffice. For the first version let’s not expose any details about the failed assertion (hence not throwing an AssertionError with the implication you can catch it and pass the test).

Tests already fail if an error is thrown. I think the twist is that maybe we record the TestFailure errors that were thrown during execution and if we see one that did not come from a failed assertion that is a special reason to fail the test? We want people to use `t.fail()’ not these errors. But I suppose that could come later.

[ZachHaber]

@novemberborn, I found out something that is problematic: Assertions in typescript require explicit type annotations for all parts of the call: microsoft/TypeScript#45385, microsoft/TypeScript#33622
This means that in order to setup type assertions with the existing API, users will need to explicitly annotate the ExecutionContext in every test.

test('falsy', (t)=>t.assert(1)); // Typescript Error
test('falsy', (t: ExecutionContext)=>t.assert(1)); // works correctly

The reason typescript complains is because t is technically inferred from the type definition of test and ImplementationFn, so there's no way to get it to work without a bunch of pointless boilerplate in TS, and a ton of JSDocs in JS. Which would probably make the current approach using typeguards more approachable in the JS side.

From a type-definitions approach, the easiest way I can think of is that we just have a top level asserts export that has the same shape as the current Assertions type, but with the throwing behavior on all the assertion objects. None of the assertions rely on execution context, so there will be an explicit type definition to them.

However, from a coding standpoint from the little I've looked at the codebase, doing that will likely break a ton of things including the ability to count how many assertions are used for t.plan. Assuming that ava is only meant to work on node.js (as opposed to other JS runtimes e.g. deno, browser), then I believe it would be possible to use async context to help keep track of the details that the ExecutionContext would track. I've never worked with that API, so I'm not entirely sure if what I'm thinking of is even feasible. It would also definitely require at least node 16.4, so it would need to be a breaking change to use it anyway.

[novemberborn]

That's too big a change. Is this not worthwhile then? Is it better in TS5.2?

[ZachHaber]

It's not better in TS5.2 (nor the nightly version). I'm not sure if they will ever relax that restriction on assertions in TS. There's been no indication of them even being interested in doing so in the issues I've looked through, so I wouldn't get my hopes up on them changing it.

[ZachHaber]

It looks like a lot of this was discussed before back in 2020: #2449

From my perspective, I think that making the assertions throw without the typescript asserts would be very counter intuitive for anyone using types.

From playing around with things, I realized that it's possible to just make an assert function that you would use to wrap the ava assertions.

import assert from 'node:assert';
//...
const value: unknown = 3;
assert(t.is(value,3));
value // is of type `3`

So, by making an assert or throws function or something that throws something ava can pick up to realize that the test should end and that X assertion is the reason why, could work. Even if it is a bit clunkier than just having t.is be an assertion by itself. It would also be a far less huge change than getting the assertions as a global export and using async context as I mentioned before.

[novemberborn]

When we make assertions throw errors, function overloads that return never should do the trick without needing type assertions.

[ZachHaber]

I don't think that'll be sufficient. The return never aspect of typescript is solely about control-flow analysis rather than types. It goes in the opposite direction than you'd think for an assertion: instead of saying "continue execution if this condition is met" it effectively says "stop execution when this condition is met".

It would also be unable to narrow types at all, which would realistically make worrying about the typescript entirely pointless. Example. Plus if your types don't match the specific overloads we setup correctly enough, then you'll get a ton of "unreachable code" errors. Any attempt at figuring out what unknown is would require type-casting ahead of time to avoid an unreachable code error.

[novemberborn]

Thanks for the example. We're not considering any changes beyond having the assertions throw. Sounds like #3233 should be reverted then?

[ZachHaber]

I don't see a need to revert it, nothing in that PR changes anything for this issue.

I did some double checking with the asserts needing the explicit type definition. For JS users, unless you have checkJs enabled, the asserts is just treated as void and doesn't show any errors, but it still won't narrow the types unless you explicitly use JSDoc syntax such as /** @type {import('ava').ExecutionContext} */ t).

Sadly, there's no way to make the behavior an override (without an extra parameter or something) as whichever override is defined first will apply regardless of the explicit vs inferred ExecutionContext type.

I would personally prefer having to explicitly add the t: ExecutionContext on every test case than have it without the type narrowing behavior. I'd also prefer the current behavior of using assert(t.assert(value)) or if(!t.assert(value))throw new Error() to get the immediate test end and type narrowing to not having the type narrowing in the library at all. : ExecutionContext isn't that much boilerplate to add to the function in comparison to the boilerplate that would be needed in TS without the narrowing.