Skip to content
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

Disable snapshots and try assertions in hooks #2523

Closed
novemberborn opened this issue Jun 21, 2020 · 3 comments · Fixed by #2527
Closed

Disable snapshots and try assertions in hooks #2523

novemberborn opened this issue Jun 21, 2020 · 3 comments · Fixed by #2527

Comments

@novemberborn
Copy link
Member

Per #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
Copy link
Contributor

oantoro commented Jun 23, 2020

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

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

oantoro commented Jun 24, 2020

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

novemberborn added a commit that referenced this issue Jul 4, 2020
`t.try()` has never worked in hooks. Fail the hook properly instead of letting it crash.

`t.snapshot()` sort of works, but not really. Add an experiment so support can be disabled. This will become the default behavior in the next major AVA release.

Fixes #2523.

Co-authored-by: Mark Wubben <mark@novemberborn.net>
@novemberborn novemberborn removed the breaking requires a SemVer major release label Jul 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants