fix: support error snippets nested browser commands

gemini-testing · Jul 17, 2024 · 3c9ce6b · 3c9ce6b
1 parent 23e8b03
commit 3c9ce6b
Show file tree

Hide file tree

Showing 9 changed files with 175 additions and 112 deletions.
diff --git a/src/browser/stacktrace/index.ts b/src/browser/stacktrace/index.ts
@@ -1,4 +1,4 @@
-import { ShallowStackFrames, applyStackFrames, captureRawStackFrames } from "./utils";
+import { ShallowStackFrames, applyStackTraceIfBetter, captureRawStackFrames } from "./utils";
 import { getBrowserCommands, getElementCommands } from "../history/commands";
 import { runWithHooks } from "../history/utils";
 
@@ -15,7 +15,7 @@ export const runWithStacktraceHooks = ({
 }): ReturnType<typeof fn> => {
     const frames = captureRawStackFrames(stackFilterFunc || runWithStacktraceHooks);
 
-    if (stackFrames.isNested(frames)) {
+    if (stackFrames.areInternal(frames)) {
         return fn();
     }
 
@@ -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),
     });
 };
 

diff --git a/src/browser/stacktrace/utils.ts b/src/browser/stacktrace/utils.ts
@@ -1,6 +1,8 @@
+import _ from "lodash";
 import ErrorStackParser from "error-stack-parser";
 import type { SetRequired } from "type-fest";
 import logger from "../../utils/logger";
+import { softFileURLToPath } from "../../utils/fs";
 import { WDIO_IGNORED_STACK_FUNCTIONS, WDIO_STACK_TRACE_LIMIT } from "./constants";
 
 export type RawStackFrames = string;
@@ -48,7 +50,7 @@ const getErrorRawStackFrames = (e: ErrorWithStack): RawStackFrames => {
     return e.stack.slice(errorMessageEndsStackIndex + 1);
 };
 
-export function captureRawStackFrames(filterFunc?: (...args: unknown[]) => unknown): RawStackFrames {
+export const captureRawStackFrames = (filterFunc?: (...args: unknown[]) => unknown): RawStackFrames => {
     const savedStackTraceLimit = Error.stackTraceLimit;
     const targetObj = {} as { stack: RawStackFrames };
 
@@ -59,19 +61,90 @@ export function captureRawStackFrames(filterFunc?: (...args: unknown[]) => unkno
     const rawFramesPosition = targetObj.stack.indexOf("\n") + 1; // crop out error message
 
     return targetObj.stack.slice(rawFramesPosition);
-}
+};
+
+/**
+ * @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_RELEVANCE: 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_RELEVANCE) {
+        if (FRAME_RELEVANCE[factor].matcher(fileName)) {
+            return FRAME_RELEVANCE[factor].value;
+        }
+    }
+
+    return 0;
+};
 
-export function applyStackFrames(error: Error, frames: RawStackFrames): Error {
+const getStackTraceRelevance = (error: Error): number => {
+    const framesParsed = ErrorStackParser.parse(error);
+
+    return framesParsed.reduce((maxRelevance, frame) => {
+        return Math.max(maxRelevance, getFrameRelevance(frame));
+    }, 0);
+};
+
+const createErrorWithStack = (stack: RawStackFrames, errorMessage = ""): Error => {
+    const newError = new Error(errorMessage);
+
+    newError.stack = getErrorTitle(newError) + "\n" + stack;
+
+    return newError;
+};
+
+const applyStackTrace = (error: Error, stack: RawStackFrames): Error => {
     if (!error || !error.message) {
         return error;
     }
 
-    error.stack = getErrorTitle(error) + "\n" + frames;
+    error.stack = getErrorTitle(error) + "\n" + stack;
 
     return error;
-}
+};
+
+export const applyStackTraceIfBetter = (error: Error, stack: RawStackFrames): Error => {
+    if (!error || !error.message) {
+        return error;
+    }
+
+    try {
+        const newStackTraceRelevance = getStackTraceRelevance(createErrorWithStack(stack));
+        const currentStackTraceRelevance = getStackTraceRelevance(error);
+
+        if (newStackTraceRelevance > currentStackTraceRelevance) {
+            applyStackTrace(error, stack);
+        }
+    } catch (err) {
+        logger.warn("Couldn't compare error stack traces");
+    }
+
+    return error;
+};
 
-export function filterExtraWdioFrames(error: Error): Error {
+export const filterExtraWdioFrames = (error: Error): Error => {
     if (!error || !error.message || !error.stack) {
         return error;
     }
@@ -103,13 +176,13 @@ export function filterExtraWdioFrames(error: Error): Error {
 
         const framesFiltered = rawFramesArr.filter((_, i) => shouldIncludeFrame(framesParsed[i])).join("\n");
 
-        return applyStackFrames(error, framesFiltered);
+        return applyStackTrace(error, framesFiltered);
     } catch (filterError) {
         logger.warn("Couldn't filter out wdio frames", filterError);
 
         return error;
     }
-}
+};
 
 export class ShallowStackFrames {
     private _framesMap: Map<string, string>;
@@ -132,13 +205,29 @@ export class ShallowStackFrames {
         this._framesMap.delete(key);
     }
 
-    isNested(childFrames: RawStackFrames): boolean {
+    private _getParentStackFrame(childFrames: RawStackFrames): RawStackFrames | null {
         for (const parentFrames of this._framesMap.values()) {
             if (childFrames.length !== parentFrames.length && childFrames.endsWith(parentFrames)) {
-                return true;
+                return parentFrames;
             }
         }
 
-        return false;
+        return null;
+    }
+
+    areInternal(childFrames: RawStackFrames): boolean {
+        const parentStackFrame = this._getParentStackFrame(childFrames);
+
+        if (!parentStackFrame) {
+            return false;
+        }
+
+        const isNodeModulesFrame = (frame: string): boolean => frame.includes("/node_modules/");
+        const isNodeInternalFrame = (frame: string): boolean => frame.includes(" (node:");
+
+        const extraFrames = childFrames.slice(0, childFrames.length - parentStackFrame.length);
+        const extraFramesArray = extraFrames.split("\n");
+
+        return extraFramesArray.every(frame => !frame || isNodeModulesFrame(frame) || isNodeInternalFrame(frame));
     }
 }
diff --git a/src/error-snippets/frames.ts b/src/error-snippets/frames.ts
@@ -1,44 +1,8 @@
-import _ from "lodash";
 import ErrorStackParser from "error-stack-parser";
 import logger from "../utils/logger";
-import { softFileURLToPath } from "./utils";
+import { getFrameRelevance } from "../browser/stacktrace/utils";
 import type { ResolvedFrame, SufficientStackFrame } from "./types";
-
-/**
- * @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
- */
-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;
-
-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;
-};
+import { softFileURLToPath } from "../utils/fs";
 
 export const findRelevantStackFrame = (error: Error): SufficientStackFrame | null => {
     try {

diff --git a/src/error-snippets/source-maps.ts b/src/error-snippets/source-maps.ts
@@ -1,7 +1,8 @@
 import { SourceMapConsumer, type BasicSourceMapConsumer } from "source-map";
 import url from "url";
 import { SOURCE_MAP_URL_COMMENT } from "./constants";
-import { softFileURLToPath, getSourceCodeFile } from "./utils";
+import { getSourceCodeFile } from "./utils";
+import { softFileURLToPath } from "../utils/fs";
 import type { SufficientStackFrame, ResolvedFrame } from "./types";
 
 export const extractSourceMaps = async (

diff --git a/src/error-snippets/utils.ts b/src/error-snippets/utils.ts
@@ -1,11 +1,11 @@
 import path from "path";
-import { fileURLToPath } from "url";
 import fs from "fs-extra";
 import { codeFrameColumns } from "@babel/code-frame";
 import { getErrorTitle } from "../browser/stacktrace/utils";
 import { SNIPPET_LINES_ABOVE, SNIPPET_LINES_BELOW, SOURCE_MAP_URL_COMMENT } from "./constants";
 import { AssertViewError } from "../browser/commands/assert-view/errors/assert-view-error";
 import { BaseStateError } from "../browser/commands/assert-view/errors/base-state-error";
+import { softFileURLToPath } from "../utils/fs";
 
 interface FormatFileNameHeaderOpts {
     line: number;
@@ -29,23 +29,16 @@ export const shouldNotAddCodeSnippet = (err: Error): boolean => {
     return isScreenshotError;
 };
 
-export const softFileURLToPath = (fileName: string): string => {
-    if (!fileName.startsWith("file://")) {
-        return fileName;
-    }
-
-    try {
-        return fileURLToPath(fileName);
-    } catch (_) {
-        return fileName;
-    }
+const trimAsyncPrefix = (fileName: string): string => {
+    const asyncPrefix = "async ";
+    return fileName.startsWith(asyncPrefix) ? fileName.slice(asyncPrefix.length) : fileName;
 };
 
 export const formatFileNameHeader = (fileName: string, opts: FormatFileNameHeaderOpts): string => {
     const lineNumberWidth = String(opts.line - opts.linesAbove).length;
     const offsetWidth = String(opts.line + opts.linesBelow).length;
 
-    const filePath = softFileURLToPath(fileName);
+    const filePath = softFileURLToPath(trimAsyncPrefix(fileName));
     const relativeFileName = path.isAbsolute(filePath) ? path.relative(process.cwd(), filePath) : filePath;
     const lineNumberOffset = ".".repeat(lineNumberWidth).padStart(offsetWidth);
     const offset = `  ${lineNumberOffset} |`;
@@ -78,7 +71,7 @@ export const formatErrorSnippet = (error: Error, { file, source, location }: For
 };
 
 export const getSourceCodeFile = async (fileName: string): Promise<string> => {
-    const filePath = softFileURLToPath(fileName);
+    const filePath = softFileURLToPath(trimAsyncPrefix(fileName));
 
     if (path.isAbsolute(filePath)) {
         return fs.readFile(filePath, "utf8");

diff --git a/src/utils/fs.ts b/src/utils/fs.ts
@@ -1,4 +1,5 @@
 import fs from "fs";
+import { fileURLToPath } from "url";
 
 export const exists = async (path: string): Promise<boolean> => {
     try {
@@ -8,3 +9,15 @@ export const exists = async (path: string): Promise<boolean> => {
         return false;
     }
 };
+
+export const softFileURLToPath = (fileName: string): string => {
+    if (!fileName.startsWith("file://")) {
+        return fileName;
+    }
+
+    try {
+        return fileURLToPath(fileName);
+    } catch (_) {
+        return fileName;
+    }
+};
diff --git a/test/src/browser/stacktrace/index.ts b/test/src/browser/stacktrace/index.ts
@@ -27,7 +27,7 @@ describe("stacktrace", () => {
         runWithStacktraceHooks = proxyquire("../../../../src/browser/stacktrace", {
             "./utils": {
                 captureRawStackFrames: captureRawStackFramesSpy,
-                applyStackFrames: applyStackFramesStub,
+                applyStackTraceIfBetter: applyStackFramesStub,
             },
         }).runWithStacktraceHooks;
 
@@ -36,7 +36,7 @@ describe("stacktrace", () => {
         sandbox.spy(stackFrames, "enter");
         sandbox.spy(stackFrames, "leave");
         sandbox.spy(stackFrames, "getKey");
-        sandbox.spy(stackFrames, "isNested");
+        sandbox.spy(stackFrames, "areInternal");
     });
 
     afterEach(() => sandbox.restore());
@@ -73,22 +73,12 @@ describe("stacktrace", () => {
             assert.calledOnce(stackFrames.enter);
         });
 
-        it("should enter stack trace once with nested calls", () => {
-            const fn = sandbox.stub().callsFake(() => {
-                const nestedFirst = sandbox.stub().callsFake(() => {
-                    const nestedSecond = sandbox.stub().callsFake(() => {
-                        return runWithStacktraceHooks_(sandbox.stub());
-                    });
+        it("should enter stack trace once if frames are irrelevant", () => {
+            stackFrames.areInternal = sandbox.stub().returns(true);
 
-                    return runWithStacktraceHooks_(nestedSecond);
-                });
+            runWithStacktraceHooks_(() => {});
 
-                return runWithStacktraceHooks_(nestedFirst);
-            });
-
-            runWithStacktraceHooks_(fn);
-
-            assert.calledOnce(stackFrames.enter);
+            assert.notCalled(stackFrames.enter);
         });
 
         it("should leave stack trace after function resolved", async () => {