Skip to content

Commit

Permalink
fix: prerequest correlation for various retried and cached requests (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
ryanthemanuel authored Sep 11, 2023
1 parent 86d8b96 commit 70248ab
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 5 deletions.
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ _Released 09/12/2023 (PENDING)_

**Bugfixes:**

- Edge cases where `cy.intercept()` would not properly intercept and asset response bodies would not properly be captured for test replay have been addressed. Addressed in [#27771](https://github.com/cypress-io/cypress/issues/27771).
- Fixed an issue where `enter`, `keyup`, and `space` events where not triggering `click` events properly in some versions of Firefox. Addressed in [#27715](https://github.com/cypress-io/cypress/pull/27715).

**Dependency Updates:**
Expand Down
20 changes: 18 additions & 2 deletions packages/proxy/lib/http/response-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,18 @@ const stringifyFeaturePolicy = (policy: any): string => {
return pairs.map((directive) => directive.join(' ')).join('; ')
}

const requestIdRegEx = /^(.*)-retry-([\d]+)$/
const getOriginalRequestId = (requestId: string) => {
let originalRequestId = requestId
const match = requestIdRegEx.exec(requestId)

if (match) {
[, originalRequestId] = match
}

return originalRequestId
}

const LogResponse: ResponseMiddleware = function () {
this.debug('received response %o', {
browserPreRequest: _.pick(this.req.browserPreRequest, 'requestId'),
Expand Down Expand Up @@ -674,8 +686,10 @@ const ClearCyInitialCookie: ResponseMiddleware = function () {
const MaybeEndWithEmptyBody: ResponseMiddleware = function () {
if (httpUtils.responseMustHaveEmptyBody(this.req, this.incomingRes)) {
if (this.protocolManager && this.req.browserPreRequest?.requestId) {
const requestId = getOriginalRequestId(this.req.browserPreRequest.requestId)

this.protocolManager.responseEndedWithEmptyBody({
requestId: this.req.browserPreRequest.requestId,
requestId,
isCached: this.incomingRes.statusCode === 304,
})
}
Expand Down Expand Up @@ -783,9 +797,11 @@ const MaybeRemoveSecurity: ResponseMiddleware = function () {

const GzipBody: ResponseMiddleware = async function () {
if (this.protocolManager && this.req.browserPreRequest?.requestId) {
const requestId = getOriginalRequestId(this.req.browserPreRequest.requestId)

const span = telemetry.startSpan({ name: 'gzip:body:protocol-notification', parentSpan: this.resMiddlewareSpan, isVerbose })
const resultingStream = this.protocolManager.responseStreamReceived({
requestId: this.req.browserPreRequest.requestId,
requestId,
responseHeaders: this.incomingRes.headers,
isAlreadyGunzipped: this.isGunzipped,
responseStream: this.incomingResStream,
Expand Down
2 changes: 1 addition & 1 deletion packages/proxy/lib/http/util/prerequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export class PreRequests {

removePending (requestId: string) {
this.pendingPreRequests.removeMatching(({ browserPreRequest }) => {
return browserPreRequest.requestId !== requestId
return (browserPreRequest.requestId.includes('-retry-') && !browserPreRequest.requestId.startsWith(`${requestId}-`)) || (!browserPreRequest.requestId.includes('-retry-') && browserPreRequest.requestId !== requestId)
})
}

Expand Down
45 changes: 43 additions & 2 deletions packages/proxy/test/unit/http/response-middleware.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1847,14 +1847,14 @@ describe('http/response-middleware', function () {
})
})

it('calls responseEndedWithEmptyBody on protocolManager if protocolManager present and request is correlated and response must have empty body and response is not cached', function () {
it('calls responseEndedWithEmptyBody on protocolManager if protocolManager present and retried request is correlated and response must have empty body and response is not cached', function () {
prepareContext({
protocolManager: {
responseEndedWithEmptyBody: responseEndedWithEmptyBodyStub,
},
req: {
browserPreRequest: {
requestId: '123',
requestId: '123-retry-1',
},
},
incomingRes: {
Expand Down Expand Up @@ -2285,6 +2285,47 @@ describe('http/response-middleware', function () {
})
})

it('calls responseStreamReceived on protocolManager if protocolManager present and retried request is correlated', function () {
const stream = Readable.from(['foo'])
const headers = { 'content-encoding': 'gzip' }
const res = {
on: (event, listener) => {},
off: (event, listener) => {},
}

prepareContext({
protocolManager: {
responseStreamReceived: responseStreamReceivedStub,
},
req: {
browserPreRequest: {
requestId: '123-retry-1',
},
},
res,
incomingRes: {
headers,
},
isGunzipped: true,
incomingResStream: stream,
})

return testMiddleware([GzipBody], ctx)
.then(() => {
expect(responseStreamReceivedStub).to.be.calledWith(
sinon.match(function (actual) {
expect(actual.requestId).to.equal('123')
expect(actual.responseHeaders).to.equal(headers)
expect(actual.isAlreadyGunzipped).to.equal(true)
expect(actual.responseStream).to.equal(stream)
expect(actual.res).to.equal(res)

return true
}),
)
})
})

it('does not call responseStreamReceived on protocolManager if protocolManager present and request is not correlated', function () {
const stream = Readable.from(['foo'])
const headers = { 'content-encoding': 'gzip' }
Expand Down
15 changes: 15 additions & 0 deletions packages/proxy/test/unit/http/util/prerequests.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,19 @@ describe('http/util/prerequests', () => {

expectPendingCounts(0, 2)
})

it('removes a pre-request with a matching requestId with retries', () => {
preRequests.addPending({ requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1235', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1235-retry-1', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1235-retry-2', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1235-retry-3', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1236', url: 'foo', method: 'GET' } as BrowserPreRequest)

expectPendingCounts(0, 6)

preRequests.removePending('1235')

expectPendingCounts(0, 2)
})
})
6 changes: 6 additions & 0 deletions packages/server/lib/browsers/cdp_automation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,12 @@ export class CdpAutomation implements CDPClient {
}

private onResponseReceived = (params: Protocol.Network.ResponseReceivedEvent) => {
if (params.response.fromDiskCache) {
this.automation.onRequestServedFromCache?.(params.requestId)

return
}

const browserResponseReceived: BrowserResponseReceived = {
requestId: params.requestId,
status: params.response.status,
Expand Down
17 changes: 17 additions & 0 deletions packages/server/test/unit/browsers/cdp_automation_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,23 @@ context('lib/browsers/cdp_automation', () => {
},
)
})

it('cleans up prerequests when response is cached from disk', function () {
const browserResponseReceived = {
requestId: '0',
response: {
status: 200,
headers: {},
fromDiskCache: true,
},
}

this.onFn
.withArgs('Network.responseReceived')
.yield(browserResponseReceived)

expect(this.automation.onRequestEvent).not.to.have.been.called
})
})

describe('.onRequestServedFromCache', function () {
Expand Down

5 comments on commit 70248ab

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 70248ab Sep 11, 2023

Choose a reason for hiding this comment

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

Circle has built the linux x64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.2.0/linux-x64/develop-70248ab9c0bb90e0dd5987d64e2199d46f3dc600/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 70248ab Sep 11, 2023

Choose a reason for hiding this comment

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

Circle has built the linux arm64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.2.0/linux-arm64/develop-70248ab9c0bb90e0dd5987d64e2199d46f3dc600/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 70248ab Sep 11, 2023

Choose a reason for hiding this comment

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

Circle has built the darwin arm64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.2.0/darwin-arm64/develop-70248ab9c0bb90e0dd5987d64e2199d46f3dc600/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 70248ab Sep 11, 2023

Choose a reason for hiding this comment

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

Circle has built the darwin x64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.2.0/darwin-x64/develop-70248ab9c0bb90e0dd5987d64e2199d46f3dc600/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 70248ab Sep 11, 2023

Choose a reason for hiding this comment

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

Circle has built the win32 x64 version of the Test Runner.

Learn more about this pre-release build at https://on.cypress.io/advanced-installation#Install-pre-release-version

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/13.2.0/win32-x64/develop-70248ab9c0bb90e0dd5987d64e2199d46f3dc600/cypress.tgz

Please sign in to comment.