-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: enhance stacktraces #916
Conversation
src/browser/stacktrace.ts
Outdated
|
||
type AnyFunc = (...args: any[]) => unknown; // eslint-disable-line @typescript-eslint/no-explicit-any | ||
|
||
export const runWithStacktraceHooks = ({ |
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.
The only function, which is different with history/index.js
. The rest is taken from history/index.js
with removing shouldNotWrapCommand
src/utils/stacktrace.ts
Outdated
const getErrorTitle = (e: Error): string => `${e.name || "Error"}: ${e.message}`; | ||
const getErrorStackFrames = (e: Error): string | undefined => e.stack?.replace(getErrorTitle(e) + "\n", ""); |
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.
CT could lose e.name
due to serialization
The first line of the trace will be prefixed with ${myObject.name}: ${myObject.message}.
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.
CT could lose e.name due to serialization
Hm, looks like problem in this helper - https://github.com/gemini-testing/testplane/blob/master/src/runner/browser-env/vite/browser-modules/errors/index.ts#L42-L45. Object.getOwnPropertyNames(error)
doesn't return name
field. I will fix it.
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.
Heh, first time i see JSON.stringify
with array being used as the second argument.
src/utils/stacktrace.ts
Outdated
Error.captureStackTrace(targetObj, filterFunc || captureRawStackFrames); | ||
Error.stackTraceLimit = savedStackTraceLimit; | ||
|
||
const rawFramesPosition = targetObj.stack.indexOf("\n") + 1; // crop out error message |
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.
The first line of the trace will be prefixed with ${myObject.name}: ${myObject.message}.
src/utils/stacktrace.ts
Outdated
if (rawFramesArr.length !== framesParsed.length) { | ||
return error; | ||
} |
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.
We assume "each line of rawFramesArr is equal to framesParsed"
Because we filter "rawFramesArr" using "framesParsed" and we can't build parsed frames back to stacktrace.
I dont know if rawFramesArr.length !== framesParsed.length
even could be true, but just in case
src/utils/stacktrace.ts
Outdated
|
||
isNested(childFrames: RawStackFrames): boolean { | ||
for (const parentFrames of this._framesMap.values()) { | ||
if (childFrames.length !== parentFrames.length && childFrames.endsWith(parentFrames)) { |
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.
childFrames.length !== parentFrames.length
Because user might have a loop.
childFrames.endsWith(parentFrames)
If true, then we already wrapped parent call
src/browser/stacktrace.ts
Outdated
const frames = captureRawStackFrames(stackFilterFunc || runWithStacktraceHooks); | ||
|
||
if (stackFrames.isNested(frames)) { | ||
return fn(); | ||
} |
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.
We dont want to save all of the frames, because with nested calls it would use some memory
b700da5
to
da9b2ad
Compare
@@ -119,6 +120,8 @@ module.exports = class TestRunner extends Runner { | |||
this._browserAgent.freeBrowser(browser); | |||
|
|||
if (error) { | |||
filterExtraWdioFrames(error); |
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.
Moved filter
to here, because stacktrace hooks are not executed in browser runner.
That way we can filter wdio frames in only one place
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.
🔥
@@ -1,7 +1,7 @@ | |||
import type { Browser } from "../types"; | |||
import logger from "../../utils/logger"; | |||
|
|||
export default async (browser: Browser): Promise<void> => { | |||
export default (browser: Browser): void => { |
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.
👍
src/utils/stacktrace.ts
Outdated
const getErrorTitle = (e: Error): string => `${e.name || "Error"}: ${e.message}`; | ||
const getErrorStackFrames = (e: Error): string | undefined => e.stack?.replace(getErrorTitle(e) + "\n", ""); |
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.
CT could lose e.name due to serialization
Hm, looks like problem in this helper - https://github.com/gemini-testing/testplane/blob/master/src/runner/browser-env/vite/browser-modules/errors/index.ts#L42-L45. Object.getOwnPropertyNames(error)
doesn't return name
field. I will fix it.
src/utils/stacktrace.ts
Outdated
return error; | ||
} | ||
|
||
const frames = getErrorStackFrames(error)!; |
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.
Why doesn't getErrorStackFrames
method return array with frames? By its name, I would expect this behavior.
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.
Because it is located near the captureRawStackFrames
and captureRawStackFrames
returns string. Replaced it with:
type ErrorWithStack = SetRequired<Error, "stack">;
const getErrorRawStackFrames = (e: ErrorWithStack): RawStackFrames => e.stack.replace(getErrorTitle(e) + "\n", "");
so it would be more clear
src/utils/stacktrace.ts
Outdated
"Element.wrapCommandFn", | ||
"Element.<anonymous>", | ||
"Element.newCommand", | ||
"Element.elementErrorHandlerCallbackFn", |
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.
How did you search for all the meaningless frames? Just run few different tests?
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.
Yes. Those are present in stacktrace of error, produced by any command. So these give us no information.
da9b2ad
to
69ce211
Compare
69ce211
to
36bb693
Compare
What is done
First part of
HERMIONE-1517
It only does two things:
WebdriverIO.Browser
andWebdriverIO.Element
callsExample
Before
After