search

avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

Disable snapshots and try assertions in hooks #2523

Closed novemberborn closed 4 years ago

novemberborn commented 4 years ago

Per https://github.com/avajs/ava/issues/2511, t.try() does not work in hooks. Rather than crashing we should fail the hook properly.

Snapshots work in hooks. It's OK in pre-test hooks, since those have unique titles, but the behavior is a little odd in the file hooks. I don't see how this is useful. Let's make snapshots-in-hooks fail. Unfortunately this is a breaking change, so we'll have to put this behind a flag.

oantoro commented 4 years ago

Hello @novemberborn is it enough to put the following hook checking before line https://github.com/avajs/ava/blob/master/lib/test.js#L94 

if (test.isHook) {
  throw new Error(`t.try() is not allowed in hooks.`);
}

and for the t.snapshot() part, it seems the test need to pass test.isHook in ExecutionContext.super() method parameter to make the Assertions.snapshot() know if the test is a hook and call fail() properly. Or maybe there is any other suggestion in doing this properly? I am not too familiar with the source code.

novemberborn commented 4 years ago

@okyantoro that sounds about right. It may be nice to add a more explicit disableSnapshots property or something like that.

Since you seem keen I'll assign this to you. Apologies if that was presumptuous.

oantoro commented 4 years ago

Thank you @novemberborn I have implemented the code. Let me know if additional work is required.