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

chore(webkit): update error stack parsing and related system tests #23730

Merged
merged 34 commits into from
Sep 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
2342f90
chore(webkit): update error stack parsing and related system tests
tbiethman Sep 6, 2022
d7805ea
Adding better comment
tbiethman Sep 7, 2022
54e3f4f
Putting column back. Indexing at 1.
tbiethman Sep 7, 2022
bea9487
Let's wait for WebKit fixes to land for this
tbiethman Sep 7, 2022
07dc2dd
Using default name/location values already used in stack enhancing logic
tbiethman Sep 7, 2022
f82d422
Incorrect bracket in regex
tbiethman Sep 7, 2022
5eb7780
Trying without location, as the fake location causes more problems do…
tbiethman Sep 7, 2022
9b2ddad
Loosening regex around locations in stack replacement
tbiethman Sep 7, 2022
9fb8c8e
Defaulting location sans row/column
tbiethman Sep 7, 2022
662ee39
Parsing stack lines for reporter with unique regex
tbiethman Sep 7, 2022
ab0f991
D'oh
tbiethman Sep 7, 2022
a04e6e3
Making the validation against '<unknown>' more specific
tbiethman Sep 7, 2022
2204d3b
Don't want a capture group here
tbiethman Sep 7, 2022
e7f7cac
Updating spec_isolation system tests
tbiethman Sep 8, 2022
2d72ee2
Consolidating regex pattern to errors package
tbiethman Sep 8, 2022
b413949
Can just keep this global now
tbiethman Sep 8, 2022
d35f786
Simplifying regex. Removing lineAndColumn numbers from unknown locati…
tbiethman Sep 8, 2022
226d2d7
Merge branch 'develop' into tbiethman/webkit-stacks-pr
tbiethman Sep 8, 2022
fb5c972
Merge branch 'develop' into tbiethman/webkit-stacks-pr
tbiethman Sep 9, 2022
2b91eff
Updating system test stack regex
tbiethman Sep 11, 2022
6f9c64d
Getting better baseline
tbiethman Sep 11, 2022
17d2709
Revert "Updating system test stack regex"
tbiethman Sep 11, 2022
97de367
Forking normalization for webkit to track down diffs
tbiethman Sep 12, 2022
aa79132
Ensure line or column are set before rendering in enhanced stack
tbiethman Sep 12, 2022
370459b
Need to be a little more flexible
tbiethman Sep 12, 2022
55ca6ef
Tweaking leading newline detection
tbiethman Sep 12, 2022
676c88e
Trying out new composed regex
tbiethman Sep 12, 2022
6c8d545
Few more tweaks for multiple leading newlines and file locations with…
tbiethman Sep 12, 2022
e542cce
Updating remainderOfStack pattern with proper escaping
tbiethman Sep 12, 2022
393f85a
Cleaning up comments
tbiethman Sep 12, 2022
e014f9e
Merge branch 'develop' into tbiethman/webkit-stacks-pr
tbiethman Sep 12, 2022
5aed77f
Filtering native code from absolute path logic
tbiethman Sep 12, 2022
1ed28f9
Merge branch 'develop' into tbiethman/webkit-stacks-pr
tbiethman Sep 12, 2022
928ca9d
Rebuild CI after outage
tbiethman Sep 12, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 44 additions & 8 deletions packages/driver/src/cypress/stack_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@ import $utils from './utils'
import $sourceMapUtils from './source_map_utils'

// Intentionally deep-importing from @packages/errors so as to not bundle the entire @packages/errors in the client unnecessarily
import { getStackLines, replacedStack, stackWithoutMessage, splitStack, unsplitStack } from '@packages/errors/src/stackUtils'
import { getStackLines, replacedStack, stackWithoutMessage, splitStack, unsplitStack, stackLineRegex } from '@packages/errors/src/stackUtils'

const whitespaceRegex = /^(\s*)*/
const stackLineRegex = /^\s*(at )?.*@?\(?.*\:\d+\:\d+\)?$/
const customProtocolRegex = /^[^:\/]+:\/{1,3}/
const percentNotEncodedRegex = /%(?![0-9A-F][0-9A-F])/g
const webkitStackLineRegex = /(.*)@(.*)(\n?)/g

const STACK_REPLACEMENT_MARKER = '__stackReplacementMarker'

const hasCrossFrameStacks = (specWindow) => {
Expand Down Expand Up @@ -244,9 +245,7 @@ const cleanFunctionName = (functionName) => {
}

const parseLine = (line) => {
const isStackLine = stackLineRegex.test(line)

if (!isStackLine) return
if (!stackLineRegex.test(line)) return

const parsed = errorStackParser.parse({ stack: line } as any)[0]

Expand Down Expand Up @@ -318,7 +317,14 @@ const getSourceDetailsForLine = (projectRoot, line): LineDetail => {

let absoluteFile

if (relativeFile && projectRoot) {
// WebKit stacks may include an `<unknown>` or `[native code]` location that is not navigable.
// We ensure that the absolute path is not set in this case.

const canBuildAbsolutePath = relativeFile && projectRoot && (
!Cypress.isBrowser('webkit') || (relativeFile !== '<unknown>' && relativeFile !== '[native code]')
)

if (canBuildAbsolutePath) {
absoluteFile = path.resolve(projectRoot, relativeFile)

// rollup-plugin-node-builtins/src/es6/path.js only support POSIX, we have
Expand Down Expand Up @@ -356,7 +362,9 @@ const reconstructStack = (parsedStack) => {

const { whitespace, originalFile, function: fn, line, column } = parsedLine

return `${whitespace}at ${fn} (${originalFile || '<unknown>'}:${line}:${column})`
const lineAndColumn = (Number.isInteger(line) || Number.isInteger(column)) ? `:${line}:${column}` : ''

return `${whitespace}at ${fn} (${originalFile || '<unknown>'}${lineAndColumn})`
}).join('\n').trimEnd()
}

Expand Down Expand Up @@ -392,7 +400,35 @@ const normalizedStack = (err) => {
// Chromium-based errors do, so we normalize them so that the stack
// always includes the name/message
const errString = err.toString()
const errStack = err.stack || ''
let errStack = err.stack || ''

if (Cypress.isBrowser('webkit')) {
// WebKit will not determine the proper stack trace for an error, with stack entries
// missing function names, call locations, or both. This is due to a number of documented
// issues with WebKit:
// https://bugs.webkit.org/show_bug.cgi?id=86493
// https://bugs.webkit.org/show_bug.cgi?id=243668
// https://bugs.webkit.org/show_bug.cgi?id=174380
//
// We update these stack entries with placeholder names/locations to more closely align
// the output with other browsers, minimizing the visual impact to the stack traces we render
// within the command log and console and ensuring that the stacks can be identified within
// and parsed out of test snapshots that include them.
errStack = errStack.replaceAll(webkitStackLineRegex, (match, ...parts: string[]) => {
// We patch WebKit's Error within the AUT as CyWebKitError, causing it to
// be presented within the stack. If we detect it within the stack, we remove it.
if (parts[0] === '__CyWebKitError') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because it's showing up in the stack trace of every new Error? Should we just not name the function then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on this. If we don't name it, it'll still render here, just as yet another anonymous '' function. Given the frequency with which it'll be presented (for any uncaught exception thrown by the AUT), I figured its presence in the stacks might be confusing, named or not. But I could understand the argument that removing it is an improper obfuscation of what is actually being executed?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, yeah... maybe we just oughtta name it Error, then. This seems fine too. Not too big an issue for experimental.

return ''
}

return [
parts[0] || '<unknown>',
'@',
parts[1] || '<unknown>',
parts[2],
].join('')
})
}

// the stack has already been normalized and normalizing the indentation
// again could mess up the whitespace
Expand Down
2 changes: 1 addition & 1 deletion packages/errors/src/stackUtils.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import _ from 'lodash'
import type { ErrorLike } from './errorTypes'

const stackLineRegex = /^\s*(at )?.*@?\(?.*\:\d+\:\d+\)?$/
export const stackLineRegex = /^\s*(at )?.*@?(?:\(?.*(?::\d+:\d+|<unknown>|\[native code\])+\)?)$/

type MessageLines = [string[], string[]] & {messageEnded?: boolean}

Expand Down
4 changes: 3 additions & 1 deletion packages/reporter/src/errors/error-stack.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ const ErrorStack = observer(({ err }: Props) => {
)

if (dontLink) {
return makeLine(key, [whitespace, `at ${fn} (${originalFile}:${line}:${column})`])
const lineAndColumn = (Number.isInteger(line) || Number.isInteger(column)) ? `:${line}:${column}` : ''

return makeLine(key, [whitespace, `at ${fn} (${originalFile}${lineAndColumn})`])
}

const link = (
Expand Down
Loading