Skip to content

Fix #1334#1348

Merged
mcollina merged 2 commits intofastify:masterfrom
0xsuid:master
Dec 24, 2018
Merged

Fix #1334#1348
mcollina merged 2 commits intofastify:masterfrom
0xsuid:master

Conversation

@0xsuid
Copy link
Copy Markdown
Contributor

@0xsuid 0xsuid commented Dec 22, 2018 •
edited
Loading

Added two Missing error codes in files lib/reply.js and lib/wrapThenable.js which fix issue #1334

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

@0xsuid 0xsuid mentioned this pull request Dec 22, 2018
Closed
2 tasks
@0xsuid
Copy link
Copy Markdown
Contributor Author

0xsuid commented Dec 22, 2018 •
edited
Loading

I am not sure about change made to hooks.test.js is appropriate so kindly review it.

delvedor
delvedor reviewed Dec 22, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@delvedor delvedor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good work! The test is ok, but you need to add another one to check that the send error as well :)

Can you also update the documentation?

@0xsuid 0xsuid force-pushed the master branch 2 times, most recently from 26a62b0 to df5dd35 Compare December 23, 2018 06:21
@0xsuid
Copy link
Copy Markdown
Contributor Author

0xsuid commented Dec 23, 2018 •
edited
Loading

Documentation is updated @delvedor @mcollina, I need help/hint regarding make new test "to check the send error".

delvedor
delvedor reviewed Dec 23, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

You should update this test:

fastify/test/hooks.test.js

Lines 2006 to 2038 in 8987e61

test('reply.send should throw if called inside the onError hook', t => {
t.plan(3)
const fastify = Fastify()
const err = new Error('kaboom')
fastify.addHook('onError', (request, reply, error, next) => {
try {
reply.send()
t.fail('Should throw')
} catch (err) {
t.is(err.message, 'You cannot use `send` inside the `onError` hook')
}
next()
})
fastify.get('/', (req, reply) => {
reply.send(err)
})
fastify.inject({
method: 'GET',
url: '/'
}, (err, res) => {
t.error(err)
t.deepEqual(JSON.parse(res.payload), {
error: 'Internal Server Error',
message: 'kaboom',
statusCode: 500
})
})
})

@0xsuid
Copy link
Copy Markdown
Contributor Author

0xsuid commented Dec 23, 2018 •
edited
Loading

t.is(err.message, 'You cannot use `send` inside the `onError` hook')
Should it changed to be t.pass() ?? @delvedor

@delvedor
Copy link
Copy Markdown
Member

delvedor commented Dec 23, 2018

@0xsuid Nope, you can leave it as is :)

mcollina
mcollina reviewed Dec 23, 2018
View reviewed changes
@mcollina
Copy link
Copy Markdown
Member

mcollina commented Dec 23, 2018

I've just added a nit.

@0xsuid
Copy link
Copy Markdown
Contributor Author

0xsuid commented Dec 23, 2018

Test updated :) @delvedor @mcollina

@0xsuid
Copy link
Copy Markdown
Contributor Author

0xsuid commented Dec 23, 2018 •
edited
Loading

Error On Linux node_8_x 😕
From Line 3141:

                                     # Subtest: file option   
2018-12-23T13:55:02.6524332Z         1..13  
2018-12-23T13:55:02.6524482Z         ok 1 - should not error  
2018-12-23T13:55:02.6524634Z         ok 2 - expect truthy value  
2018-12-23T13:55:02.6524860Z         not ok 3 - ENOENT: no such file or directory, open '/tmp/sonic-boom-5094-424,796640439-0'  
2018-12-23T13:55:02.6525012Z           ---  
2018-12-23T13:55:02.6525163Z           errno: -2  
2018-12-23T13:55:02.6525220Z           code: ENOENT  
2018-12-23T13:55:02.6525255Z           syscall: open  
2018-12-23T13:55:02.6525432Z           path: '/tmp/sonic-boom-5094-424,796640439-0'  
2018-12-23T13:55:02.6525495Z           test: file option  
2018-12-23T13:55:02.6525529Z           ...  
2018-12-23T13:55:02.6525561Z           
2018-12-23T13:55:02.6525611Z         # test count(3) != plan(13)  
2018-12-23T13:55:02.6525650Z         # failed 1 of 3 tests  
2018-12-23T13:55:02.6525815Z     not ok 21 - file option # time=20.103ms

@delvedor @mcollina

@mcollina
Copy link
Copy Markdown
Member

mcollina commented Dec 23, 2018 •
edited
Loading

CI restarted, it should be a flaky test.

@0xsuid
Copy link
Copy Markdown
Contributor Author

0xsuid commented Dec 24, 2018

So, what should we do now? @mcollina @delvedor

@mcollina
Copy link
Copy Markdown
Member

mcollina commented Dec 24, 2018

@0xsuid I'm rerunning CI again now.

@mcollina
Copy link
Copy Markdown
Member

mcollina commented Dec 24, 2018

CI was flaky, landing.

@mcollina mcollina merged commit a584210 into fastify:master Dec 24, 2018
@fralonra fralonra mentioned this pull request Dec 24, 2018
Closed
21 tasks
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 12, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2022
@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mcollina mcollina mcollina left review comments

@delvedor delvedor delvedor left review comments

Assignees

No one assigned

Labels

semver-minor Issue or PR that should land as semver minor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@0xsuid @delvedor @mcollina @Eomm