-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Fix issues with stack traces and command log in Chrome 99 #20049
Changes from 6 commits
2921c16
74e964e
ed0ed36
6b489e1
3424f90
ac6af16
d2cabd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
{ | ||
"chrome:beta": "98.0.4758.80", | ||
"chrome:beta": "99.0.4844.17", | ||
"chrome:stable": "98.0.4758.80" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,18 @@ export function create (chai) { | |
typeof object.nodeName === 'string' | ||
} | ||
|
||
// We can't just check if object instanceof ShadowRoot, because it might be the document of an iframe, | ||
// which in Chrome 99+ is a separate class, and instanceof ShadowRoot returns false. | ||
const isShadowRoot = function (object) { | ||
return isDOMElement(object.host) && object.host.shadowRoot === object | ||
} | ||
|
||
// We can't just check if object instanceof Document, because it might be the document of an iframe, | ||
// which in Chrome 99+ is a separate class, and instanceof Document returns false. | ||
const isDocument = function (object) { | ||
return object.defaultView && object.defaultView === object.defaultView.window | ||
} | ||
|
||
let formatValueHook | ||
|
||
const setFormatValueHook = (fn) => formatValueHook = fn | ||
|
@@ -124,6 +136,14 @@ export function create (chai) { | |
} | ||
} | ||
|
||
if (isShadowRoot(value)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we start recursively inspecting objects, listing out their properties, this can include elements with references to objects like Ultimately though, there's no reason we should be looking over every property of a No tests have been added, because the output of this function is already comprehensively tested - dozens of tests failed in Chrome 99 before the changes to this file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🥳 yay to improved performance! |
||
return value.innerHTML | ||
} | ||
|
||
if (isDocument(value)) { | ||
return value.documentElement.outerHTML | ||
} | ||
|
||
// Look up the keys of the object. | ||
let visibleKeys = getEnumerableProperties(value) | ||
let keys = ctx.showHidden ? getProperties(value) : visibleKeys | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,13 +139,21 @@ const captureUserInvocationStack = (ErrorConstructor, userInvocationStack?) => { | |
if (!userInvocationStack) { | ||
const newErr = new ErrorConstructor('userInvocationStack') | ||
|
||
userInvocationStack = newErr.stack | ||
|
||
// if browser natively supports Error.captureStackTrace, use it (chrome) (must be bound) | ||
// otherwise use our polyfill on top.Error | ||
const captureStackTrace = ErrorConstructor.captureStackTrace ? ErrorConstructor.captureStackTrace.bind(ErrorConstructor) : Error.captureStackTrace | ||
|
||
captureStackTrace(newErr, captureUserInvocationStack) | ||
|
||
userInvocationStack = newErr.stack | ||
// On Chrome 99+, captureStackTrace strips away the whole stack, | ||
// leaving nothing beyond the error message. If we get back a single line | ||
// (just the error message with no stack trace), then use the original value | ||
// instead of the trimmed one. | ||
if (newErr.stack.match('\n')) { | ||
userInvocationStack = newErr.stack | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not understanding this comment 100%, which one is the trimmed one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This guard is against the case - in Chrome 99 - where it was already only two lines, and gets reduced to one. Eg, it might be
and it gets trimmed down to
I'm not 100% sure what caused the changed behavior of captureStackTrace in chromium 99, but the first stack trace is far more useful than the second. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahhh the fact |
||
} | ||
} | ||
|
||
userInvocationStack = normalizedUserInvocationStack(userInvocationStack) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change with chrome has impact on the implementation of the
cy.shadow()
command?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So no tests in the shadom dom specs required changing, but there is a change in the way it's displayed. I'm updating the PR description with details, since it's user-facing.
Unintentional, but I think the new version is much cleaner, so looks like a feature rather than a bug to me!