From c8ebe2e763dfcf3fc01e28e7d81a308e8af5d815 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Thu, 4 Apr 2024 13:44:38 -0700 Subject: [PATCH] chore: wire-only watch implementation --- src/extension.ts | 40 +++--- src/playwrightTestCLI.ts | 3 + src/playwrightTestServer.ts | 12 ++ src/testModel.ts | 30 +++-- src/upstream/testServerConnection.ts | 7 ++ tests/mock/vscode.ts | 2 + tests/watch.spec.ts | 182 +++++++++++++++++---------- 7 files changed, 185 insertions(+), 91 deletions(-) diff --git a/src/extension.ts b/src/extension.ts index 720fb1b61..0739d1f09 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -74,7 +74,7 @@ export class Extension implements RunHooks { private _runProfile: vscodeTypes.TestRunProfile; private _debugProfile: vscodeTypes.TestRunProfile; private _playwrightTestLog: string[] = []; - private _runQueue = Promise.resolve(); + private _commandQueue = Promise.resolve(); constructor(vscode: vscodeTypes.VSCode) { this._vscode = vscode; @@ -240,7 +240,7 @@ export class Extension implements RunHooks { } private async _rebuildModels(userGesture: boolean): Promise { - this._runQueue = Promise.resolve(); + this._commandQueue = Promise.resolve(); this._models.clear(); this._testTree.startedLoading(); @@ -316,29 +316,20 @@ export class Extension implements RunHooks { if (this._testRun && !request.continuous) return; + await this._queueTestRun(request.include, isDebug ? 'debug' : 'run'); + if (request.continuous) { for (const model of this._models.enabledModels()) model.addToWatch(request.include, cancellationToken!); - return; } - - await this._queueTestRun(request.include, isDebug ? 'debug' : 'run'); } private async _queueTestRun(include: readonly vscodeTypes.TestItem[] | undefined, mode: 'run' | 'debug' | 'watch') { - this._runQueue = this._runQueue.then(async () => { - await this._runTests(include, mode).catch(e => console.error(e)); - }); - await this._runQueue; + await this._queueCommand(() => this._runTests(include, mode), undefined); } private async _queueGlobalHooks(type: 'setup' | 'teardown'): Promise { - let result: reporterTypes.FullResult['status'] = 'passed'; - this._runQueue = this._runQueue.then(async () => { - result = await this._runGlobalHooks(type); - }).catch(e => console.error(e)); - await this._runQueue; - return result; + return await this._queueCommand(() => this._runGlobalHooks(type), 'failed'); } private async _runGlobalHooks(type: 'setup' | 'teardown') { @@ -421,8 +412,11 @@ export class Extension implements RunHooks { } private async _workspaceChanged(change: WorkspaceChange) { - for (const model of this._models.enabledModels()) - await model.workspaceChanged(change); + await this._queueCommand(async () => { + for (const model of this._models.enabledModels()) + await model.handleWorkspaceChange(change); + }, undefined); + // Workspace change can be deferred, make sure editors are // decorated. await this._updateVisibleEditorItems(); @@ -576,8 +570,10 @@ export class Extension implements RunHooks { } private async _ensureTestsInAllModels(inputFiles: string[]): Promise { - for (const model of this._models.enabledModels()) - await model.ensureTests(inputFiles); + await this._queueCommand(async () => { + for (const model of this._models.enabledModels()) + await model.ensureTests(inputFiles); + }, undefined); } private _updateDiagnostics() { @@ -754,6 +750,12 @@ export class Extension implements RunHooks { if (testModel) this._traceViewer.open(traceUrl, testModel.config); } + + private _queueCommand(callback: () => Promise, defaultValue: T): Promise { + const result = this._commandQueue.then(callback).catch(e => { console.error(e); return defaultValue; }); + this._commandQueue = result.then(() => {}); + return result; + } } function parseLocationFromStack(stack: string | undefined, testItem?: vscodeTypes.TestItem): reporterTypes.Location | undefined { diff --git a/src/playwrightTestCLI.ts b/src/playwrightTestCLI.ts index 5ec01a2e4..3a1e5bbe4 100644 --- a/src/playwrightTestCLI.ts +++ b/src/playwrightTestCLI.ts @@ -194,6 +194,9 @@ export class PlaywrightTestCLI { } } + async watchFiles(fileNames: string[]) { + } + async findRelatedTestFiles(files: string[]): Promise { const configFolder = path.dirname(this._model.config.configFile); const configFile = path.basename(this._model.config.configFile); diff --git a/src/playwrightTestServer.ts b/src/playwrightTestServer.ts index 5e776a586..365546158 100644 --- a/src/playwrightTestServer.ts +++ b/src/playwrightTestServer.ts @@ -250,6 +250,13 @@ export class PlaywrightTestServer { } } + async watchFiles(fileNames: string[]) { + const testServer = await this._testServer(); + if (!testServer) + return; + await testServer.watch({ fileNames }); + } + async findRelatedTestFiles(files: string[]): Promise { const testServer = await this._testServer(); if (!testServer) @@ -286,6 +293,7 @@ export class PlaywrightTestServer { if (!wsEndpoint) return null; const testServer = new TestServerConnection(wsEndpoint); + testServer.onTestFilesChanged(params => this._testFilesChanged(params.testFiles)); await testServer.initialize({ serializer: require.resolve('./oopReporter'), interceptStdio: true, @@ -313,6 +321,10 @@ export class PlaywrightTestServer { }); } + private _testFilesChanged(testFiles: string[]) { + this._model.testFilesChanged(testFiles.map(f => this._vscode.Uri.file(f).fsPath)); + } + private _disposeTestServer() { const testServer = this._testServerPromise; this._testServerPromise = undefined; diff --git a/src/testModel.ts b/src/testModel.ts index be8540518..58bdfcf28 100644 --- a/src/testModel.ts +++ b/src/testModel.ts @@ -238,7 +238,7 @@ export class TestModel { project.suite.suites = [...files.values()]; } - async workspaceChanged(change: WorkspaceChange) { + async handleWorkspaceChange(change: WorkspaceChange) { const testDirs = [...new Set([...this._projects.values()].map(p => p.project.testDir))]; const changed = this._mapFilesToSources(testDirs, change.changed); @@ -253,16 +253,13 @@ export class TestModel { this._filesWithListedTests.delete(c); await this.ensureTests(changedWithListedTests); } + } + testFilesChanged(testFiles: string[]) { if (!this._watches.size) return; - if (!changed.length) - return; - - const result = await this._playwrightTest.findRelatedTestFiles([...change.changed]); - if (!result.testFiles.length) + if (!testFiles.length) return; - const testFiles = result.testFiles.map(f => this._vscode.Uri.file(f).fsPath); const files: string[] = []; const items: vscodeTypes.TestItem[] = []; @@ -492,7 +489,7 @@ export class TestModel { return [...result]; } - addToWatch(include: readonly vscodeTypes.TestItem[] | undefined, cancellationToken: vscodeTypes.CancellationToken) { + async addToWatch(include: readonly vscodeTypes.TestItem[] | undefined, cancellationToken: vscodeTypes.CancellationToken) { const watch: Watch = { include }; this._watches.add(watch); cancellationToken.onCancellationRequested(() => this._watches.delete(watch)); @@ -509,6 +506,23 @@ export class TestModel { } } } + + const filesToWatch = new Set(); + for (const watch of this._watches) { + if (!watch.include) { + for (const project of this._projects.values()) { + for (const fileSuite of project.suite.suites) + filesToWatch.add(fileSuite.location!.file); + } + continue; + } + for (const include of watch.include) { + if (!include.uri) + continue; + filesToWatch.add(include.uri.fsPath); + } + } + await this._playwrightTest.watchFiles([...filesToWatch]); } narrowDownLocations(items: vscodeTypes.TestItem[]): { locations: string[] | null, testIds?: string[] } { diff --git a/src/upstream/testServerConnection.ts b/src/upstream/testServerConnection.ts index 5ff5523cd..691a871d5 100644 --- a/src/upstream/testServerConnection.ts +++ b/src/upstream/testServerConnection.ts @@ -51,6 +51,7 @@ export class TestServerConnection implements TestServerInterface, TestServerInte this._ws.addEventListener('message', event => { const message = JSON.parse(String(event.data)); const { id, result, error, method, params } = message; + this._debugLog('RECV>', message); if (id) { const callback = this._callbacks.get(id); if (!callback) @@ -82,6 +83,7 @@ export class TestServerConnection implements TestServerInterface, TestServerInte await this._connectedPromise; const id = ++this._lastId; const message = { id, method, params }; + this._debugLog('SEND>', message); this._ws.send(JSON.stringify(message)); return new Promise((resolve, reject) => { this._callbacks.set(id, { resolve, reject }); @@ -191,4 +193,9 @@ export class TestServerConnection implements TestServerInterface, TestServerInte } catch { } } + + _debugLog(name: string, message: any) { + if (process.env.DEBUG?.includes('pwtest:server')) + console.log(name, message.id,message.method); + } } diff --git a/tests/mock/vscode.ts b/tests/mock/vscode.ts index 3a11a10ed..1548981df 100644 --- a/tests/mock/vscode.ts +++ b/tests/mock/vscode.ts @@ -144,6 +144,8 @@ export class WorkspaceFolder { } async changeFile(file: string, content: string) { + // TODO: investigate why watch immediately followed by changeFile doesn't emit the event. + await new Promise(f => setTimeout(f, 1000)); const fsPath = path.join(this.uri.fsPath, file); await fs.promises.writeFile(fsPath, content); for (const watcher of this.vscode.fsWatchers) { diff --git a/tests/watch.spec.ts b/tests/watch.spec.ts index 947ba819b..118795e61 100644 --- a/tests/watch.spec.ts +++ b/tests/watch.spec.ts @@ -18,6 +18,8 @@ import { TestRun } from './mock/vscode'; import { escapedPathSep, expect, test } from './utils'; import path from 'path'; +test.skip(({ useTestServer }) => !useTestServer); + test.beforeEach(async ({ vscode }) => { const configuration = vscode.workspace.getConfiguration('playwright'); configuration.update('allowWatchingFiles', true); @@ -37,6 +39,7 @@ test('should watch all tests', async ({ activate }) => { }); await testController.watch(); + const [testRun] = await Promise.all([ new Promise(f => testController.onDidCreateTestRun(testRun => { testRun.onDidEnd(() => f(testRun)); @@ -49,22 +52,37 @@ test('should watch all tests', async ({ activate }) => { expect(testRun.renderLog()).toBe(` tests > test-1.spec.ts > should pass [2:0] + enqueued enqueued started passed `); - await expect(vscode).toHaveExecLog(` - > playwright list-files -c playwright.config.js - > playwright find-related-test-files -c playwright.config.js - > playwright test -c playwright.config.js tests/test-1.spec.ts - `); await expect(vscode).toHaveConnectionLog([ { method: 'listFiles', params: {} }, - { method: 'findRelatedTestFiles', params: { - files: [expect.stringContaining(`tests${path.sep}test-1.spec.ts`)] - } }, { method: 'runGlobalSetup', params: {} }, + { + method: 'runTests', + params: expect.objectContaining({ + locations: [], + testIds: undefined + }) + }, + { + method: 'watch', + params: expect.objectContaining({ + fileNames: [ + expect.stringContaining(`tests${path.sep}test-1.spec.ts`), + expect.stringContaining(`tests${path.sep}test-2.spec.ts`), + ], + }) + }, + { + method: 'listTests', + params: expect.objectContaining({ + locations: [expect.stringContaining(`tests${escapedPathSep}test-1\\.spec\\.ts`)], + }) + }, { method: 'runTests', params: expect.objectContaining({ @@ -98,16 +116,35 @@ test('should unwatch all tests', async ({ activate }) => { test('should pass', async () => {}); `); - // Workspace observer has setTimeout(0) for coalescing. await new Promise(f => setTimeout(f, 500)); expect(testRuns).toHaveLength(0); - await expect(vscode).toHaveExecLog(` - > playwright list-files -c playwright.config.js - `); await expect(vscode).toHaveConnectionLog([ { method: 'listFiles', params: {} }, + { method: 'runGlobalSetup', params: {} }, + { + method: 'runTests', + params: expect.objectContaining({ + locations: [], + testIds: undefined + }) + }, + { + method: 'watch', + params: expect.objectContaining({ + fileNames: [ + expect.stringContaining(`tests${path.sep}test-1.spec.ts`), + expect.stringContaining(`tests${path.sep}test-2.spec.ts`), + ], + }) + }, + { + method: 'listTests', + params: expect.objectContaining({ + locations: [expect.stringContaining(`tests${escapedPathSep}test-1\\.spec\\.ts`)], + }) + }, ]); }); @@ -125,9 +162,21 @@ test('should watch test file', async ({ activate }) => { }); const testItem2 = testController.findTestItems(/test-2/); - await testController.watch(testItem2); - const [testRun] = await Promise.all([ + new Promise(f => testController.onDidCreateTestRun(testRun => { + testRun.onDidEnd(() => f(testRun)); + })), + testController.watch(testItem2), + ]); + + expect(testRun.renderLog()).toBe(` + tests > test-2.spec.ts > should fail [2:0] + enqueued + started + failed + `); + + const [watchRun] = await Promise.all([ new Promise(f => testController.onDidCreateTestRun(testRun => { testRun.onDidEnd(() => f(testRun)); })), @@ -141,15 +190,16 @@ test('should watch test file', async ({ activate }) => { `), ]); - expect(testRun.renderLog()).toBe(` + expect(watchRun.renderLog()).toBe(` tests > test-2.spec.ts > should pass [2:0] + enqueued enqueued started passed `); }); -test.skip('should watch tests via helper', async ({ activate }) => { +test('should watch tests via helper', async ({ activate }) => { // This test requires nightly playwright. const { vscode, testController, workspaceFolder } = await activate({ 'playwright.config.js': `module.exports = { testDir: 'tests' }`, @@ -177,32 +227,40 @@ test.skip('should watch tests via helper', async ({ activate }) => { ]); expect(testRun.renderLog()).toBe(` - tests > test.spec.ts > should pass [2:0] + tests > test.spec.ts > should pass [3:0] + enqueued enqueued started failed `); - await expect(vscode).toHaveExecLog(` - > playwright list-files -c playwright.config.js - > playwright find-related-test-files -c playwright.config.js - > playwright test -c playwright.config.js tests/test.spec.ts - `); await expect(vscode).toHaveConnectionLog([ { method: 'listFiles', params: {} }, - { method: 'findRelatedTests', params: {} }, { method: 'runGlobalSetup', params: {} }, { method: 'runTests', params: expect.objectContaining({ - locations: [expect.stringContaining(`tests1${escapedPathSep}test\\.spec\\.ts`)], + locations: [], + testIds: undefined + }) + }, + { + method: 'watch', + params: expect.objectContaining({ + fileNames: [expect.stringContaining(`tests${path.sep}test.spec.ts`)], + }) + }, + { + method: 'runTests', + params: expect.objectContaining({ + locations: [expect.stringContaining(`tests${escapedPathSep}test\\.spec\\.ts`)], testIds: undefined }) }, ]); }); -test('should watch test in a file', async ({ activate }) => { +test('should watch one test in a file', async ({ activate }) => { const { vscode, testController, workspaceFolder } = await activate({ 'playwright.config.js': `module.exports = { testDir: 'tests' }`, 'tests/test.spec.ts': ` @@ -236,15 +294,6 @@ test('should watch test in a file', async ({ activate }) => { passed `); - // first --list is for expand - // second --list is for workspace change - await expect(vscode).toHaveExecLog(` - > playwright list-files -c playwright.config.js - > playwright test -c playwright.config.js --list --reporter=null tests/test.spec.ts - > playwright test -c playwright.config.js --list --reporter=null tests/test.spec.ts - > playwright find-related-test-files -c playwright.config.js - > playwright test -c playwright.config.js tests/test.spec.ts:3 - `); await expect(vscode).toHaveConnectionLog([ { method: 'listFiles', params: {} }, { @@ -253,26 +302,33 @@ test('should watch test in a file', async ({ activate }) => { locations: [expect.stringContaining(`tests${escapedPathSep}test\\.spec\\.ts`)], }) }, + { method: 'runGlobalSetup', params: {} }, { - method: 'listTests', + method: 'runTests', params: expect.objectContaining({ - locations: [expect.stringContaining(`tests${escapedPathSep}test\\.spec\\.ts`)], + locations: undefined, + testIds: [ + expect.any(String) + ] }) }, { - method: 'findRelatedTestFiles', - params: { - files: [expect.stringContaining(`tests${path.sep}test.spec.ts`)], - } + method: 'watch', + params: expect.objectContaining({ + fileNames: [expect.stringContaining(`tests${path.sep}test.spec.ts`)], + }) + }, + { + method: 'listTests', + params: expect.objectContaining({ + locations: [expect.stringContaining(`tests${escapedPathSep}test\\.spec\\.ts`)], + }) }, - { method: 'runGlobalSetup', params: {} }, { method: 'runTests', params: expect.objectContaining({ locations: undefined, - testIds: [ - expect.any(String) - ] + testIds: [expect.any(String)] }) }, ]); @@ -317,15 +373,6 @@ test('should watch two tests in a file', async ({ activate }) => { passed `); - // first --list is for expand - // second --list is for workspace change - await expect(vscode).toHaveExecLog(` - > playwright list-files -c playwright.config.js - > playwright test -c playwright.config.js --list --reporter=null tests/test.spec.ts - > playwright test -c playwright.config.js --list --reporter=null tests/test.spec.ts - > playwright find-related-test-files -c playwright.config.js - > playwright test -c playwright.config.js tests/test.spec.ts:3 tests/test.spec.ts:4 - `); await expect(vscode).toHaveConnectionLog([ { method: 'listFiles', params: {} }, { @@ -334,27 +381,34 @@ test('should watch two tests in a file', async ({ activate }) => { locations: [expect.stringContaining(`tests${escapedPathSep}test\\.spec\\.ts`)], }) }, + { method: 'runGlobalSetup', params: {} }, { - method: 'listTests', + method: 'runTests', params: expect.objectContaining({ - locations: [expect.stringContaining(`tests${escapedPathSep}test\\.spec\\.ts`)], + locations: undefined, + testIds: [ + expect.any(String), + expect.any(String), + ] }) }, { - method: 'findRelatedTestFiles', - params: { - files: [expect.stringContaining(`tests${path.sep}test.spec.ts`)], - } + method: 'watch', + params: expect.objectContaining({ + fileNames: [expect.stringContaining(`tests${path.sep}test.spec.ts`)], + }) + }, + { + method: 'listTests', + params: expect.objectContaining({ + locations: [expect.stringContaining(`tests${escapedPathSep}test\\.spec\\.ts`)], + }) }, - { method: 'runGlobalSetup', params: {} }, { method: 'runTests', params: expect.objectContaining({ locations: undefined, - testIds: [ - expect.any(String), - expect.any(String), - ] + testIds: [expect.any(String), expect.any(String)] }) }, ]);