From b10cc03314b27776358398c2f34805fe0ba1900b Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 8 May 2023 18:51:27 -0700 Subject: [PATCH] chore: render parse errors in the UI mode (#22888) Fixes: https://github.com/microsoft/playwright/issues/22863 --- .../playwright-test/src/runner/taskRunner.ts | 7 +- packages/playwright-test/src/runner/tasks.ts | 16 ++-- packages/trace-viewer/src/ui/modelUtil.ts | 11 ++- packages/trace-viewer/src/ui/sourceTab.tsx | 34 ++++---- packages/trace-viewer/src/ui/uiModeView.tsx | 86 ++++++++++++------- packages/trace-viewer/src/ui/workbench.tsx | 7 +- packages/web/src/components/listView.css | 5 -- .../ui-mode-test-source.spec.ts | 36 ++++++++ 8 files changed, 134 insertions(+), 68 deletions(-) diff --git a/packages/playwright-test/src/runner/taskRunner.ts b/packages/playwright-test/src/runner/taskRunner.ts index 55190d4989..1cce1089db 100644 --- a/packages/playwright-test/src/runner/taskRunner.ts +++ b/packages/playwright-test/src/runner/taskRunner.ts @@ -22,7 +22,7 @@ import { serializeError } from '../util'; import type { InternalReporter } from '../reporters/internalReporter'; type TaskTeardown = () => Promise | undefined; -export type Task = (context: Context, errors: TestError[]) => Promise | undefined; +export type Task = (context: Context, errors: TestError[], softErrors: TestError[]) => Promise | undefined; export class TaskRunner { private _tasks: { name: string, task: Task }[] = []; @@ -62,15 +62,16 @@ export class TaskRunner { break; debug('pw:test:task')(`"${name}" started`); const errors: TestError[] = []; + const softErrors: TestError[] = []; try { - const teardown = await task(context, errors); + const teardown = await task(context, errors, softErrors); if (teardown) teardownRunner._tasks.unshift({ name: `teardown for ${name}`, task: teardown }); } catch (e) { debug('pw:test:task')(`error in "${name}": `, e); errors.push(serializeError(e)); } finally { - for (const error of errors) + for (const error of [...softErrors, ...errors]) this._reporter.onError?.(error); if (errors.length) { if (!this._isTearDown) diff --git a/packages/playwright-test/src/runner/tasks.ts b/packages/playwright-test/src/runner/tasks.ts index db3c5caa42..4a28e71246 100644 --- a/packages/playwright-test/src/runner/tasks.ts +++ b/packages/playwright-test/src/runner/tasks.ts @@ -63,7 +63,7 @@ export class TestRun { export function createTaskRunner(config: FullConfigInternal, reporter: InternalReporter): TaskRunner { const taskRunner = new TaskRunner(reporter, config.config.globalTimeout); addGlobalSetupTasks(taskRunner, config); - taskRunner.addTask('load tests', createLoadTask('in-process', true)); + taskRunner.addTask('load tests', createLoadTask('in-process', { filterOnly: true, failOnLoadErrors: true })); addRunTasks(taskRunner, config); return taskRunner; } @@ -76,7 +76,7 @@ export function createTaskRunnerForWatchSetup(config: FullConfigInternal, report export function createTaskRunnerForWatch(config: FullConfigInternal, reporter: InternalReporter, additionalFileMatcher?: Matcher): TaskRunner { const taskRunner = new TaskRunner(reporter, 0); - taskRunner.addTask('load tests', createLoadTask('out-of-process', true, additionalFileMatcher)); + taskRunner.addTask('load tests', createLoadTask('out-of-process', { filterOnly: true, failOnLoadErrors: false, additionalFileMatcher })); addRunTasks(taskRunner, config); return taskRunner; } @@ -104,7 +104,7 @@ function addRunTasks(taskRunner: TaskRunner, config: FullConfigInternal export function createTaskRunnerForList(config: FullConfigInternal, reporter: InternalReporter, mode: 'in-process' | 'out-of-process'): TaskRunner { const taskRunner = new TaskRunner(reporter, config.config.globalTimeout); - taskRunner.addTask('load tests', createLoadTask(mode, false)); + taskRunner.addTask('load tests', createLoadTask(mode, { filterOnly: false, failOnLoadErrors: false })); taskRunner.addTask('report begin', async ({ reporter, rootSuite }) => { reporter.onBegin(config.config, rootSuite!); return () => reporter.onEnd(); @@ -166,11 +166,11 @@ function createRemoveOutputDirsTask(): Task { }; } -function createLoadTask(mode: 'out-of-process' | 'in-process', shouldFilterOnly: boolean, additionalFileMatcher?: Matcher): Task { - return async (testRun, errors) => { - await collectProjectsAndTestFiles(testRun, additionalFileMatcher); - await loadFileSuites(testRun, mode, errors); - testRun.rootSuite = await createRootSuite(testRun, errors, shouldFilterOnly); +function createLoadTask(mode: 'out-of-process' | 'in-process', options: { filterOnly: boolean, failOnLoadErrors: boolean, additionalFileMatcher?: Matcher }): Task { + return async (testRun, errors, softErrors) => { + await collectProjectsAndTestFiles(testRun, options.additionalFileMatcher); + 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) throw new Error(`No tests found`); diff --git a/packages/trace-viewer/src/ui/modelUtil.ts b/packages/trace-viewer/src/ui/modelUtil.ts index f18f35ae68..d5aa384125 100644 --- a/packages/trace-viewer/src/ui/modelUtil.ts +++ b/packages/trace-viewer/src/ui/modelUtil.ts @@ -19,7 +19,6 @@ import type { ResourceSnapshot } from '@trace/snapshot'; import type * as trace from '@trace/trace'; import type { ActionTraceEvent, EventTraceEvent } from '@trace/trace'; import type { ContextEntry, PageEntry } from '../entries'; -import type { SerializedError, StackFrame } from '@protocol/channels'; const contextSymbol = Symbol('context'); const nextInContextSymbol = Symbol('next'); @@ -27,8 +26,14 @@ const prevInListSymbol = Symbol('prev'); const eventsSymbol = Symbol('events'); const resourcesSymbol = Symbol('resources'); +export type SourceLocation = { + file: string; + line: number; + source: SourceModel; +}; + export type SourceModel = { - errors: { error: SerializedError['error'], location: StackFrame }[]; + errors: { line: number, message: string }[]; content: string | undefined; }; @@ -203,7 +208,7 @@ function collectSources(actions: trace.ActionTraceEvent[]): Map, hideStackFrames?: boolean, rootDir?: string, - fallbackLocation?: StackFrame, + fallbackLocation?: SourceLocation, }> = ({ action, sources, hideStackFrames, rootDir, fallbackLocation }) => { const [lastAction, setLastAction] = React.useState(); const [selectedFrame, setSelectedFrame] = React.useState(0); @@ -43,31 +42,34 @@ export const SourceTab: React.FunctionComponent<{ }, [action, lastAction, setLastAction, setSelectedFrame]); const { source, highlight, targetLine, fileName } = useAsyncMemo<{ source: SourceModel, targetLine?: number, fileName?: string, highlight: SourceHighlight[] }>(async () => { - const location = action?.stack?.[selectedFrame] || fallbackLocation; - if (!location?.file) - return { source: { errors: [], content: undefined }, targetLine: 0, highlight: [] }; + const actionLocation = action?.stack?.[selectedFrame]; + const shouldUseFallback = !actionLocation?.file; + if (shouldUseFallback && !shouldUseFallback) + return { source: { file: '', errors: [], content: undefined }, targetLine: 0, highlight: [] }; - let source = sources.get(location.file); + const file = shouldUseFallback ? fallbackLocation!.file : actionLocation.file; + let source = sources.get(file); // Fallback location can fall outside the sources model. if (!source) { - source = { errors: [], content: undefined }; - sources.set(location.file, source); + source = { errors: fallbackLocation?.source?.errors || [], content: undefined }; + sources.set(file, source); } - const targetLine = location.line || 0; - const fileName = rootDir && location.file.startsWith(rootDir) ? location.file.substring(rootDir.length + 1) : location.file; - const highlight: SourceHighlight[] = source.errors.map(e => ({ type: 'error', line: e.location.line, message: e.error!.message })); + const targetLine = shouldUseFallback ? fallbackLocation?.line || source.errors[0]?.line || 0 : actionLocation.line; + const fileName = rootDir && file.startsWith(rootDir) ? file.substring(rootDir.length + 1) : file; + const highlight: SourceHighlight[] = source.errors.map(e => ({ type: 'error', line: e.line, message: e.message })); highlight.push({ line: targetLine, type: 'running' }); - if (source.content === undefined || fallbackLocation) { - const sha1 = await calculateSha1(location.file); + // After the source update, but before the test run, don't trust the cache. + if (source.content === undefined || shouldUseFallback) { + const sha1 = await calculateSha1(file); try { let response = await fetch(`sha1/src@${sha1}.txt`); if (response.status === 404) - response = await fetch(`file?path=${location.file}`); + response = await fetch(`file?path=${file}`); source.content = await response.text(); } catch { - source.content = ``; + source.content = ``; } } return { source, highlight, targetLine, fileName }; diff --git a/packages/trace-viewer/src/ui/uiModeView.tsx b/packages/trace-viewer/src/ui/uiModeView.tsx index bc9f85bdc3..6cd85c514e 100644 --- a/packages/trace-viewer/src/ui/uiModeView.tsx +++ b/packages/trace-viewer/src/ui/uiModeView.tsx @@ -25,6 +25,7 @@ import type { TeleTestCase } from '@testIsomorphic/teleReceiver'; import type { FullConfig, Suite, TestCase, Location, TestError } from '@playwright/test/types/testReporter'; import { SplitView } from '@web/components/splitView'; import { idForAction, MultiTraceModel } from './modelUtil'; +import type { SourceLocation } from './modelUtil'; import './uiModeView.css'; import { ToolbarButton } from '@web/components/toolbarButton'; import { Toolbar } from '@web/components/toolbar'; @@ -37,7 +38,7 @@ import { artifactsFolderName } from '@testIsomorphic/folders'; import { msToString, settings, useSetting } from '@web/uiUtils'; import type { ActionTraceEvent } from '@trace/trace'; -let updateRootSuite: (config: FullConfig, rootSuite: Suite, progress: Progress | undefined) => void = () => {}; +let updateRootSuite: (config: FullConfig, rootSuite: Suite, loadErrors: TestError[], progress: Progress | undefined) => void = () => {}; let runWatchedTests = (fileNames: string[]) => {}; let xtermSize = { cols: 80, rows: 24 }; @@ -54,6 +55,7 @@ const xtermDataSource: XtermDataSource = { type TestModel = { config: FullConfig | undefined; rootSuite: Suite | undefined; + loadErrors: TestError[]; }; export const UIModeView: React.FC<{}> = ({ @@ -67,9 +69,9 @@ export const UIModeView: React.FC<{}> = ({ ['skipped', false], ])); const [projectFilters, setProjectFilters] = React.useState>(new Map()); - const [testModel, setTestModel] = React.useState({ config: undefined, rootSuite: undefined }); + const [testModel, setTestModel] = React.useState({ config: undefined, rootSuite: undefined, loadErrors: [] }); const [progress, setProgress] = React.useState(); - const [selectedItem, setSelectedItem] = React.useState<{ location?: Location, testCase?: TestCase }>({}); + const [selectedItem, setSelectedItem] = React.useState<{ testFile?: SourceLocation, testCase?: TestCase }>({}); const [visibleTestIds, setVisibleTestIds] = React.useState>(new Set()); const [isLoading, setIsLoading] = React.useState(false); const [runningState, setRunningState] = React.useState<{ testIds: Set, itemSelectedByUser?: boolean } | undefined>(); @@ -81,21 +83,21 @@ export const UIModeView: React.FC<{}> = ({ const inputRef = React.useRef(null); - const reloadTests = () => { + const reloadTests = React.useCallback(() => { setIsLoading(true); setWatchedTreeIds({ value: new Set() }); - updateRootSuite(baseFullConfig, new TeleSuite('', 'root'), undefined); + updateRootSuite(baseFullConfig, new TeleSuite('', 'root'), [], undefined); refreshRootSuite(true).then(() => { setIsLoading(false); }); - }; + }, []); React.useEffect(() => { inputRef.current?.focus(); reloadTests(); - }, []); + }, [reloadTests]); - updateRootSuite = (config: FullConfig, rootSuite: Suite, newProgress: Progress | undefined) => { + updateRootSuite = React.useCallback((config: FullConfig, rootSuite: Suite, loadErrors: TestError[], newProgress: Progress | undefined) => { const selectedProjects = config.configFile ? settings.getObject(config.configFile + ':projects', undefined) : undefined; for (const projectName of projectFilters.keys()) { if (!rootSuite.suites.find(s => s.title === projectName)) @@ -108,13 +110,13 @@ export const UIModeView: React.FC<{}> = ({ if (!selectedProjects && projectFilters.size && ![...projectFilters.values()].includes(true)) projectFilters.set(projectFilters.entries().next().value[0], true); - setTestModel({ config, rootSuite }); + setTestModel({ config, rootSuite, loadErrors }); setProjectFilters(new Map(projectFilters)); if (runningState && newProgress) setProgress({ ...newProgress, total: runningState.testIds.size }); else if (!newProgress) setProgress(undefined); - }; + }, [projectFilters, runningState]); const runTests = React.useCallback((mode: 'queue-if-busy' | 'bounce-if-busy', testIds: Set) => { if (mode === 'bounce-if-busy' && runningState) @@ -300,7 +302,7 @@ const TestList: React.FC<{ statusFilters: Map, projectFilters: Map, filterText: string, - testModel: { rootSuite: Suite | undefined, config: FullConfig | undefined }, + testModel: TestModel, runTests: (mode: 'bounce-if-busy' | 'queue-if-busy', testIds: Set) => void, runningState?: { testIds: Set, itemSelectedByUser?: boolean }, watchAll: boolean, @@ -308,7 +310,7 @@ const TestList: React.FC<{ setWatchedTreeIds: (ids: { value: Set }) => void, isLoading?: boolean, setVisibleTestIds: (testIds: Set) => void, - onItemSelected: (item: { testCase?: TestCase, location?: Location }) => void, + onItemSelected: (item: { testCase?: TestCase, testFile?: SourceLocation }) => void, requestedCollapseAllCount: number, }> = ({ statusFilters, projectFilters, filterText, testModel, runTests, runningState, watchAll, watchedTreeIds, setWatchedTreeIds, isLoading, onItemSelected, setVisibleTestIds, requestedCollapseAllCount }) => { const [treeState, setTreeState] = React.useState({ expandedItems: new Map() }); @@ -317,7 +319,7 @@ const TestList: React.FC<{ // Build the test tree. const { rootItem, treeItemMap, fileNames } = React.useMemo(() => { - let rootItem = createTree(testModel.rootSuite, projectFilters); + let rootItem = createTree(testModel.rootSuite, testModel.loadErrors, projectFilters); filterTree(rootItem, filterText, statusFilters, runningState?.testIds); sortAndPropagateStatus(rootItem); rootItem = shortenRoot(rootItem); @@ -375,15 +377,25 @@ const TestList: React.FC<{ // Compute selected item. const { selectedTreeItem } = React.useMemo(() => { const selectedTreeItem = selectedTreeItemId ? treeItemMap.get(selectedTreeItemId) : undefined; - const location = selectedTreeItem?.location; + let testFile: SourceLocation | undefined; + if (selectedTreeItem) { + testFile = { + file: selectedTreeItem.location.file, + line: selectedTreeItem.location.line, + source: { + errors: testModel.loadErrors.filter(e => e.location?.file === selectedTreeItem.location.file).map(e => ({ line: e.location!.line, message: e.message! })), + content: undefined, + } + }; + } let selectedTest: TestCase | undefined; if (selectedTreeItem?.kind === 'test') selectedTest = selectedTreeItem.test; else if (selectedTreeItem?.kind === 'case' && selectedTreeItem.tests.length === 1) selectedTest = selectedTreeItem.tests[0]; - onItemSelected({ testCase: selectedTest, location }); + onItemSelected({ testCase: selectedTest, testFile }); return { selectedTreeItem }; - }, [onItemSelected, selectedTreeItemId, treeItemMap]); + }, [onItemSelected, selectedTreeItemId, testModel, treeItemMap]); // Update watch all. React.useEffect(() => { @@ -471,12 +483,13 @@ const TestList: React.FC<{ runningState.itemSelectedByUser = true; setSelectedTreeItemId(treeItem.id); }} + isError={treeItem => treeItem.kind === 'group' ? treeItem.hasLoadErrors : false} autoExpandDepth={filterText ? 5 : 1} noItemsMessage={isLoading ? 'Loading\u2026' : 'No tests'} />; }; const TraceView: React.FC<{ - item: { location?: Location, testCase?: TestCase }, + item: { testFile?: SourceLocation, testCase?: TestCase }, rootDir?: string, }> = ({ item, rootDir }) => { const [model, setModel] = React.useState(); @@ -543,7 +556,7 @@ const TraceView: React.FC<{ rootDir={rootDir} initialSelection={initialSelection} onSelectionChanged={onSelectionChanged} - defaultSourceLocation={item.location} />; + fallbackLocation={item.testFile} />; }; declare global { @@ -555,15 +568,15 @@ declare global { let receiver: TeleReporterReceiver | undefined; let throttleTimer: NodeJS.Timeout | undefined; -let throttleData: { config: FullConfig, rootSuite: Suite, progress: Progress } | undefined; +let throttleData: { config: FullConfig, rootSuite: Suite, loadErrors: TestError[], progress: Progress } | undefined; const throttledAction = () => { clearTimeout(throttleTimer); throttleTimer = undefined; - updateRootSuite(throttleData!.config, throttleData!.rootSuite, throttleData!.progress); + updateRootSuite(throttleData!.config, throttleData!.rootSuite, throttleData!.loadErrors, throttleData!.progress); }; -const throttleUpdateRootSuite = (config: FullConfig, rootSuite: Suite, progress: Progress, immediate = false) => { - throttleData = { config, rootSuite, progress }; +const throttleUpdateRootSuite = (config: FullConfig, rootSuite: Suite, loadErrors: TestError[], progress: Progress, immediate = false) => { + throttleData = { config, rootSuite, loadErrors, progress }; if (immediate) throttledAction(); else if (!throttleTimer) @@ -575,6 +588,7 @@ const refreshRootSuite = (eraseResults: boolean): Promise => { return sendMessage('list', {}); let rootSuite: Suite; + let loadErrors: TestError[]; const progress: Progress = { passed: 0, failed: 0, @@ -583,21 +597,23 @@ const refreshRootSuite = (eraseResults: boolean): Promise => { let config: FullConfig; receiver = new TeleReporterReceiver(pathSeparator, { onBegin: (c: FullConfig, suite: Suite) => { - if (!rootSuite) + if (!rootSuite) { rootSuite = suite; + loadErrors = []; + } config = c; progress.passed = 0; progress.failed = 0; progress.skipped = 0; - throttleUpdateRootSuite(config, rootSuite, progress, true); + throttleUpdateRootSuite(config, rootSuite, loadErrors, progress, true); }, onEnd: () => { - throttleUpdateRootSuite(config, rootSuite, progress, true); + throttleUpdateRootSuite(config, rootSuite, loadErrors, progress, true); }, onTestBegin: () => { - throttleUpdateRootSuite(config, rootSuite, progress); + throttleUpdateRootSuite(config, rootSuite, loadErrors, progress); }, onTestEnd: (test: TestCase) => { @@ -607,11 +623,13 @@ const refreshRootSuite = (eraseResults: boolean): Promise => { ++progress.failed; else ++progress.passed; - throttleUpdateRootSuite(config, rootSuite, progress); + throttleUpdateRootSuite(config, rootSuite, loadErrors, progress); }, onError: (error: TestError) => { xtermDataSource.write((error.stack || error.value || '') + '\n'); + loadErrors.push(error); + throttleUpdateRootSuite(config, rootSuite, loadErrors, progress); }, }); receiver._setClearPreviousResultsWhenTestBegins(); @@ -708,6 +726,7 @@ type TreeItemBase = { type GroupItem = TreeItemBase & { kind: 'group'; subKind: 'folder' | 'file' | 'describe'; + hasLoadErrors: boolean; children: (TestCaseItem | GroupItem)[]; }; @@ -743,13 +762,14 @@ function getFileItem(rootItem: GroupItem, filePath: string[], isFile: boolean, f parent: parentFileItem, children: [], status: 'none', + hasLoadErrors: false, }; parentFileItem.children.push(fileItem); fileItems.set(fileName, fileItem); return fileItem; } -function createTree(rootSuite: Suite | undefined, projectFilters: Map): GroupItem { +function createTree(rootSuite: Suite | undefined, loadErrors: TestError[], projectFilters: Map): GroupItem { const filterProjects = [...projectFilters.values()].some(Boolean); const rootItem: GroupItem = { kind: 'group', @@ -761,6 +781,7 @@ function createTree(rootSuite: Suite | undefined, projectFilters: Map { @@ -778,6 +799,7 @@ function createTree(rootSuite: Suite | undefined, projectFilters: Map void, -}> = ({ model, hideTimelineBars, hideStackFrames, showSourcesFirst, rootDir, defaultSourceLocation, initialSelection, onSelectionChanged }) => { +}> = ({ model, hideTimelineBars, hideStackFrames, showSourcesFirst, rootDir, fallbackLocation, initialSelection, onSelectionChanged }) => { const [selectedAction, setSelectedAction] = React.useState(undefined); const [highlightedAction, setHighlightedAction] = React.useState(); const [selectedNavigatorTab, setSelectedNavigatorTab] = React.useState('actions'); @@ -85,7 +84,7 @@ export const Workbench: React.FunctionComponent<{ sources={sources} hideStackFrames={hideStackFrames} rootDir={rootDir} - fallbackLocation={defaultSourceLocation} /> + fallbackLocation={fallbackLocation} /> }; const consoleTab: TabbedPaneTabModel = { id: 'console', diff --git a/packages/web/src/components/listView.css b/packages/web/src/components/listView.css index 6dcc35b252..d14425a8c0 100644 --- a/packages/web/src/components/listView.css +++ b/packages/web/src/components/listView.css @@ -61,10 +61,6 @@ background-color: transparent !important; } -.list-view-content:focus .list-view-entry.error.selected { - outline: 1px solid var(--vscode-inputValidation-errorBorder); -} - .list-view-content:focus .list-view-entry.selected .codicon { color: var(--vscode-list-activeSelectionForeground) !important; } @@ -78,5 +74,4 @@ .list-view-entry.error { color: var(--vscode-list-errorForeground); - background-color: var(--vscode-inputValidation-errorBackground); } diff --git a/tests/playwright-test/ui-mode-test-source.spec.ts b/tests/playwright-test/ui-mode-test-source.spec.ts index f8b9d33f58..a2b36ded68 100644 --- a/tests/playwright-test/ui-mode-test-source.spec.ts +++ b/tests/playwright-test/ui-mode-test-source.spec.ts @@ -64,3 +64,39 @@ test('should show selected test in sources', async ({ runUITest }) => { page.locator('.CodeMirror .source-line-running'), ).toHaveText(`3 test('third', () => {});`); }); + +test('should show syntax errors in file', async ({ runUITest }) => { + const { page } = await runUITest({ + 'a.test.ts': ` + import { test } from '@playwright/test'; + const a = 1; + a = 2; + test('first', () => {}); + test('second', () => {}); + `, + 'b.test.ts': ` + import { test } from '@playwright/test'; + test('third', () => {}); + `, + }); + await expect.poll(dumpTestTree(page)).toBe(` + ◯ a.test.ts + ▼ ◯ b.test.ts + ◯ third + `); + + 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(`4 a = 2;`); + + await expect( + page.locator('.CodeMirror-linewidget') + ).toHaveText([ + '            ', + 'Assignment to constant variable.' + ]); +});