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
fix: Parallel requests on same Interceptor are exposed correctly in reply functions #2056
fix: Parallel requests on same Interceptor are exposed correctly in reply functions #2056
Conversation
…eply functions. Fixes regression created in nock#2033. Reply functions get access to the current request via the Interceptor, which acts as the context during the function executions. The previous change, incorrectly moved the line where the request was attached to the Interceptor up in the flow and out of the same microtask that calls the reply functions. Causing requests in parallel to lose the reference to the correct request.
tests/test_intercept_parallel.js
Outdated
.persist() | ||
.get('/') | ||
.reply(function () { | ||
seenFooHeaders.push(this.req.headers.foo) |
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.
Could this be clearer using a spy? You could assert called with 'A'
, and then a second assert called with 'B'
.
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.
Sorry, looks like I lost track of the PR.
To use Sinon for the assertions, I think it would look like the following. I'm not sure it makes the test any clearer.
const fooHeadersStub = sinon.stub()
nock(origin)
.persist()
.get('/')
.reply(function () {
fooHeadersStub(this.req.headers.foo)
return [200]
})
await Promise.all([
makeRequest({ headers: { foo: 'A' } }),
makeRequest({ headers: { foo: 'B' } }),
])
expect(fooHeadersStub).to.have.callCount(2)
expect(fooHeadersStub).to.have.been.calledWithExactly('A')
expect(fooHeadersStub).to.have.been.calledWithExactly('B')
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.
@paulmelnikow I'd like to get this out. Any thoughts on preference between the current approach with an array vs Sinon?
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.
Just played around with this a bit. Personally I think the Sinon version is better. Though I do think this is just a style preference.
You could use expect(fooHeadersStub).to.have.been.calledTwice()
in place of callCount(2)
.
🎉 This PR is included in version 13.0.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes regression created in #2033.
Reply functions get access to the current request via the Interceptor,
which acts as the context during the function executions.
The previous change, incorrectly moved the line where the request was attached
to the Interceptor up in the flow and out of the same microtask that calls the
reply functions. Causing requests in parallel to lose the reference to the correct request.
Fixes: #2054