Skip to content
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

Merged
merged 4 commits into from Aug 11, 2020

Conversation

mastermatt
Copy link
Member

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

…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.
@mastermatt mastermatt added the bug label Jul 23, 2020
@mastermatt mastermatt requested a review from a team July 23, 2020 20:04
.persist()
.get('/')
.reply(function () {
seenFooHeaders.push(this.req.headers.foo)
Copy link
Member

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'.

Copy link
Member Author

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')

Copy link
Member Author

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?

Copy link
Member

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).

@mastermatt mastermatt merged commit 6260217 into nock:main Aug 11, 2020
@mastermatt mastermatt deleted the 2054/fix-parallel-request-context branch August 11, 2020 18:08
@github-actions
Copy link

🎉 This PR is included in version 13.0.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

[Regression 13.0.2] Persistent interceptors using request
2 participants