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: Error codeframes in retry failures display the code that produced the error #30962

Merged
merged 11 commits into from
Feb 4, 2025
Merged
5 changes: 3 additions & 2 deletions .circleci/workflows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ mainBuildFilters: &mainBuildFilters
- /^release\/\d+\.\d+\.\d+$/
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- 'update-v8-snapshot-cache-on-develop'
- 'chore/browser_spike'
- 'cacie/30927/retry-error-codeframe'

# 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
Expand All @@ -44,6 +44,7 @@ macWorkflowFilters: &darwin-workflow-filters
- equal: [ develop, << pipeline.git.branch >> ]
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
- equal: [ 'cacie/30927/retry-error-codeframe', << pipeline.git.branch >> ]
- equal: [ 'chore/browser_spike', << pipeline.git.branch >> ]
- matches:
pattern: /^release\/\d+\.\d+\.\d+$/
Expand Down Expand Up @@ -154,7 +155,7 @@ commands:
name: Set environment variable to determine whether or not to persist artifacts
command: |
echo "Setting SHOULD_PERSIST_ARTIFACTS variable"
echo 'if ! [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "chore/browser_spike" ]]; then
echo 'if ! [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "cacie/30927/retry-error-codeframe" ]]; then
export SHOULD_PERSIST_ARTIFACTS=true
fi' >> "$BASH_ENV"
# You must run `setup_should_persist_artifacts` command and be using bash before running this command
Expand Down
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ _Released 2/11/2025 (PENDING)_

**Bugfixes:**

- Fixed a regression introduced in [`14.0.0`](https://docs.cypress.io/guides/references/changelog#14-0-0) where error codeframes in the runner UI were not populated with the correct data in failed retry attempts. Fixes [#30927](https://github.com/cypress-io/cypress/issues/30927).
- All commands performed in `after` and `afterEach` hooks will now correctly retry when a test fails. Commands that are actions like `.click()` and `.type()` will now perform the action in this situation also. Fixes [#2831](https://github.com/cypress-io/cypress/issues/2831).
- Fixed an issue in Cypress [`14.0.0`](https://docs.cypress.io/guides/references/changelog#14-0-0) where privileged commands did not run correctly when a spec file or support file contained characters that required encoding. Fixes [#30933](https://github.com/cypress-io/cypress/issues/30933).

Expand Down
37 changes: 31 additions & 6 deletions packages/driver/src/cypress/error_utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
// See: ./errorScenarios.md for details about error messages and stack traces

// NOTE: If you modify the logic relating to this file, ensure the
// UI for error code frames works as expected with the binary. This includes each
// browser, as well as e2e and CT testing types. Stack patterns differ in Chrome
// between the binary and dev mode, so Cy in Cy tests cannot catch them proactively.

// Various stack patterns are saved as scenario fixtures in ./driver/test
// to prevent regressions.

import chai from 'chai'
import _ from 'lodash'
import $dom from '../dom'
Expand Down Expand Up @@ -136,7 +144,7 @@ const getUserInvocationStack = (err, state) => {
// command errors and command assertion errors (default assertion or cy.should)
// have the invocation stack attached to the current command
// prefer err.userInvocation stack if it's been set
let userInvocationStack = err.userInvocationStack || state('currentAssertionUserInvocationStack')
let userInvocationStack: string | undefined = err.userInvocationStack || state('currentAssertionUserInvocationStack')

// if there is no user invocation stack from an assertion or it is the default
// assertion, meaning it came from a command (e.g. cy.get), prefer the
Expand All @@ -153,14 +161,29 @@ const getUserInvocationStack = (err, state) => {
userInvocationStack = withInvocationStack?.get('userInvocationStack')
}

if (!userInvocationStack) return
if (!userInvocationStack) return undefined

// In CT with vite, the user invocation stack includes internal cypress code, so clean it up
// In some environments, additional codepoints are included in the stack prior
// to the first userland codepoint.
const internalCodepointIdentifier = [
'/__cypress/runner', // binary environments and most dev environments
'cypress:///../driver', // webpack CT with a dev build
].find((identifier) => {
return userInvocationStack?.includes(identifier)
})

// remove lines that are included _prior_ to the first userland line
userInvocationStack = $stackUtils.stackWithLinesDroppedFromMarker(userInvocationStack, '/__cypress', true)
// removes lines in the invocation stack above the first userland line. If one
// of the cypress codepoint identifiers is not present in the stack trace,
// the first line will be a userland codepoint, so no dropping is necessary.
userInvocationStack = internalCodepointIdentifier ? _.dropWhile(
userInvocationStack.split('\n'),
(stackLine) => {
return stackLine.includes(internalCodepointIdentifier)
},
).join('\n') : userInvocationStack

// remove lines that are included _after and including_ the replacement marker
// remove lines that are included _after and including_ the replacement marker -
// these are also internal to cypress, and unimportant for the user invocation stack
userInvocationStack = $stackUtils.stackPriorToReplacementMarker(userInvocationStack)

if (
Expand All @@ -170,6 +193,8 @@ const getUserInvocationStack = (err, state) => {
) {
return userInvocationStack
}

return undefined
}

const modifyErrMsg = (err, newErrMsg, cb) => {
Expand Down
9 changes: 9 additions & 0 deletions packages/driver/src/cypress/stack_utils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
// See: ./errorScenarios.md for details about error messages and stack traces

// NOTE: If you modify the logic relating to this file, ensure the
// UI for error code frames works as expected with the binary. This includes each
// browser, as well as e2e and CT testing types. Stack patterns differ in Chrome
// between the binary and dev mode, so Cy in Cy tests cannot catch them proactively.

// Various stack patterns are saved as scenario fixtures in ./driver/test
// to prevent regressions.

import _ from 'lodash'
import path from 'path'
import errorStackParser from 'error-stack-parser'
Expand Down

Large diffs are not rendered by default.

67 changes: 67 additions & 0 deletions packages/driver/test/unit/cypress/err_utils.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* @vitest-environment jsdom
*/
import { vi, describe, it, expect, beforeEach } from 'vitest'

// source_map_utils must be included in order for vite to mock it, even
// if it isn't referenced.
// eslint-disable-next-line
import source_map_utils from '../../../src/cypress/source_map_utils'
import errUtils from '../../../src/cypress/error_utils'
import stackFrameFixture from './__fixtures__/getUserInvocationStack_stackFrames.json'

vi.mock('../../../src/cypress/source_map_utils', () => {
return {
default: {
getSourcePosition: vi.fn(),
},
}
})

describe('err_utils', () => {
beforeEach(() => {
// @ts-expect-error
global.Cypress = {
config: vi.fn(),
}

vi.resetAllMocks()
})

describe('getUserInvocationStack', () => {
const { invocationFile, line, column, scenarios } = stackFrameFixture

let stack: string

class MockError {
name = 'CypressError'
get userInvocationStack () {
return stack
}
}

const state = () => undefined

for (const scenario of scenarios) {
const { browser, build, testingType, stack: scenarioStack } = scenario

describe(`${browser}:${build}:${testingType}`, () => {
beforeEach(() => {
stack = scenarioStack
})

it('returns the userInvocationStack with no leading internal cypress codeframes', () => {
const invocationStack = errUtils.getUserInvocationStack(new MockError(), state)

expect(invocationStack).not.toBeUndefined()

const [first, second] = (invocationStack as string).split('\n')

const invocationFrame = second ?? first

expect(invocationFrame).toContain(`${invocationFile}:${line}:${column}`)
})
})
}
})
})
2 changes: 1 addition & 1 deletion packages/driver/test/unit/cypress/stack_utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'

import source_map_utils from '../../../src/cypress/source_map_utils'
import stack_utils from '../../../src/cypress/stack_utils'
import stackFrameFixture from './__fixtures__/spec_stackframes.json'
import stackFrameFixture from './__fixtures__/getInvocationDetails_spec_stackframes.json'

vi.mock('../../../src/cypress/source_map_utils', () => {
return {
Expand Down
7 changes: 6 additions & 1 deletion packages/extension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,10 @@
"lib",
"theme"
],
"nx": {}
"nx": {
"implicitDependencies": [
"@packages/server",
"@packages/socket"
]
}
}
Loading