From 3c9ce6becccf1f8940dbd9c92204b15c4efa184a Mon Sep 17 00:00:00 2001 From: Roman Kuznetsov Date: Mon, 15 Jul 2024 03:45:24 +0300 Subject: [PATCH] fix: support error snippets nested browser commands --- src/browser/stacktrace/index.ts | 6 +- src/browser/stacktrace/utils.ts | 111 ++++++++++++++++++++++++--- src/error-snippets/frames.ts | 40 +--------- src/error-snippets/source-maps.ts | 3 +- src/error-snippets/utils.ts | 19 ++--- src/utils/fs.ts | 13 ++++ test/src/browser/stacktrace/index.ts | 22 ++---- test/src/browser/stacktrace/utils.ts | 65 ++++++++++------ test/src/error-snippets/utils.ts | 8 +- 9 files changed, 175 insertions(+), 112 deletions(-) diff --git a/src/browser/stacktrace/index.ts b/src/browser/stacktrace/index.ts index 2fd4daa9e..2dc53e355 100644 --- 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 => { 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 index a42a6ef06..20a2be9d8 100644 --- 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 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; @@ -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 index 1507126e6..12e6efa0c 100644 --- 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 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 index e2162acfa..5604803b3 100644 --- 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 index dc8608da2..2f9cfd0e4 100644 --- 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 => { - 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 index 79d678b6a..c9faa2715 100644 --- 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 => { try { @@ -8,3 +9,15 @@ export const exists = async (path: string): Promise => { 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 index 59a317568..84c0b4b58 100644 --- 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 () => { diff --git a/test/src/browser/stacktrace/utils.ts b/test/src/browser/stacktrace/utils.ts index bb4a01cf3..63d294efd 100644 --- a/test/src/browser/stacktrace/utils.ts +++ b/test/src/browser/stacktrace/utils.ts @@ -1,13 +1,13 @@ import { ShallowStackFrames, - applyStackFrames, + applyStackTraceIfBetter, captureRawStackFrames, filterExtraWdioFrames, } from "../../../../src/browser/stacktrace/utils"; type AnyFunc = (...args: any[]) => unknown; // eslint-disable-line @typescript-eslint/no-explicit-any -describe("utils/stacktrace", () => { +describe("stacktrace/utils", () => { describe("captureRawStackFrames", () => { it("should only return frames", () => { const frames = captureRawStackFrames(); @@ -29,32 +29,30 @@ describe("utils/stacktrace", () => { }); }); - describe("applyStackFrames", () => { + describe("applyStackTraceIfBetter", () => { it("should work with multiline error messages", () => { const error = new Error("my\nmulti-line\nerror\nmessage"); - const frames = "foo\nbar"; + error.stack = "Error: " + error.message + "\n"; - applyStackFrames(error, frames); - - const expectedStack = ["Error: my\nmulti-line\nerror\nmessage", "foo", "bar"].join("\n"); - - assert.equal(error.stack, expectedStack); - }); - - it("should work with error-like objects", () => { - const error = { message: "foo" } as Error; - const frames = "bar"; + const frames = [ + "at Context. (test/src/browser/stacktrace/utils.ts:43:20)", + "at processImmediate (node:internal/timers:471:21)", + ].join("\n"); - applyStackFrames(error, frames); + applyStackTraceIfBetter(error, frames); - const expectedStack = ["Error: foo", "bar"].join("\n"); + const expectedStack = [ + "Error: my\nmulti-line\nerror\nmessage", + "at Context. (test/src/browser/stacktrace/utils.ts:43:20)", + "at processImmediate (node:internal/timers:471:21)", + ].join("\n"); assert.equal(error.stack, expectedStack); }); it("should not throw on bad input", () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any - assert.doesNotThrow(() => applyStackFrames("foo" as any, 1 as any)); + assert.doesNotThrow(() => applyStackTraceIfBetter("foo" as any, 1 as any)); }); }); @@ -73,7 +71,7 @@ describe("utils/stacktrace", () => { "at async Socket. (http://localhost:4001/node_modules/testplane/build/src/runner/browser-env/vite/browser-modules/mocha/index.js?v=80fca7b2:54:17)", ].join("\n"); - applyStackFrames(error, errorStack); + error.stack = `${error.name}: ${error.message}\n${errorStack}`; filterExtraWdioFrames(error); const expectedStack = [ @@ -106,14 +104,14 @@ describe("utils/stacktrace", () => { }); }); - describe("isNested", () => { + describe("areInternal", () => { it("should return 'false' on different frames", () => { const key = stackFrames.getKey(); const parentFrames = "f\no\no"; stackFrames.enter(key, parentFrames); - assert.isFalse(stackFrames.isNested("b\na\nr")); + assert.isFalse(stackFrames.areInternal("b\na\nr")); stackFrames.leave(key); }); @@ -124,18 +122,37 @@ describe("utils/stacktrace", () => { stackFrames.enter(key, parentFrames); - assert.isFalse(stackFrames.isNested("f\no\no")); + assert.isFalse(stackFrames.areInternal("f\no\no")); + + stackFrames.leave(key); + }); + + it("should return 'true' on nested frames if those are internal frames", () => { + const key = stackFrames.getKey(); + const parentFrames = `b\na\nr`; + const childFrames = [ + "at async Element.wrapCommandFn (file:///project_folder/node_modules/webdriverio/node_modules/@wdio/utils/build/shim.js:81:29)", + "at processTicksAndRejections (node:internal/process/task_queues:95:5)", + "b", + "a", + "r", + ].join("\n"); + + stackFrames.enter(key, parentFrames); + + assert.isTrue(stackFrames.areInternal(childFrames)); stackFrames.leave(key); }); - it("should return 'true' on nested frames", () => { + it("should return 'false' on nested frames if those are not internal frames", () => { const key = stackFrames.getKey(); - const parentFrames = "b\na\nr"; + const parentFrames = `b\na\nr`; + const childFrames = ["f", "o", "o", "b", "a", "r"].join("\n"); stackFrames.enter(key, parentFrames); - assert.isTrue(stackFrames.isNested("f\no\no\nb\na\nr")); + assert.isFalse(stackFrames.areInternal(childFrames)); stackFrames.leave(key); }); diff --git a/test/src/error-snippets/utils.ts b/test/src/error-snippets/utils.ts index 22f2bf564..494f5ae48 100644 --- a/test/src/error-snippets/utils.ts +++ b/test/src/error-snippets/utils.ts @@ -2,12 +2,8 @@ import path from "path"; import sinon from "sinon"; import url from "url"; import fs from "fs-extra"; -import { - softFileURLToPath, - formatFileNameHeader, - getSourceCodeFile, - formatErrorSnippet, -} from "../../../src/error-snippets/utils"; +import { formatFileNameHeader, getSourceCodeFile, formatErrorSnippet } from "../../../src/error-snippets/utils"; +import { softFileURLToPath } from "../../../src/utils/fs"; import type { codeFrameColumns } from "@babel/code-frame"; const codeFrame = require("@babel/code-frame"); // eslint-disable-line @typescript-eslint/no-var-requires