From 083d13a13dddb6ce3f3e8009c47adeee72f5f3e3 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Fri, 12 May 2023 14:23:22 -0700 Subject: [PATCH] chore: surface syntax error in ui mode (#22982) Fixes https://github.com/microsoft/playwright/issues/22863 --- .../playwright-test/src/common/testLoader.ts | 20 +++++++++++-- .../playwright-test/src/common/transform.ts | 20 +++++-------- .../playwright-test/src/matchers/expect.ts | 8 +++-- packages/playwright-test/src/runner/runner.ts | 2 +- packages/playwright-test/src/runner/tasks.ts | 6 ++-- packages/playwright-test/src/runner/uiMode.ts | 2 +- packages/playwright-test/src/util.ts | 21 ++++++------- .../ui-mode-test-source.spec.ts | 30 ++++++++++++++++++- 8 files changed, 73 insertions(+), 36 deletions(-) diff --git a/packages/playwright-test/src/common/testLoader.ts b/packages/playwright-test/src/common/testLoader.ts index 08b4cfee88..da152f2ab8 100644 --- a/packages/playwright-test/src/common/testLoader.ts +++ b/packages/playwright-test/src/common/testLoader.ts @@ -15,11 +15,12 @@ */ import path from 'path'; +import util from 'util'; import type { TestError } from '../../reporter'; import { isWorkerProcess, setCurrentlyLoadingFileSuite } from './globals'; import { Suite } from './test'; import { requireOrImport } from './transform'; -import { serializeError } from '../util'; +import { filterStackTrace } from '../util'; import { startCollectingFileDeps, stopCollectingFileDeps } from './compilationCache'; export const defaultTimeout = 30000; @@ -44,7 +45,7 @@ export async function loadTestFile(file: string, rootDir: string, testErrors?: T } catch (e) { if (!testErrors) throw e; - testErrors.push(serializeError(e)); + testErrors.push(serializeLoadError(file, e)); } finally { stopCollectingFileDeps(file); setCurrentlyLoadingFileSuite(undefined); @@ -72,3 +73,18 @@ export async function loadTestFile(file: string, rootDir: string, testErrors?: T return suite; } + +function serializeLoadError(file: string, error: Error | any): TestError { + if (error instanceof Error) { + const result: TestError = filterStackTrace(error); + // Babel parse errors have location. + const loc = (error as any).loc; + result.location = loc ? { + file, + line: loc.line || 0, + column: loc.column || 0, + } : undefined; + return result; + } + return { value: util.inspect(error) }; +} diff --git a/packages/playwright-test/src/common/transform.ts b/packages/playwright-test/src/common/transform.ts index f311e4dbb6..e97c41bb72 100644 --- a/packages/playwright-test/src/common/transform.ts +++ b/packages/playwright-test/src/common/transform.ts @@ -134,7 +134,7 @@ export function resolveHook(filename: string, specifier: string): string | undef } } -export function transformHook(code: string, filename: string, moduleUrl?: string): string { +export function transformHook(preloadedCode: string, filename: string, moduleUrl?: string): string { const isTypeScript = filename.endsWith('.ts') || filename.endsWith('.tsx'); const hasPreprocessor = process.env.PW_TEST_SOURCE_TRANSFORM && @@ -142,7 +142,7 @@ export function transformHook(code: string, filename: string, moduleUrl?: string process.env.PW_TEST_SOURCE_TRANSFORM_SCOPE.split(pathSeparator).some(f => filename.startsWith(f)); const pluginsPrologue = babelPlugins; const pluginsEpilogue = hasPreprocessor ? [[process.env.PW_TEST_SOURCE_TRANSFORM!]] as BabelPlugin[] : []; - const hash = calculateHash(code, filename, !!moduleUrl, pluginsPrologue, pluginsEpilogue); + const hash = calculateHash(preloadedCode, filename, !!moduleUrl, pluginsPrologue, pluginsEpilogue); const { cachedCode, addToCache } = getFromCompilationCache(filename, hash, moduleUrl); if (cachedCode) return cachedCode; @@ -151,17 +151,11 @@ export function transformHook(code: string, filename: string, moduleUrl?: string // Silence the annoying warning. process.env.BROWSERSLIST_IGNORE_OLD_DATA = 'true'; - try { - const { babelTransform }: { babelTransform: BabelTransformFunction } = require('./babelBundle'); - const { code, map } = babelTransform(filename, isTypeScript, !!moduleUrl, pluginsPrologue, pluginsEpilogue); - if (code) - addToCache!(code, map); - return code || ''; - } catch (e) { - // Re-throw error with a playwright-test stack - // that could be filtered out. - throw new Error(e.message); - } + const { babelTransform }: { babelTransform: BabelTransformFunction } = require('./babelBundle'); + const { code, map } = babelTransform(filename, isTypeScript, !!moduleUrl, pluginsPrologue, pluginsEpilogue); + if (code) + addToCache!(code, map); + return code || ''; } function calculateHash(content: string, filePath: string, isModule: boolean, pluginsPrologue: BabelPlugin[], pluginsEpilogue: BabelPlugin[]): string { diff --git a/packages/playwright-test/src/matchers/expect.ts b/packages/playwright-test/src/matchers/expect.ts index 4f7d0ca747..11c4d1b23b 100644 --- a/packages/playwright-test/src/matchers/expect.ts +++ b/packages/playwright-test/src/matchers/expect.ts @@ -280,10 +280,12 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler { jestError.stack = jestError.name + ': ' + newMessage + '\n' + stringifyStackFrames(stackFrames).join('\n'); } - const serializerError = serializeError(jestError); - step.complete({ error: serializerError }); + const serializedError = serializeError(jestError); + // Serialized error has filtered stack trace. + jestError.stack = serializedError.stack; + step.complete({ error: serializedError }); if (this._info.isSoft) - testInfo._failWithError(serializerError, false /* isHardError */); + testInfo._failWithError(serializedError, false /* isHardError */); else throw jestError; }; diff --git a/packages/playwright-test/src/runner/runner.ts b/packages/playwright-test/src/runner/runner.ts index d54fb6b7bb..8f4a33d333 100644 --- a/packages/playwright-test/src/runner/runner.ts +++ b/packages/playwright-test/src/runner/runner.ts @@ -70,7 +70,7 @@ export class Runner { webServerPluginsForConfig(config).forEach(p => config.plugins.push({ factory: p })); const reporter = new InternalReporter(await createReporters(config, listOnly ? 'list' : 'run')); - const taskRunner = listOnly ? createTaskRunnerForList(config, reporter, 'in-process') + const taskRunner = listOnly ? createTaskRunnerForList(config, reporter, 'in-process', { failOnLoadErrors: true }) : createTaskRunner(config, reporter); const testRun = new TestRun(config, reporter); diff --git a/packages/playwright-test/src/runner/tasks.ts b/packages/playwright-test/src/runner/tasks.ts index 4a28e71246..e771982dc9 100644 --- a/packages/playwright-test/src/runner/tasks.ts +++ b/packages/playwright-test/src/runner/tasks.ts @@ -102,9 +102,9 @@ function addRunTasks(taskRunner: TaskRunner, config: FullConfigInternal return taskRunner; } -export function createTaskRunnerForList(config: FullConfigInternal, reporter: InternalReporter, mode: 'in-process' | 'out-of-process'): TaskRunner { +export function createTaskRunnerForList(config: FullConfigInternal, reporter: InternalReporter, mode: 'in-process' | 'out-of-process', options: { failOnLoadErrors: boolean }): TaskRunner { const taskRunner = new TaskRunner(reporter, config.config.globalTimeout); - taskRunner.addTask('load tests', createLoadTask(mode, { filterOnly: false, failOnLoadErrors: false })); + taskRunner.addTask('load tests', createLoadTask(mode, { ...options, filterOnly: false })); taskRunner.addTask('report begin', async ({ reporter, rootSuite }) => { reporter.onBegin(config.config, rootSuite!); return () => reporter.onEnd(); @@ -172,7 +172,7 @@ function createLoadTask(mode: 'out-of-process' | 'in-process', options: { filter await loadFileSuites(testRun, mode, options.failOnLoadErrors ? errors : softErrors); testRun.rootSuite = await createRootSuite(testRun, options.failOnLoadErrors ? errors : softErrors, !!options.filterOnly); // Fail when no tests. - if (!testRun.rootSuite.allTests().length && !testRun.config.cliPassWithNoTests && !testRun.config.config.shard) + if (options.failOnLoadErrors && !testRun.rootSuite.allTests().length && !testRun.config.cliPassWithNoTests && !testRun.config.config.shard) throw new Error(`No tests found`); }; } diff --git a/packages/playwright-test/src/runner/uiMode.ts b/packages/playwright-test/src/runner/uiMode.ts index 895c77cf45..317d43a7fe 100644 --- a/packages/playwright-test/src/runner/uiMode.ts +++ b/packages/playwright-test/src/runner/uiMode.ts @@ -149,7 +149,7 @@ class UIMode { const reporter = new InternalReporter([listReporter]); this._config.cliListOnly = true; this._config.testIdMatcher = undefined; - const taskRunner = createTaskRunnerForList(this._config, reporter, 'out-of-process'); + const taskRunner = createTaskRunnerForList(this._config, reporter, 'out-of-process', { failOnLoadErrors: false }); const testRun = new TestRun(this._config, reporter); clearCompilationCache(); reporter.onConfigure(this._config); diff --git a/packages/playwright-test/src/util.ts b/packages/playwright-test/src/util.ts index 1b7083220f..a03d6453c1 100644 --- a/packages/playwright-test/src/util.ts +++ b/packages/playwright-test/src/util.ts @@ -29,13 +29,15 @@ import type { RawStack } from 'playwright-core/lib/utils'; const PLAYWRIGHT_TEST_PATH = path.join(__dirname, '..'); const PLAYWRIGHT_CORE_PATH = path.dirname(require.resolve('playwright-core/package.json')); -export function filterStackTrace(e: Error) { +export function filterStackTrace(e: Error): { message: string, stack: string } { if (process.env.PWDEBUGIMPL) - return; + return { message: e.message, stack: e.stack || '' }; + const stackLines = stringifyStackFrames(filteredStackTrace(e.stack?.split('\n') || [])); - const message = e.message; - e.stack = `${e.name}: ${e.message}\n${stackLines.join('\n')}`; - e.message = message; + return { + message: e.message, + stack: `${e.name}: ${e.message}\n${stackLines.join('\n')}` + }; } export function filteredStackTrace(rawStack: RawStack): StackFrame[] { @@ -65,13 +67,8 @@ export function stringifyStackFrames(frames: StackFrame[]): string[] { } export function serializeError(error: Error | any): TestInfoError { - if (error instanceof Error) { - filterStackTrace(error); - return { - message: error.message, - stack: error.stack - }; - } + if (error instanceof Error) + return filterStackTrace(error); return { value: util.inspect(error) }; diff --git a/tests/playwright-test/ui-mode-test-source.spec.ts b/tests/playwright-test/ui-mode-test-source.spec.ts index a2b36ded68..3f1373a205 100644 --- a/tests/playwright-test/ui-mode-test-source.spec.ts +++ b/tests/playwright-test/ui-mode-test-source.spec.ts @@ -65,7 +65,7 @@ test('should show selected test in sources', async ({ runUITest }) => { ).toHaveText(`3 test('third', () => {});`); }); -test('should show syntax errors in file', async ({ runUITest }) => { +test('should show top-level errors in file', async ({ runUITest }) => { const { page } = await runUITest({ 'a.test.ts': ` import { test } from '@playwright/test'; @@ -100,3 +100,31 @@ test('should show syntax errors in file', async ({ runUITest }) => { 'Assignment to constant variable.' ]); }); + +test('should show syntax errors in file', async ({ runUITest }) => { + const { page } = await runUITest({ + 'a.test.ts': ` + import { test } from '@playwright/test'& + test('first', () => {}); + test('second', () => {}); + `, + }); + await expect.poll(dumpTestTree(page)).toBe(` + ◯ a.test.ts + `); + + await page.getByTestId('test-tree').getByText('a.test.ts').click(); + await expect( + page.getByTestId('source-code').locator('.source-tab-file-name') + ).toHaveText('a.test.ts'); + await expect( + page.locator('.CodeMirror .source-line-running'), + ).toHaveText(`2 import { test } from '@playwright/test'&`); + + await expect( + page.locator('.CodeMirror-linewidget') + ).toHaveText([ + '                                              ', + /Missing semicolon./ + ]); +});