fix: support error snippets nested browser commands #972

KuznetsovRoman · 2024-07-15T00:45:51Z

Examples: https://nda.ya.ru/t/1XWaQ6YG76y5LA

KuznetsovRoman · 2024-07-15T10:28:44Z

src/browser/stacktrace/index.ts

@@ -15,7 +15,7 @@ export const runWithStacktraceHooks = ({
 }): ReturnType<typeof fn> => {
    const frames = captureRawStackFrames(stackFilterFunc || runWithStacktraceHooks);

-    if (stackFrames.isNested(frames)) {
+    if (stackFrames.areInternal(frames)) {


Now some nested frames are saved.
Only internal ones are completely ignored (node_modules, node:internal)

KuznetsovRoman · 2024-07-15T10:29:18Z

src/browser/stacktrace/index.ts

@@ -25,7 +25,7 @@ export const runWithStacktraceHooks = ({
        before: () => stackFrames.enter(key, frames),
        fn,
        after: () => stackFrames.leave(key),
-        error: (err: Error) => applyStackFrames(err, frames),
+        error: (err: Error) => applyStackTraceIfBetter(err, frames),


From multiple saved stacktraces, we apply the best one

KuznetsovRoman · 2024-07-15T10:29:36Z

src/browser/stacktrace/utils.ts

+/**
+ * @description
+ * Rank values:
+ *
+ * 0: Can't extract code snippet; useless
+ *
+ * 1: WebdriverIO internals: Better than nothing
+ *
+ * 2: Project internals: Better than WebdriverIO internals, but worse, than user code part
+ *
+ * 3: User code: Best choice
+ */
+export const FRAME_REELVANCE: Record<string, { value: number; matcher: (fileName: string) => boolean }> = {
+    repl: { value: 0, matcher: fileName => /^REPL\d+$/.test(fileName) },
+    nodeInternals: { value: 0, matcher: fileName => /^node:[a-zA-Z\-_]/.test(fileName) },
+    wdioInternals: { value: 1, matcher: fileName => fileName.includes("/node_modules/webdriverio/") },
+    projectInternals: { value: 2, matcher: fileName => fileName.includes("/node_modules/") },
+    userCode: { value: 3, matcher: () => true },
+} as const;
+
+export const getFrameRelevance = (frame: StackFrame): number => {
+    if ([frame.fileName, frame.lineNumber, frame.columnNumber].some(_.isUndefined)) {
+        return 0;
+    }
+
+    const fileName: string = softFileURLToPath(frame.fileName!);
+
+    for (const factor in FRAME_REELVANCE) {
+        if (FRAME_REELVANCE[factor].matcher(fileName)) {
+            return FRAME_REELVANCE[factor].value;
+        }
+    }
+
+    return 0;
+};


Pulled out from "error-snippets"

KuznetsovRoman · 2024-07-15T10:30:18Z

src/browser/stacktrace/utils.ts

+const getStackTraceRelevance = (error: Error): number => {
+    const framesParsed = ErrorStackParser.parse(error);
+
+    return framesParsed.reduce((maxRelevance, frame) => {
+        return Math.max(maxRelevance, getFrameRelevance(frame));
+    }, 0);
+};


Calculates relevance of each frame and returns stacktrace relevance, based on relevance of the most relevant frame

KuznetsovRoman · 2024-07-15T10:30:47Z

src/browser/stacktrace/utils.ts

+const createErrorWithStack = (stack: RawStackFrames, errorMessage = ""): Error => {
+    const newError = new Error(errorMessage);
+
+    newError.stack = getErrorTitle(newError) + "\n" + stack;
+
+    return newError;
+};


Helper function. Its only needed to parse stack frames.

KuznetsovRoman · 2024-07-15T10:32:57Z

src/browser/stacktrace/utils.ts

-}
+};
+
+export const applyStackTraceIfBetter = (error: Error, stack: RawStackFrames): Error => {


Calculates current stacktrace relevance and stacktrace relevance of the next nested level call.
Apples the new one if it suites better

Why not just leave the old name - applyStackTrace? To my mind user of this method should not know about its implementation details. I would rename applyStackTrace above to something else, for example to modifyStackTrace.

Why not just leave the old name - applyStackTrace?

Lets pretend you don't know implementation details. You call

const error = ... const myNewStack = ... applyStackTrace(error, myNewStack);

What do you think error.stack is now equal to? According to applyStackTrace function name, of course it is "myNewStack". But turns out, it is not, and its even worse. It is "myNewStack" sometimes, but also sometimes original "error.stack"

So that's why it is called "applyStackTraceIfBetter". That way developer would not be ensured "error.stack" to be "myNewStack". He would get from this function name that "it only apples stacktrace if it is better"

KuznetsovRoman · 2024-07-15T10:34:50Z

src/browser/stacktrace/utils.ts

+        const isNodeModulesFrame = (frame: string): boolean => frame.includes("/node_modules/");
+        const isNodeInternalFrame = (frame: string): boolean => frame.includes(" (node:");


Here it is too expensive to parse frames, because "areInternal" method will be called a lot of times on every browser command. So we are checking node_modules frames and internal frames by these functions

KuznetsovRoman · 2024-07-15T10:36:32Z

src/error-snippets/utils.ts

-    } catch (_) {
-        return fileName;
-    }
+const trimAsyncPrefix = (fileName: string): string => {


Also noticed "error-stack-parser" does not trim "async" prefix on files.

KuznetsovRoman · 2024-07-15T10:37:03Z

src/utils/fs.ts

+export const softFileURLToPath = (fileName: string): string => {
+    if (!fileName.startsWith("file://")) {
+        return fileName;
+    }
+
+    try {
+        return fileURLToPath(fileName);
+    } catch (_) {
+        return fileName;
+    }
+};


Pulled out of "error-snippets" because it is used inside "browser/stacktrace" too

DudaGod · 2024-07-17T09:22:21Z

src/browser/stacktrace/utils.ts

-}
+};
+
+export const applyStackTraceIfBetter = (error: Error, stack: RawStackFrames): Error => {


Why not just leave the old name - applyStackTrace? To my mind user of this method should not know about its implementation details. I would rename applyStackTrace above to something else, for example to modifyStackTrace.

DudaGod · 2024-07-17T09:23:47Z

src/browser/stacktrace/utils.ts

+    return newError;
+};
+
+const applyStackTrace = (error: Error, stack: RawStackFrames): Error => {
    if (!error || !error.message) {
        return error;
    }


It looks like that we don't need this condition. Because passed error from applyStackTraceIfBetter and filterExtraWdioFrames already perform the same check

In this scenarios, yes. In general, it is critical for the whole "stacktrace" module not to throw errors. Because if it does, the entire run will fail due to unhandled error.

As the check is cheep, i would like to keep it here.

src/browser/stacktrace/utils.ts

KuznetsovRoman force-pushed the TESTPLANE-111.stacktrace_relevance branch from 6477795 to 370090e Compare July 15, 2024 10:27

KuznetsovRoman commented Jul 15, 2024

View reviewed changes

DudaGod approved these changes Jul 17, 2024

View reviewed changes

KuznetsovRoman force-pushed the TESTPLANE-111.stacktrace_relevance branch from 370090e to bf8a367 Compare July 17, 2024 11:45

fix: support error snippets nested browser commands

3c9ce6b

KuznetsovRoman force-pushed the TESTPLANE-111.stacktrace_relevance branch from bf8a367 to 3c9ce6b Compare July 17, 2024 11:48

KuznetsovRoman merged commit 64576dc into master Jul 17, 2024
2 checks passed

KuznetsovRoman deleted the TESTPLANE-111.stacktrace_relevance branch July 17, 2024 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support error snippets nested browser commands #972

fix: support error snippets nested browser commands #972

KuznetsovRoman commented Jul 15, 2024 •

edited

Loading

KuznetsovRoman Jul 15, 2024

KuznetsovRoman Jul 15, 2024

KuznetsovRoman Jul 15, 2024

KuznetsovRoman Jul 15, 2024

KuznetsovRoman Jul 15, 2024

KuznetsovRoman Jul 15, 2024

DudaGod Jul 17, 2024

KuznetsovRoman Jul 17, 2024

KuznetsovRoman Jul 15, 2024

KuznetsovRoman Jul 15, 2024

KuznetsovRoman Jul 15, 2024

DudaGod Jul 17, 2024

DudaGod Jul 17, 2024

KuznetsovRoman Jul 17, 2024

		const isNodeModulesFrame = (frame: string): boolean => frame.includes("/node_modules/");
		const isNodeInternalFrame = (frame: string): boolean => frame.includes(" (node:");

fix: support error snippets nested browser commands #972

fix: support error snippets nested browser commands #972

Conversation

KuznetsovRoman commented Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KuznetsovRoman commented Jul 15, 2024 •

edited

Loading