From 8412d973c03ecfce9c0dbabe0e3fec6d307cdc2d Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Tue, 30 Jul 2024 14:17:41 +0200 Subject: [PATCH] fix(ui): added test in watched file should be run (#31842) Closes https://github.com/microsoft/playwright/issues/22211 Currently, when the server notifies the UI about changed files, the UI determines what files to re-run based on an old test list. By listing tests before that, we make sure that the test list is up-to-date, and that added tests are included in the next run. I've also removed the `listChanged` event as discussed in the team sync. The event isn't used anywhere and fires in exactly the same cases where `testFilesChanged` fired, so i've folded them into one another. This allowed simplifying `Watcher`. --- packages/playwright-ct-core/src/devServer.ts | 4 +- packages/playwright/src/fsWatcher.ts | 22 ++--- .../src/isomorphic/testServerConnection.ts | 5 - .../src/isomorphic/testServerInterface.ts | 2 - packages/playwright/src/runner/testServer.ts | 37 ++++--- packages/trace-viewer/src/ui/uiModeView.tsx | 46 +++++---- .../playwright-test-fixtures.ts | 14 +++ .../test-server-connection.spec.ts | 97 +++++++++++++++++++ .../ui-mode-test-watch.spec.ts | 31 ++++++ 9 files changed, 202 insertions(+), 56 deletions(-) create mode 100644 tests/playwright-test/test-server-connection.spec.ts diff --git a/packages/playwright-ct-core/src/devServer.ts b/packages/playwright-ct-core/src/devServer.ts index 2e1e7c76bf..3e833fcd9c 100644 --- a/packages/playwright-ct-core/src/devServer.ts +++ b/packages/playwright-ct-core/src/devServer.ts @@ -61,7 +61,7 @@ export async function runDevServer(config: FullConfigInternal): Promise<() => Pr projectOutputs.add(p.project.outputDir); } - const globalWatcher = new Watcher('deep', async () => { + const globalWatcher = new Watcher(async () => { const registry: ComponentRegistry = new Map(); await populateComponentsFromTests(registry); // compare componentRegistry to registry key sets. @@ -80,6 +80,6 @@ export async function runDevServer(config: FullConfigInternal): Promise<() => Pr if (rootModule) devServer.moduleGraph.onFileChange(rootModule.file!); }); - globalWatcher.update([...projectDirs], [...projectOutputs], false); + await globalWatcher.update([...projectDirs], [...projectOutputs], false); return () => Promise.all([devServer.close(), globalWatcher.close()]).then(() => {}); } diff --git a/packages/playwright/src/fsWatcher.ts b/packages/playwright/src/fsWatcher.ts index a1c09c343d..53e6849b1a 100644 --- a/packages/playwright/src/fsWatcher.ts +++ b/packages/playwright/src/fsWatcher.ts @@ -21,26 +21,24 @@ export type FSEvent = { event: 'add' | 'addDir' | 'change' | 'unlink' | 'unlinkD export class Watcher { private _onChange: (events: FSEvent[]) => void; - private _watchedFiles: string[] = []; + private _watchedPaths: string[] = []; private _ignoredFolders: string[] = []; private _collector: FSEvent[] = []; private _fsWatcher: FSWatcher | undefined; private _throttleTimer: NodeJS.Timeout | undefined; - private _mode: 'flat' | 'deep'; - constructor(mode: 'flat' | 'deep', onChange: (events: FSEvent[]) => void) { - this._mode = mode; + constructor(onChange: (events: FSEvent[]) => void) { this._onChange = onChange; } - update(watchedFiles: string[], ignoredFolders: string[], reportPending: boolean) { - if (JSON.stringify([this._watchedFiles, this._ignoredFolders]) === JSON.stringify(watchedFiles, ignoredFolders)) + async update(watchedPaths: string[], ignoredFolders: string[], reportPending: boolean) { + if (JSON.stringify([this._watchedPaths, this._ignoredFolders]) === JSON.stringify(watchedPaths, ignoredFolders)) return; if (reportPending) this._reportEventsIfAny(); - this._watchedFiles = watchedFiles; + this._watchedPaths = watchedPaths; this._ignoredFolders = ignoredFolders; void this._fsWatcher?.close(); this._fsWatcher = undefined; @@ -48,20 +46,18 @@ export class Watcher { clearTimeout(this._throttleTimer); this._throttleTimer = undefined; - if (!this._watchedFiles.length) + if (!this._watchedPaths.length) return; const ignored = [...this._ignoredFolders, '**/node_modules/**']; - this._fsWatcher = chokidar.watch(watchedFiles, { ignoreInitial: true, ignored }).on('all', async (event, file) => { + this._fsWatcher = chokidar.watch(watchedPaths, { ignoreInitial: true, ignored }).on('all', async (event, file) => { if (this._throttleTimer) clearTimeout(this._throttleTimer); - if (this._mode === 'flat' && event !== 'add' && event !== 'change') - return; - if (this._mode === 'deep' && event !== 'add' && event !== 'change' && event !== 'unlink' && event !== 'addDir' && event !== 'unlinkDir') - return; this._collector.push({ event, file }); this._throttleTimer = setTimeout(() => this._reportEventsIfAny(), 250); }); + + await new Promise((resolve, reject) => this._fsWatcher!.once('ready', resolve).once('error', reject)); } async close() { diff --git a/packages/playwright/src/isomorphic/testServerConnection.ts b/packages/playwright/src/isomorphic/testServerConnection.ts index 156af26e8c..34ce0e31ef 100644 --- a/packages/playwright/src/isomorphic/testServerConnection.ts +++ b/packages/playwright/src/isomorphic/testServerConnection.ts @@ -23,14 +23,12 @@ export class TestServerConnection implements TestServerInterface, TestServerInte readonly onClose: events.Event; readonly onReport: events.Event; readonly onStdio: events.Event<{ type: 'stderr' | 'stdout'; text?: string | undefined; buffer?: string | undefined; }>; - readonly onListChanged: events.Event; readonly onTestFilesChanged: events.Event<{ testFiles: string[] }>; readonly onLoadTraceRequested: events.Event<{ traceUrl: string }>; private _onCloseEmitter = new events.EventEmitter(); private _onReportEmitter = new events.EventEmitter(); private _onStdioEmitter = new events.EventEmitter<{ type: 'stderr' | 'stdout'; text?: string | undefined; buffer?: string | undefined; }>(); - private _onListChangedEmitter = new events.EventEmitter(); private _onTestFilesChangedEmitter = new events.EventEmitter<{ testFiles: string[] }>(); private _onLoadTraceRequestedEmitter = new events.EventEmitter<{ traceUrl: string }>(); @@ -44,7 +42,6 @@ export class TestServerConnection implements TestServerInterface, TestServerInte this.onClose = this._onCloseEmitter.event; this.onReport = this._onReportEmitter.event; this.onStdio = this._onStdioEmitter.event; - this.onListChanged = this._onListChangedEmitter.event; this.onTestFilesChanged = this._onTestFilesChangedEmitter.event; this.onLoadTraceRequested = this._onLoadTraceRequestedEmitter.event; @@ -103,8 +100,6 @@ export class TestServerConnection implements TestServerInterface, TestServerInte this._onReportEmitter.fire(params); else if (method === 'stdio') this._onStdioEmitter.fire(params); - else if (method === 'listChanged') - this._onListChangedEmitter.fire(params); else if (method === 'testFilesChanged') this._onTestFilesChangedEmitter.fire(params); else if (method === 'loadTraceRequested') diff --git a/packages/playwright/src/isomorphic/testServerInterface.ts b/packages/playwright/src/isomorphic/testServerInterface.ts index 08214ee723..0460ae56d9 100644 --- a/packages/playwright/src/isomorphic/testServerInterface.ts +++ b/packages/playwright/src/isomorphic/testServerInterface.ts @@ -118,7 +118,6 @@ export interface TestServerInterface { export interface TestServerInterfaceEvents { onReport: Event; onStdio: Event<{ type: 'stdout' | 'stderr', text?: string, buffer?: string }>; - onListChanged: Event; onTestFilesChanged: Event<{ testFiles: string[] }>; onLoadTraceRequested: Event<{ traceUrl: string }>; } @@ -126,7 +125,6 @@ export interface TestServerInterfaceEvents { export interface TestServerInterfaceEventEmitters { dispatchEvent(event: 'report', params: ReportEntry): void; dispatchEvent(event: 'stdio', params: { type: 'stdout' | 'stderr', text?: string, buffer?: string }): void; - dispatchEvent(event: 'listChanged', params: {}): void; dispatchEvent(event: 'testFilesChanged', params: { testFiles: string[] }): void; dispatchEvent(event: 'loadTraceRequested', params: { traceUrl: string }): void; } diff --git a/packages/playwright/src/runner/testServer.ts b/packages/playwright/src/runner/testServer.ts index 559f95514a..5a498337af 100644 --- a/packages/playwright/src/runner/testServer.ts +++ b/packages/playwright/src/runner/testServer.ts @@ -64,8 +64,12 @@ class TestServer { class TestServerDispatcher implements TestServerInterface { private _configLocation: ConfigLocation; - private _globalWatcher: Watcher; - private _testWatcher: Watcher; + + private _watcher: Watcher; + private _watchedProjectDirs = new Set(); + private _ignoredProjectOutputs = new Set(); + private _watchedTestDependencies = new Set(); + private _testRun: { run: Promise, stop: ManualPromise } | undefined; readonly transport: Transport; private _queue = Promise.resolve(); @@ -86,8 +90,7 @@ class TestServerDispatcher implements TestServerInterface { gracefullyProcessExitDoNotHang(0); }, }; - this._globalWatcher = new Watcher('deep', () => this._dispatchEvent('listChanged', {})); - this._testWatcher = new Watcher('flat', events => { + this._watcher = new Watcher(events => { const collector = new Set(); events.forEach(f => collectAffectedTestFiles(f.file, collector)); this._dispatchEvent('testFilesChanged', { testFiles: [...collector] }); @@ -279,24 +282,28 @@ class TestServerDispatcher implements TestServerInterface { await taskRunner.reporter.onEnd({ status }); await taskRunner.reporter.onExit(); - const projectDirs = new Set(); - const projectOutputs = new Set(); + this._watchedProjectDirs = new Set(); + this._ignoredProjectOutputs = new Set(); for (const p of config.projects) { - projectDirs.add(p.project.testDir); - projectOutputs.add(p.project.outputDir); + this._watchedProjectDirs.add(p.project.testDir); + this._ignoredProjectOutputs.add(p.project.outputDir); } const result = await resolveCtDirs(config); if (result) { - projectDirs.add(result.templateDir); - projectOutputs.add(result.outDir); + this._watchedProjectDirs.add(result.templateDir); + this._ignoredProjectOutputs.add(result.outDir); } if (this._watchTestDirs) - this._globalWatcher.update([...projectDirs], [...projectOutputs], false); + await this.updateWatcher(false); return { report, status }; } + private async updateWatcher(reportPending: boolean) { + await this._watcher.update([...this._watchedProjectDirs, ...this._watchedTestDependencies], [...this._ignoredProjectOutputs], reportPending); + } + async runTests(params: Parameters[0]): ReturnType { let result: Awaited> = { status: 'passed' }; this._queue = this._queue.then(async () => { @@ -364,12 +371,12 @@ class TestServerDispatcher implements TestServerInterface { } async watch(params: { fileNames: string[]; }) { - const files = new Set(); + this._watchedTestDependencies = new Set(); for (const fileName of params.fileNames) { - files.add(fileName); - dependenciesForTestFile(fileName).forEach(file => files.add(file)); + this._watchedTestDependencies.add(fileName); + dependenciesForTestFile(fileName).forEach(file => this._watchedTestDependencies.add(file)); } - this._testWatcher.update([...files], [], true); + await this.updateWatcher(true); } async findRelatedTestFiles(params: Parameters[0]): ReturnType { diff --git a/packages/trace-viewer/src/ui/uiModeView.tsx b/packages/trace-viewer/src/ui/uiModeView.tsx index 7c1ed69905..2bd876f61b 100644 --- a/packages/trace-viewer/src/ui/uiModeView.tsx +++ b/packages/trace-viewer/src/ui/uiModeView.tsx @@ -94,6 +94,7 @@ export const UIModeView: React.FC<{}> = ({ const [isDisconnected, setIsDisconnected] = React.useState(false); const [hasBrowsers, setHasBrowsers] = React.useState(true); const [testServerConnection, setTestServerConnection] = React.useState(); + const [teleSuiteUpdater, setTeleSuiteUpdater] = React.useState(); const [settingsVisible, setSettingsVisible] = React.useState(false); const [testingOptionsVisible, setTestingOptionsVisible] = React.useState(false); const [revealSource, setRevealSource] = React.useState(false); @@ -191,20 +192,7 @@ export const UIModeView: React.FC<{}> = ({ pathSeparator, }); - const updateList = async () => { - commandQueue.current = commandQueue.current.then(async () => { - setIsLoading(true); - try { - const result = await testServerConnection.listTests({ projects: queryParams.projects, locations: queryParams.args, grep: queryParams.grep, grepInvert: queryParams.grepInvert }); - teleSuiteUpdater.processListReport(result.report); - } catch (e) { - // eslint-disable-next-line no-console - console.log(e); - } finally { - setIsLoading(false); - } - }); - }; + setTeleSuiteUpdater(teleSuiteUpdater); setTestModel(undefined); setIsLoading(true); @@ -223,7 +211,6 @@ export const UIModeView: React.FC<{}> = ({ const result = await testServerConnection.listTests({ projects: queryParams.projects, locations: queryParams.args, grep: queryParams.grep, grepInvert: queryParams.grepInvert }); teleSuiteUpdater.processListReport(result.report); - testServerConnection.onListChanged(updateList); testServerConnection.onReport(params => { teleSuiteUpdater.processTestReportEvent(params); }); @@ -336,11 +323,32 @@ export const UIModeView: React.FC<{}> = ({ }); }, [projectFilters, runningState, testModel, testServerConnection, runWorkers, runHeaded, runUpdateSnapshots]); - // Watch implementation. React.useEffect(() => { - if (!testServerConnection) + if (!testServerConnection || !teleSuiteUpdater) return; - const disposable = testServerConnection.onTestFilesChanged(params => { + const disposable = testServerConnection.onTestFilesChanged(async params => { + // fetch the new list of tests + commandQueue.current = commandQueue.current.then(async () => { + setIsLoading(true); + try { + const result = await testServerConnection.listTests({ projects: queryParams.projects, locations: queryParams.args, grep: queryParams.grep, grepInvert: queryParams.grepInvert }); + teleSuiteUpdater.processListReport(result.report); + } catch (e) { + // eslint-disable-next-line no-console + console.log(e); + } finally { + setIsLoading(false); + } + }); + await commandQueue.current; + + if (params.testFiles.length === 0) + return; + + // run affected watched tests + const testModel = teleSuiteUpdater.asModel(); + const testTree = new TestTree('', testModel.rootSuite, testModel.loadErrors, projectFilters, pathSeparator); + const testIds: string[] = []; const set = new Set(params.testFiles); if (watchAll) { @@ -363,7 +371,7 @@ export const UIModeView: React.FC<{}> = ({ runTests('queue-if-busy', new Set(testIds)); }); return () => disposable.dispose(); - }, [runTests, testServerConnection, testTree, watchAll, watchedTreeIds]); + }, [runTests, testServerConnection, watchAll, watchedTreeIds, teleSuiteUpdater, projectFilters]); // Shortcuts. React.useEffect(() => { diff --git a/tests/playwright-test/playwright-test-fixtures.ts b/tests/playwright-test/playwright-test-fixtures.ts index 86b07d7a80..f9670fa305 100644 --- a/tests/playwright-test/playwright-test-fixtures.ts +++ b/tests/playwright-test/playwright-test-fixtures.ts @@ -125,6 +125,10 @@ function startPlaywrightTest(childProcess: CommonFixtures['childProcess'], baseD ); if (options.additionalArgs) args.push(...options.additionalArgs); + return startPlaywrightChildProcess(childProcess, baseDir, args, env, options); +} + +function startPlaywrightChildProcess(childProcess: CommonFixtures['childProcess'], baseDir: string, args: string[], env: NodeJS.ProcessEnv, options: RunOptions): TestChildProcess { return childProcess({ command: ['node', cliEntrypoint, ...args], env: cleanEnv(env), @@ -246,6 +250,7 @@ type Fixtures = { deleteFile: (file: string) => Promise; runInlineTest: (files: Files, params?: Params, env?: NodeJS.ProcessEnv, options?: RunOptions) => Promise; runCLICommand: (files: Files, command: string, args?: string[], entryPoint?: string) => Promise<{ stdout: string, stderr: string, exitCode: number }>; + startCLICommand: (files: Files, command: string, args?: string[], options?: RunOptions) => Promise; runWatchTest: (files: Files, params?: Params, env?: NodeJS.ProcessEnv, options?: RunOptions) => Promise; interactWithTestRunner: (files: Files, params?: Params, env?: NodeJS.ProcessEnv, options?: RunOptions) => Promise; runTSC: (files: Files) => Promise; @@ -287,6 +292,15 @@ export const test = base await removeFolders([cacheDir]); }, + startCLICommand: async ({ childProcess }, use, testInfo: TestInfo) => { + const cacheDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'playwright-test-cache-')); + await use(async (files: Files, command: string, args?: string[], options: RunOptions = {}) => { + const baseDir = await writeFiles(testInfo, files, true); + return startPlaywrightChildProcess(childProcess, baseDir, [command, ...(args || [])], { PWTEST_CACHE_DIR: cacheDir }, options); + }); + await removeFolders([cacheDir]); + }, + runWatchTest: async ({ interactWithTestRunner }, use, testInfo: TestInfo) => { await use((files, params, env, options) => interactWithTestRunner(files, params, { ...env, PWTEST_WATCH: '1' }, options)); }, diff --git a/tests/playwright-test/test-server-connection.spec.ts b/tests/playwright-test/test-server-connection.spec.ts new file mode 100644 index 0000000000..af5f0223e5 --- /dev/null +++ b/tests/playwright-test/test-server-connection.spec.ts @@ -0,0 +1,97 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { test as baseTest, expect } from './ui-mode-fixtures'; + +import { TestServerConnection } from '../../packages/playwright/lib/isomorphic/testServerConnection'; + +class TestServerConnectionUnderTest extends TestServerConnection { + events: [string, any][] = []; + + constructor(wsUrl: string) { + super(wsUrl); + this.onTestFilesChanged(params => this.events.push(['testFilesChanged', params])); + this.onStdio(params => this.events.push(['stdio', params])); + this.onLoadTraceRequested(params => this.events.push(['loadTraceRequested', params])); + } +} + +const test = baseTest.extend<{ testServerConnection: TestServerConnectionUnderTest }>({ + testServerConnection: async ({ startCLICommand }, use, testInfo) => { + testInfo.skip(!globalThis.WebSocket, 'WebSocket not available prior to Node 22.4.0'); + + const testServerProcess = await startCLICommand({}, 'test-server'); + + await testServerProcess.waitForOutput('Listening on'); + const line = testServerProcess.output.split('\n').find(l => l.includes('Listening on')); + const wsEndpoint = line!.split(' ')[2]; + + await use(new TestServerConnectionUnderTest(wsEndpoint)); + + await testServerProcess.kill(); + } +}); + +test('file watching', async ({ testServerConnection, writeFiles }, testInfo) => { + await writeFiles({ + 'utils.ts': ` + export const expected = 42; + `, + 'a.test.ts': ` + import { test } from '@playwright/test'; + import { expected } from "./utils"; + test('foo', () => { + expect(123).toBe(expected); + }); + `, + }); + + const tests = await testServerConnection.listTests({}); + expect(tests.report.map(e => e.method)).toEqual(['onConfigure', 'onProject', 'onBegin', 'onEnd']); + + await testServerConnection.watch({ fileNames: [testInfo.outputPath('a.test.ts')] }); + + await writeFiles({ + 'utils.ts': ` + export const expected = 123; + `, + }); + + await expect.poll(() => testServerConnection.events).toHaveLength(1); + expect(testServerConnection.events).toEqual([ + ['testFilesChanged', { testFiles: [testInfo.outputPath('a.test.ts')] }] + ]); +}); + +test('stdio interception', async ({ testServerConnection, writeFiles }) => { + await testServerConnection.initialize({ interceptStdio: true }); + await writeFiles({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('foo', () => { + console.log("this goes to stdout"); + console.error("this goes to stderr"); + expect(true).toBe(true); + }); + `, + }); + + const tests = await testServerConnection.runTests({ trace: 'on' }); + expect(tests).toEqual({ status: 'passed' }); + await expect.poll(() => testServerConnection.events).toEqual(expect.arrayContaining([ + ['stdio', { type: 'stderr', text: 'this goes to stderr\n' }], + ['stdio', { type: 'stdout', text: 'this goes to stdout\n' }] + ])); +}); \ No newline at end of file diff --git a/tests/playwright-test/ui-mode-test-watch.spec.ts b/tests/playwright-test/ui-mode-test-watch.spec.ts index f248baaacd..bd04750a1f 100644 --- a/tests/playwright-test/ui-mode-test-watch.spec.ts +++ b/tests/playwright-test/ui-mode-test-watch.spec.ts @@ -220,6 +220,37 @@ test('should watch new file', async ({ runUITest, writeFiles }) => { `); }); +test('should run added test in watched file', async ({ runUITest, writeFiles }) => { + const { page } = await runUITest({ + 'a.test.ts': ` + import { test } from '@playwright/test'; + test('foo', () => {}); + `, + }); + + await page.getByText('a.test.ts').click(); + await page.getByRole('listitem').filter({ hasText: 'a.test.ts' }).getByTitle('Watch').click(); + + await expect.poll(dumpTestTree(page)).toBe(` + ▼ ◯ a.test.ts 👁 <= + ◯ foo + `); + + await writeFiles({ + 'a.test.ts': ` + import { test } from '@playwright/test'; + test('foo', () => {}); + test('bar', () => {}); + `, + }); + + await expect.poll(dumpTestTree(page)).toBe(` + ▼ ✅ a.test.ts 👁 <= + ✅ foo + ✅ bar + `); +}); + test('should queue watches', async ({ runUITest, writeFiles, createLatch }) => { const latch = createLatch(); const { page } = await runUITest({