chore: remove try-catch from tests where functions are async #2531
Conversation
achingbrain
force-pushed
the
remove-try-catch-from-tests
branch
from
2e33d12
to
d76f147
Compare
We had a few instances in the tests where assertions may be missed due to functions not throwing where we thought they would so this PR refactors that code to use `.then(onResult, onReject)` instead. Follows on from #2514 (comment)
achingbrain
force-pushed
the
remove-try-catch-from-tests
branch
from
c9f879e
to
5cc3072
Compare
test/cli/daemon.js
Outdated
| expect(err.killed).to.be.true() | ||
| expect(stdout).to.include('Daemon is ready') | ||
| } | ||
| ) |
There was a problem hiding this comment.
These look equivalent to me - am I missing something?
There was a problem hiding this comment.
No, you're not missing anything - it was more an attempt at consistency with other tests that have been refactored to be more rigorous.
There was a problem hiding this comment.
Changes look ok to me, I'm just not sold on mixing await and .then. To me this reads like there's something weird going on that warrants falling back to using .then when that's not true in most cases here.
IMHO we should only use .then if we really have to. Using it when it's not absolutely required sets a precedent for contributors that using .then is ok, when actually we strongly prefer using await.
|
The I don't mind either way, but if we're going to go back & forth on this, something should be added to |
|
I'm not a huge fan of mixing the two either, but I think it has it's place. Tests where you expect failures are a place I find them useful, because IMO it helps prevent false positive tests. It's very easy to forget to throw or add the |
|
Ah ok now I understand - would an assertion like https://github.com/avajs/ava/blob/master/docs/03-assertions.md#throwsasyncthrower-expected-message be the solution then? |
|
We could do this sort of thing instead? await expect(promise)
// where we assert on the error type
.to.eventually.be.rejectedWith(TypeError)
// or where we assert on the exact error (e.g. when stubbing)
.to.eventually.be.rejectedWith(someErrInstance)
// or where we assert on the error message
.to.eventually.be.rejectedWith(Error, /match regex or could be string/)
// or where we do expect(err).to.be.ok(); expect(err.code).to.equal('...')
.to.eventually.be.rejected()
.with.property('code', 'ERR_A_CODE') |
* chore: remove try-catch from tests where functions are async We had a few instances in the tests where assertions may be missed due to functions not throwing where we thought they would so this PR refactors that code to use `.then(onResult, onReject)` instead. Follows on from #2514 (comment)
We had a few instances in the tests where assertions may be missed due to functions not throwing where we thought they would so this PR refactors that code to use
.then(onResult, onReject)instead.Follows on from #2514 (comment)