Skip to content

Conversation

@jasontbradshaw
Copy link

@jasontbradshaw jasontbradshaw commented Oct 13, 2020

This makes most of the built-in assertion functions return Boolean
values, true when the assertion succeeds and false when the
assertion fails.

This is intended to allow a user to emulate the assertion function
throwing an exception by allowing control flow to only proceed if the
assertion passes, something like:

if (t.is(foo, 42)) {
  // Do some other things that only make sense when `foo` is `42`.
}

snapshot is left alone since it doesn't seem to make much sense to
make it return a Boolean in this way, given its existing contract.

pass and fail are somewhat special since they act more as "flags"
that tell the test runner to pass or fail the test and don't actually do
any asserting. In their cases, their returned flag values are chosen for
use as analogues of if (true) { ... } and if (false) { ... }, i.e.
if (pass()) { ... } and if (fail()) { ... }.

Fixes issue #2455, and refines the work done in #2586.

This makes most of the built-in assertion functions return Boolean
values, `true` when the assertion succeeds and `false` when the
assertion fails.

This is intended to allow a user to emulate the assertion function
throwing an exception by allowing control flow to only proceed if the
assertion passes, something like:

```
if (t.is(foo, 42)) {
  // Do some other things that only make sense when `foo` is `42`.
}
```

The various "throws" functions are left alone since they already return
exception values. `snapshot` is left alone since it doesn't seem to make
much sense to make it return a Boolean in this way.

`pass` and `fail` are somewhat special since they act more as "flags"
that tell the test runner to pass or fail the test and don't actually do
any asserting. In their cases, their returned flag values are chosen for
use as analogues of `if (true) { ... }` and `if (false) { ... }`.

Fixes issue #2455.
@sindresorhus
Copy link
Member

Duplicate of #2586

@sindresorhus sindresorhus marked this as a duplicate of #2586 Oct 14, 2020
@jasontbradshaw
Copy link
Author

jasontbradshaw commented Oct 14, 2020

I did this mostly because there hasn't been any movement on the original issue for a while, and this is my version of planting my flag and saying "I'm willing to get this over the line" :)

If the original author(s) of #2586 would like to do so, I'd be quite happy to let them!

I also realized this doesn't implement Boolean return for notThrows(Async), so I'll implement that and push as a follow-up commit tomorrow.

@jasontbradshaw
Copy link
Author

As an aside, the tests are the hairiest part of this whole thing by far, and I'm not too attached to my approach.

My approach was to change the test functions implemented in this test suite itself to additionally validate that the returned value matches the new "assertions return Boolean indicating whether they succeeded" contract, and to add functions that don't bother to validate this (for things like throwsAsync and friends) as an escape hatch for those particular cases.

I'm happy to entertain less... interesting approaches, assuming they don't involve essentially re-writing all these tests, which isn't something I'm willing to take on at the moment. I'd be happy to leave a comment suggesting a major refactor of this file, though.

});

test('.notThrowsAsync() returns undefined for a fulfilled promise returned by the function', t => {
test('.notThrowsAsync() returns true for a fulfilled promise returned by the function', t => {
Copy link
Author

@jasontbradshaw jasontbradshaw Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a change in the method contract, so it may be that we don't make this change to notThrowsAsync until the next major version.

If we don't make the change to notThrowsAsync, though, that kind of makes me wonder whether we should decline to make the change to notThrows too, to keep parity between the sync and async versions of the method.

}));

test('.notThrowsAsync() returns undefined for a fulfilled promise', t => {
test('.notThrowsAsync() returns true for a fulfilled promise', t => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/avajs/ava/pull/2601/files#r504349454, this is a change in method contract, of which I'm unsure of the larger implications.

@novemberborn
Copy link
Member

Thanks for the PR @jasontbradshaw. #2586 is back on track but I reckon that could use the updated type definitions from this one.

I think the tests for notThrows are explicit like that because of how they contrast with the throws() behavior. I'm fine with the subtle change in behavior.

If it's OK with you I'll close this in favor of #2586.

@jasontbradshaw
Copy link
Author

jasontbradshaw commented Oct 18, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants