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.

Sign up for GitHub

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

Jump to bottom

fix: Parallel requests on same Interceptor are exposed correctly in reply functions #2056

Merged
merged 4 commits into from
Aug 11, 2020
Merged

fix: Parallel requests on same Interceptor are exposed correctly in reply functions #2056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0d15e0
fix: Parallel requests on same Interceptor are exposed correctly in r…
mastermatt Jul 23, 2020
73cf1cc
Merge branch 'main' into 2054/fix-parallel-request-context
mastermatt Aug 9, 2020
db44d2d
Merge branch 'main' into 2054/fix-parallel-request-context
mastermatt Aug 11, 2020
d42bae5
fixup: use Sinon in test
mastermatt Aug 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/intercepted_request_router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,6 @@ class InterceptedRequestRouter {
)

matchedInterceptor.markConsumed()
matchedInterceptor.req = req

// wait to emit the finish event until we know for sure an Interceptor is going to playback.
// otherwise an unmocked request might emit finish twice.
Expand Down
7 changes: 7 additions & 0 deletions lib/playback_interceptor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,13 @@ function playbackInterceptor({
response.rawHeaders = [...interceptor.rawHeaders]
logger('response.rawHeaders:', response.rawHeaders)

// TODO: MAJOR: Don't tack the request onto the interceptor.
// The only reason we do this is so that it's available inside reply functions.
// It would be better to pass the request as an argument to the functions instead.
// Not adding the req as a third arg now because it should first be decided if (path, body, req)
// is the signature we want to go with going forward.
interceptor.req = req

if (interceptor.replyFunction) {
const parsedRequestBody = parseJSONRequestBody(req, requestBodyString)

Expand Down
23 changes: 21 additions & 2 deletions tests/test_intercept_parallel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ require('./setup')

describe('interception in parallel', () => {
const origin = 'https://example.test'
const makeRequest = () =>
got(origin)
const makeRequest = opts =>
got(origin, opts)
.then(res => res.statusCode)
.catch(reason => {
if (reason.code === 'ERR_NOCK_NO_MATCH') return 418
Expand Down Expand Up @@ -61,4 +61,23 @@ describe('interception in parallel', () => {
expect(results.sort()).to.deep.equal([200, 201, 418])
expect(nock.isDone()).to.equal(true)
})

it('provides the correct request instance on the Interceptor inside reply callbacks', async () => {
const seenFooHeaders = []
nock(origin)
.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).

return [200]
})

await Promise.all([
makeRequest({ headers: { foo: 'A' } }),
makeRequest({ headers: { foo: 'B' } }),
])

expect(seenFooHeaders.sort()).to.deep.equal(['A', 'B'])
expect(nock.isDone()).to.equal(true)
})
})