-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
test: add tests for error handling #5451
Conversation
// In this test file numbered comments indicate the order statements are expected to execute | ||
|
||
test('encapsulates an asynchronous error handler', async t => { | ||
t.plan(4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.plan(4) | |
t.plan(3) |
Why t.plan
is 4
but not 3
?
I can see three t.equal
only and none of the code path run twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one missing @domdomegg
@domdomegg would you also add a fix? |
Hi folk, it's been a while since I've looked at the fastify code, especially since #5222 which this is based off. I think I was wrong here, and I think #5445 does fix it. Think I've confused myself with misunderstanding some of the errors from the tap test runner 🤦, thank you @climba03003 for flagging this. Glad this is all finally sorted :) |
@domdomegg I think this PR still worth merging. Would you consoder re-open and fix the test plan? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
These are failing tests to show that #5445 does not fix #5220
Checklist
npm run test
andnpm run benchmark
and the Code of conduct