From b58e6de3d12c2ae841a429036a9fac2a4e32151e Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Thu, 2 Feb 2023 14:38:05 -0600 Subject: [PATCH 01/13] fix: Prevent request from being re-queued in event of total middleware failure --- packages/proxy/lib/http/index.ts | 18 +++++- .../test/unit/http/request-middleware.spec.ts | 63 +++++++++++++++++++ packages/server/lib/server-base.ts | 10 ++- packages/server/lib/server-e2e.ts | 2 +- 4 files changed, 87 insertions(+), 6 deletions(-) diff --git a/packages/proxy/lib/http/index.ts b/packages/proxy/lib/http/index.ts index 67fc7629b15..eb0dc898a32 100644 --- a/packages/proxy/lib/http/index.ts +++ b/packages/proxy/lib/http/index.ts @@ -70,7 +70,7 @@ export const defaultMiddleware = { export type ServerCtx = Readonly<{ config: CyServer.Config & Cypress.Config shouldCorrelatePreRequests?: () => boolean - getFileServerToken: () => string + getFileServerToken: () => string | undefined getCookieJar: () => CookieJar remoteStates: RemoteStates requestedWithAndCredentialManager: RequestedWithAndCredentialManager @@ -182,7 +182,11 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) { const fullCtx = { next: () => { fullCtx.next = () => { - throw new Error('Error running proxy middleware: Cannot call this.next() more than once in the same middleware function. Doing so can cause unintended issues.') + if (type === HttpStages.Error) { + ctx.debug('`this.next` called more than once within the same middleware function but was ignored due to being in an Error flow. %o', { middlewareName, error: ctx.error }) + } else { + throw new Error(`Error running proxy middleware: Cannot call this.next() more than once in the same middleware function. Doing so can cause unintended issues.`) + } } copyChangedCtx() @@ -207,6 +211,14 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) { try { middleware.call(fullCtx) } catch (err) { + // TODO Find better way to log this + // eslint-disable-next-line no-console + console.error(err) + + // This is a total failure so we will not be retrying the request, clear it before executing handler + // If we do not do this we will end up attempting to double-execute the middleware `next` method + // https://github.com/cypress-io/cypress/issues/22825 + fullCtx.req.browserPreRequest = null fullCtx.onError(err) } }) @@ -230,7 +242,7 @@ export class Http { config: CyServer.Config shouldCorrelatePreRequests: () => boolean deferredSourceMapCache: DeferredSourceMapCache - getFileServerToken: () => string + getFileServerToken: () => string | undefined remoteStates: RemoteStates middleware: HttpMiddlewareStacks netStubbingState: NetStubbingState diff --git a/packages/proxy/test/unit/http/request-middleware.spec.ts b/packages/proxy/test/unit/http/request-middleware.spec.ts index 396afb22b54..58e3920f89a 100644 --- a/packages/proxy/test/unit/http/request-middleware.spec.ts +++ b/packages/proxy/test/unit/http/request-middleware.spec.ts @@ -770,4 +770,67 @@ describe('http/request-middleware', () => { }) }) }) + + describe('SendRequestOutgoing', () => { + const { SendRequestOutgoing } = RequestMiddleware + + let ctx + + beforeEach(() => { + const headers = {} + const remoteStates = new RemoteStates(() => {}) + + ctx = { + onError: sinon.stub(), + request: { + create: (opts) => { + return { + inputArgs: opts, + on: (event, callback) => { + if (event === 'response') { + callback() + } + }, + } + }, + }, + req: { + body: '{}', + headers, + socket: { + on: () => {}, + }, + }, + res: {} as Partial, + remoteStates, + } + }) + + context('same-origin file request', () => { + beforeEach(() => { + ctx.getFileServerToken = () => 'abcd1234' + ctx.req.proxiedUrl = 'https://www.cypress.io/file' + ctx.remoteStates.set({ + origin: 'https://www.cypress.io', + strategy: 'file', + } as any) + }) + + it('adds `x-cypress-authorization` header', async () => { + await testMiddleware([SendRequestOutgoing], ctx) + .then(() => { + expect(ctx.req.headers['x-cypress-authorization']).to.equal('abcd1234') + }) + }) + + it('handles nil fileServer token', async () => { + ctx.getFileServerToken = () => undefined + + await testMiddleware([SendRequestOutgoing], ctx) + .then(() => { + expect(ctx.req.headers['x-cypress-authorization']).to.be.undefined + }) + }) + }) + }) }) diff --git a/packages/server/lib/server-base.ts b/packages/server/lib/server-base.ts index 0cb818589c9..66bb346e01b 100644 --- a/packages/server/lib/server-base.ts +++ b/packages/server/lib/server-base.ts @@ -95,6 +95,12 @@ const notSSE = (req, res) => { export type WarningErr = Record +export type FileServer = { + token: string + port: () => number + close: () => void +} + export interface OpenServerOptions { SocketCtor: typeof SocketE2E | typeof SocketCt testingType: Cypress.TestingType @@ -112,7 +118,7 @@ export abstract class ServerBase { protected isListening: boolean protected socketAllowed: SocketAllowed protected requestedWithAndCredentialManager: RequestedWithAndCredentialManager - protected _fileServer + protected _fileServer: FileServer | null protected _baseUrl: string | null protected _server?: DestroyableHttpServer protected _socket?: TSocket @@ -320,7 +326,7 @@ export abstract class ServerBase { createNetworkProxy ({ config, remoteStates, requestedWithAndCredentialManager, shouldCorrelatePreRequests }) { const getFileServerToken = () => { - return this._fileServer.token + return this._fileServer?.token } this._netStubbingState = netStubbingState() diff --git a/packages/server/lib/server-e2e.ts b/packages/server/lib/server-e2e.ts index 7e83a5d79fd..db37afb6a4e 100644 --- a/packages/server/lib/server-e2e.ts +++ b/packages/server/lib/server-e2e.ts @@ -221,7 +221,7 @@ export class ServerE2E extends ServerBase { if (!fullyQualifiedRe.test(urlStr)) { handlingLocalFile = true - options.headers['x-cypress-authorization'] = this._fileServer.token + options.headers['x-cypress-authorization'] = this._fileServer?.token const state = this._remoteStates.set(urlStr, options) From 329f03adbbd873906faaaf14d6e2e0809545ea1e Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Fri, 3 Feb 2023 10:25:33 -0600 Subject: [PATCH 02/13] Improve error logging --- packages/proxy/lib/http/index.ts | 16 ++-- .../unit/http/response-middleware.spec.ts | 86 ++++++++++++------- 2 files changed, 64 insertions(+), 38 deletions(-) diff --git a/packages/proxy/lib/http/index.ts b/packages/proxy/lib/http/index.ts index eb0dc898a32..1614a992590 100644 --- a/packages/proxy/lib/http/index.ts +++ b/packages/proxy/lib/http/index.ts @@ -23,6 +23,7 @@ import type { RemoteStates } from '@packages/server/lib/remote_states' import type { CookieJar } from '@packages/server/lib/util/cookies' import type { RequestedWithAndCredentialManager } from '@packages/server/lib/util/requestedWithAndCredentialManager' import type { AutomationCookie } from '@packages/server/lib/automation/cookies' +import { errorUtils } from '@packages/errors' function getRandomColorFn () { return chalk.hex(`#${Number( @@ -182,11 +183,13 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) { const fullCtx = { next: () => { fullCtx.next = () => { - if (type === HttpStages.Error) { - ctx.debug('`this.next` called more than once within the same middleware function but was ignored due to being in an Error flow. %o', { middlewareName, error: ctx.error }) - } else { - throw new Error(`Error running proxy middleware: Cannot call this.next() more than once in the same middleware function. Doing so can cause unintended issues.`) + const error = new Error('Error running proxy middleware: Detected `this.next()` was called more than once in the same middleware function. This can cause unintended issues and may indicate an error in your middleware logic or configuration.') + + if (ctx.error) { + error.message = error.message += `\nThis middleware invocation previously encountered an error which may be related: ${ctx.error}` } + + throw error } copyChangedCtx() @@ -211,10 +214,7 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) { try { middleware.call(fullCtx) } catch (err) { - // TODO Find better way to log this - // eslint-disable-next-line no-console - console.error(err) - + errorUtils.logError(err) // This is a total failure so we will not be retrying the request, clear it before executing handler // If we do not do this we will end up attempting to double-execute the middleware `next` method // https://github.com/cypress-io/cypress/issues/22825 diff --git a/packages/proxy/test/unit/http/response-middleware.spec.ts b/packages/proxy/test/unit/http/response-middleware.spec.ts index 0e40bd65bb4..41821fa2cdd 100644 --- a/packages/proxy/test/unit/http/response-middleware.spec.ts +++ b/packages/proxy/test/unit/http/response-middleware.spec.ts @@ -31,41 +31,67 @@ describe('http/response-middleware', function () { ]) }) - it('errors if this.next() is called more than once in the same middleware', function (done) { - const middleware = function () { - this.next() - this.next() - } + describe('multiple this.next invocations', () => { + context('within the same middleware', () => { + it('throws an error', function (done) { + const middleware = function () { + this.next() + this.next() + } + + testMiddleware([middleware], { + res: { + on: (event, listener) => {}, + off: (event, listener) => {}, + }, + onError (err) { + expect(err.message).to.equal('Error running proxy middleware: Detected `this.next()` was called more than once in the same middleware function. This can cause unintended issues and may indicate an error in your middleware logic or configuration.') - testMiddleware([middleware], { - res: { - on: (event, listener) => {}, - off: (event, listener) => {}, - }, - onError (err) { - expect(err.message).to.equal('Error running proxy middleware: Cannot call this.next() more than once in the same middleware function. Doing so can cause unintended issues.') + done() + }, + }) + }) - done() - }, + it('includes a previous context error in error message if one exists', (done) => { + const middleware = function () { + this.next() + this.next() + } + + testMiddleware([middleware], { + error: new Error('previous error'), + res: { + on: (event, listener) => {}, + off: (event, listener) => {}, + }, + onError (err) { + expect(err.message).to.contain('This middleware invocation previously encountered an error which may be related: Error: previous error') + + done() + }, + }) + }) }) - }) - it('does not error if this.next() is called more than once in different middleware', function () { - const middleware1 = function () { - this.next() - } - const middleware2 = function () { - this.next() - } + context('across different middleware', () => { + it('does not throw an error', function () { + const middleware1 = function () { + this.next() + } + const middleware2 = function () { + this.next() + } - return testMiddleware([middleware1, middleware2], { - res: { - on: (event, listener) => {}, - off: (event, listener) => {}, - }, - onError () { - throw new Error('onError should not be called') - }, + return testMiddleware([middleware1, middleware2], { + res: { + on: (event, listener) => {}, + off: (event, listener) => {}, + }, + onError () { + throw new Error('onError should not be called') + }, + }) + }) }) }) From 520c329b3f0fa6a388e826424355c102dc49959d Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Mon, 6 Feb 2023 08:02:36 -0600 Subject: [PATCH 03/13] run ci From bd3886771e0ff71b833c7a4175562ef5abe0f4f1 Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Mon, 6 Feb 2023 08:29:54 -0600 Subject: [PATCH 04/13] Add changelog entry --- cli/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 3556a27bb82..a4ec6c95660 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -3,6 +3,10 @@ _Released 02/14/2023 (PENDING)_ +**Bugfixes:** + +- Fixed an issue in middleware where error-handling code could itself generate an error and fail to report the original issue. Fixes [#22825](https://github.com/cypress-io/cypress/issues/22825). + **Misc:** - Improved the layout of the Debug Page on smaller viewports when there is a pending run. Addresses [#25664](https://github.com/cypress-io/cypress/issues/25664). From 459d6b47239248d9b101f75b57b1da751d6ebd5e Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Mon, 6 Feb 2023 09:24:09 -0600 Subject: [PATCH 05/13] Generate pre-release build --- .circleci/workflows.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml index 15701f41fac..6947eb3e6b9 100644 --- a/.circleci/workflows.yml +++ b/.circleci/workflows.yml @@ -28,7 +28,7 @@ mainBuildFilters: &mainBuildFilters only: - develop - /^release\/\d+\.\d+\.\d+$/ - - 'emily/next-version' + - 'mikep/22825-next-middleware' # usually we don't build Mac app - it takes a long time # but sometimes we want to really confirm we are doing the right thing @@ -37,7 +37,7 @@ macWorkflowFilters: &darwin-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ] + - equal: [ 'mikep/22825-next-middleware', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -46,7 +46,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ] + - equal: [ 'mikep/22825-next-middleware', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -64,7 +64,7 @@ windowsWorkflowFilters: &windows-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ] + - equal: [ 'mikep/22825-next-middleware', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -130,7 +130,7 @@ commands: - run: name: Check current branch to persist artifacts command: | - if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "emily/next-version" ]]; then + if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "mikep/22825-next-middleware" ]]; then echo "Not uploading artifacts or posting install comment for this branch." circleci-agent step halt fi From bb729e770d892f20c373d845f805353705766334 Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Mon, 6 Feb 2023 09:24:23 -0600 Subject: [PATCH 06/13] run ci From 1fa8ca5a2db6d88a2df9f6108f78aab89697f961 Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Mon, 6 Feb 2023 10:51:52 -0600 Subject: [PATCH 07/13] Fix test, remove unnecessary cleanup action --- .circleci/workflows.yml | 2 +- packages/proxy/lib/http/index.ts | 4 ---- packages/proxy/test/unit/http/request-middleware.spec.ts | 5 ++++- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml index 6947eb3e6b9..5627bc82e55 100644 --- a/.circleci/workflows.yml +++ b/.circleci/workflows.yml @@ -64,7 +64,7 @@ windowsWorkflowFilters: &windows-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'mikep/22825-next-middleware', << pipeline.git.branch >> ] + - equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> diff --git a/packages/proxy/lib/http/index.ts b/packages/proxy/lib/http/index.ts index 1614a992590..d2512cdec80 100644 --- a/packages/proxy/lib/http/index.ts +++ b/packages/proxy/lib/http/index.ts @@ -215,10 +215,6 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) { middleware.call(fullCtx) } catch (err) { errorUtils.logError(err) - // This is a total failure so we will not be retrying the request, clear it before executing handler - // If we do not do this we will end up attempting to double-execute the middleware `next` method - // https://github.com/cypress-io/cypress/issues/22825 - fullCtx.req.browserPreRequest = null fullCtx.onError(err) } }) diff --git a/packages/proxy/test/unit/http/request-middleware.spec.ts b/packages/proxy/test/unit/http/request-middleware.spec.ts index 58e3920f89a..58995e35c75 100644 --- a/packages/proxy/test/unit/http/request-middleware.spec.ts +++ b/packages/proxy/test/unit/http/request-middleware.spec.ts @@ -801,7 +801,10 @@ describe('http/request-middleware', () => { on: () => {}, }, }, - res: {} as Partial, + res: { + on: (event, listener) => {}, + off: (event, listener) => {}, + } as Partial, remoteStates, } }) From 5632e1feaca27610d94d87c344d4dbba7c1737c9 Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Mon, 6 Feb 2023 11:36:06 -0600 Subject: [PATCH 08/13] Fix changelog --- cli/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index bd22b2d19dc..987cd829049 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -8,6 +8,7 @@ _Released 02/14/2023 (PENDING)_ - Added the "Open in IDE" feature for failed tests reported from the Debug page. Addressed in [#25691](https://github.com/cypress-io/cypress/pull/25691). **Bugfixes:** + - Fixed an issue in middleware where error-handling code could itself generate an error and fail to report the original issue. Fixes [#22825](https://github.com/cypress-io/cypress/issues/22825). **Misc:** From cc15163d33677b4a1888650dd4f14a1323453edf Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Tue, 7 Feb 2023 08:19:11 -0600 Subject: [PATCH 09/13] Address review comments * Append root error as `cause` on `this.next` error * Improve error messages * Remove unneeded type export --- packages/proxy/lib/http/index.ts | 5 +++-- .../proxy/test/unit/http/response-middleware.spec.ts | 9 +++++---- packages/server/lib/server-base.ts | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/proxy/lib/http/index.ts b/packages/proxy/lib/http/index.ts index d2512cdec80..86004f34788 100644 --- a/packages/proxy/lib/http/index.ts +++ b/packages/proxy/lib/http/index.ts @@ -183,10 +183,11 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) { const fullCtx = { next: () => { fullCtx.next = () => { - const error = new Error('Error running proxy middleware: Detected `this.next()` was called more than once in the same middleware function. This can cause unintended issues and may indicate an error in your middleware logic or configuration.') + const error = new Error('Error running proxy middleware: Detected `this.next()` was called more than once in the same middleware function, but a middleware can only be completed once.') if (ctx.error) { - error.message = error.message += `\nThis middleware invocation previously encountered an error which may be related: ${ctx.error}` + error.message = error.message += '\nThis middleware invocation previously encountered an error which may be related, see `error.cause`' + error['cause'] = ctx.error } throw error diff --git a/packages/proxy/test/unit/http/response-middleware.spec.ts b/packages/proxy/test/unit/http/response-middleware.spec.ts index 41821fa2cdd..c5a57421689 100644 --- a/packages/proxy/test/unit/http/response-middleware.spec.ts +++ b/packages/proxy/test/unit/http/response-middleware.spec.ts @@ -45,7 +45,7 @@ describe('http/response-middleware', function () { off: (event, listener) => {}, }, onError (err) { - expect(err.message).to.equal('Error running proxy middleware: Detected `this.next()` was called more than once in the same middleware function. This can cause unintended issues and may indicate an error in your middleware logic or configuration.') + expect(err.message).to.equal('Error running proxy middleware: Detected `this.next()` was called more than once in the same middleware function, but a middleware can only be completed once.') done() }, @@ -57,16 +57,17 @@ describe('http/response-middleware', function () { this.next() this.next() } + const error = new Error('previous error') testMiddleware([middleware], { - error: new Error('previous error'), + error, res: { on: (event, listener) => {}, off: (event, listener) => {}, }, onError (err) { - expect(err.message).to.contain('This middleware invocation previously encountered an error which may be related: Error: previous error') - + expect(err.message).to.contain('This middleware invocation previously encountered an error which may be related, see `error.cause`') + expect(err['cause']).to.equal(error) done() }, }) diff --git a/packages/server/lib/server-base.ts b/packages/server/lib/server-base.ts index 66bb346e01b..89d37c01174 100644 --- a/packages/server/lib/server-base.ts +++ b/packages/server/lib/server-base.ts @@ -95,7 +95,7 @@ const notSSE = (req, res) => { export type WarningErr = Record -export type FileServer = { +type FileServer = { token: string port: () => number close: () => void From ace3763ff9e0c7db645d6c0ed93d885bbc4312e8 Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Tue, 7 Feb 2023 13:50:12 -0600 Subject: [PATCH 10/13] Address review comments * Improve logged error message to provide request context * Log `cause` if one is present * Add tests --- packages/errors/src/errorUtils.ts | 9 +++- packages/errors/test/unit/errors_spec.ts | 63 +++++++++++++++++++++--- packages/proxy/lib/http/index.ts | 1 + 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/packages/errors/src/errorUtils.ts b/packages/errors/src/errorUtils.ts index 501249803ee..f54e7b9ae51 100644 --- a/packages/errors/src/errorUtils.ts +++ b/packages/errors/src/errorUtils.ts @@ -50,9 +50,10 @@ type AllowedChalkColors = 'red' | 'blue' | 'green' | 'magenta' | 'yellow' * * @param err * @param color + * @param causeDepth If error has a `cause` limits the maximum depth of causes to log. Set to `0` to not log any `cause` * @returns */ -export const logError = function (err: CypressError | ErrorLike, color: AllowedChalkColors = 'red') { +export const logError = function (err: CypressError | ErrorLike, color: AllowedChalkColors = 'red', causeDepth: number = 3) { console.log(chalk[color](err.message)) if (err.details) { @@ -67,5 +68,11 @@ export const logError = function (err: CypressError | ErrorLike, color: AllowedC console.log(chalk[color](err.stack ?? '')) + if (causeDepth > 0 && err['cause']) { + // Limit the recursions on `cause` in case there is a loop + console.log(chalk[color]('Caused by:')) + logError(err['cause'], color, causeDepth - 1) + } + return err } diff --git a/packages/errors/test/unit/errors_spec.ts b/packages/errors/test/unit/errors_spec.ts index 61f15f6f5ef..609464bebed 100644 --- a/packages/errors/test/unit/errors_spec.ts +++ b/packages/errors/test/unit/errors_spec.ts @@ -3,7 +3,7 @@ import style from 'ansi-styles' import chai, { expect } from 'chai' /* eslint-disable no-console */ import chalk from 'chalk' -import sinon from 'sinon' +import sinon, { SinonSpy } from 'sinon' import * as errors from '../../src' import { parseResolvedPattern } from '../../src/errorUtils' @@ -72,16 +72,65 @@ describe('lib/errors', () => { expect(console.log).to.be.calledWithMatch(chalk.magenta(userError.stack ?? '')) }) - it('logs err.stack in development', () => { - process.env.CYPRESS_INTERNAL_ENV = 'development' + describe('err.stack', () => { + it('is logged if not a known Cypress error', () => { + const err = new Error('foo') - const err = new Error('foo') + const ret = errors.log(err) - const ret = errors.log(err) + expect(ret).to.eq(err) + + expect(console.log).to.be.calledWith(chalk.red(err.stack ?? '')) + }) + + it('is not logged if a known Cypress error', () => { + const err = new Error('foo') + + err['isCypressErr'] = true + + const ret = errors.log(err) + + expect(ret).to.be.undefined + + expect(console.log).not.to.be.calledWith(chalk.red(err.stack ?? '')) + }) + }) + + context('err.cause', () => { + let err + + beforeEach(() => { + err = new Error('foo') + err['cause'] = err + }) + + it('is not logged if a known Cypress error', () => { + err['isCypressErr'] = true + + const ret = errors.log(err) + + expect(ret).to.be.undefined + + expect(console.log).not.to.be.calledWith(chalk.red('Caused by:')) + }) + + it('is not logged if max cause depth === 0', () => { + const ret = errors.log(err, 'red', 0) + + expect(ret).to.eq(ret) + + expect(console.log).not.to.be.calledWith(chalk.red('Caused by:')) + }) + + it('is logged to a specified max depth', () => { + const ret = errors.log(err, 'red', 5) + + expect(ret).to.eq(err) - expect(ret).to.eq(err) + const causeLogs = (console.log as SinonSpy).getCalls().filter((call) => call.args[0] === chalk.red('Caused by:')) - expect(console.log).to.be.calledWith(chalk.red(err.stack ?? '')) + expect(causeLogs).to.have.length(5) + }) }) }) diff --git a/packages/proxy/lib/http/index.ts b/packages/proxy/lib/http/index.ts index 86004f34788..87448976c8f 100644 --- a/packages/proxy/lib/http/index.ts +++ b/packages/proxy/lib/http/index.ts @@ -215,6 +215,7 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) { try { middleware.call(fullCtx) } catch (err) { + err.message = `Internal error while proxying "${ctx.req.method} ${ctx.req.proxiedUrl}" in ${middlewareName}:\n${err.message}` errorUtils.logError(err) fullCtx.onError(err) } From 187b130b65154747f3f916037ffe166534711c13 Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Tue, 7 Feb 2023 13:51:50 -0600 Subject: [PATCH 11/13] Fix changelog --- cli/CHANGELOG.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index f9dfb9cdd70..871400c8dc0 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -10,9 +10,6 @@ _Released 02/14/2023 (PENDING)_ **Features:** - Added the "Open in IDE" feature for failed tests reported from the Debug page. Addressed in [#25691](https://github.com/cypress-io/cypress/pull/25691). - -**Bugfixes:** - - Fixed an issue in middleware where error-handling code could itself generate an error and fail to report the original issue. Fixes [#22825](https://github.com/cypress-io/cypress/issues/22825). **Misc:** From 2303f1ea6c9d7fc18f098362fb9496593517ca77 Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Tue, 7 Feb 2023 13:53:32 -0600 Subject: [PATCH 12/13] Fix changelog --- cli/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 871400c8dc0..992a8b2994d 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -5,12 +5,12 @@ _Released 02/14/2023 (PENDING)_ **Bugfixes:** - - Fixed an issue with the Cloud project selection modal not showing the correct prompts. Fixes [#25520](https://github.com/cypress-io/cypress/issues/25520). +- Fixed an issue with the Cloud project selection modal not showing the correct prompts. Fixes [#25520](https://github.com/cypress-io/cypress/issues/25520). +- Fixed an issue in middleware where error-handling code could itself generate an error and fail to report the original issue. Fixes [#22825](https://github.com/cypress-io/cypress/issues/22825). **Features:** - Added the "Open in IDE" feature for failed tests reported from the Debug page. Addressed in [#25691](https://github.com/cypress-io/cypress/pull/25691). -- Fixed an issue in middleware where error-handling code could itself generate an error and fail to report the original issue. Fixes [#22825](https://github.com/cypress-io/cypress/issues/22825). **Misc:** From fbd5d515904ffd8380e91f4b260b25dd80c76695 Mon Sep 17 00:00:00 2001 From: Mike Plummer Date: Tue, 7 Feb 2023 14:56:27 -0600 Subject: [PATCH 13/13] Fix unit tests --- packages/proxy/test/unit/http/index.spec.ts | 12 ++++++------ .../proxy/test/unit/http/response-middleware.spec.ts | 4 +++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/proxy/test/unit/http/index.spec.ts b/packages/proxy/test/unit/http/index.spec.ts index a18aec6f2fc..2d394857e48 100644 --- a/packages/proxy/test/unit/http/index.spec.ts +++ b/packages/proxy/test/unit/http/index.spec.ts @@ -63,13 +63,13 @@ describe('http', function () { incomingRequest.throws(new Error('oops')) error.callsFake(function () { - expect(this.error.message).to.eq('oops') + expect(this.error.message).to.eq('Internal error while proxying "GET url" in 0:\noops') this.end() }) return new Http(httpOpts) // @ts-ignore - .handle({}, { on, off }) + .handle({ method: 'GET', proxiedUrl: 'url' }, { on, off }) .then(function () { expect(incomingRequest).to.be.calledOnce expect(incomingResponse).to.not.be.called @@ -88,13 +88,13 @@ describe('http', function () { incomingResponse.throws(new Error('oops')) error.callsFake(function () { - expect(this.error.message).to.eq('oops') + expect(this.error.message).to.eq('Internal error while proxying "GET url" in 0:\noops') this.end() }) return new Http(httpOpts) // @ts-ignore - .handle({}, { on, off }) + .handle({ method: 'GET', proxiedUrl: 'url' }, { on, off }) .then(function () { expect(incomingRequest).to.be.calledOnce expect(incomingResponse).to.be.calledOnce @@ -141,7 +141,7 @@ describe('http', function () { }) error.callsFake(function () { - expect(this.error.message).to.eq('goto error stack') + expect(this.error.message).to.eq('Internal error while proxying "GET url" in 1:\ngoto error stack') expect(this).to.include.keys(expectedKeys) this.errorAdded = errorAdded this.next() @@ -159,7 +159,7 @@ describe('http', function () { return new Http(httpOpts) // @ts-ignore - .handle({}, { on, off }) + .handle({ method: 'GET', proxiedUrl: 'url' }, { on, off }) .then(function () { [ incomingRequest, incomingRequest2, diff --git a/packages/proxy/test/unit/http/response-middleware.spec.ts b/packages/proxy/test/unit/http/response-middleware.spec.ts index c5a57421689..efed881d6d5 100644 --- a/packages/proxy/test/unit/http/response-middleware.spec.ts +++ b/packages/proxy/test/unit/http/response-middleware.spec.ts @@ -45,7 +45,7 @@ describe('http/response-middleware', function () { off: (event, listener) => {}, }, onError (err) { - expect(err.message).to.equal('Error running proxy middleware: Detected `this.next()` was called more than once in the same middleware function, but a middleware can only be completed once.') + expect(err.message).to.equal('Internal error while proxying "undefined undefined" in 0:\nError running proxy middleware: Detected `this.next()` was called more than once in the same middleware function, but a middleware can only be completed once.') done() }, @@ -70,6 +70,8 @@ describe('http/response-middleware', function () { expect(err['cause']).to.equal(error) done() }, + method: 'GET', + proxiedUrl: 'url', }) }) })