From 9474880529d77f2c347d6ac7829e678702e41f48 Mon Sep 17 00:00:00 2001 From: ConnectDotz Date: Sun, 4 Feb 2024 18:19:45 -0500 Subject: [PATCH] fix active editor files sometimes got attached to the wrong workspace folder (#1108) * fix active editor files sometimes got attached to the wrong workspace folder * remove outdated comment * update comment --- README.md | 2 +- package.json | 2 +- src/JestExt/core.ts | 59 +++++++-------- src/TestResults/TestResultProvider.ts | 34 ++++++--- tests/JestExt/core.test.ts | 80 +++++++++++--------- tests/TestResults/TestResultProvider.test.ts | 60 +++++++++------ 6 files changed, 135 insertions(+), 102 deletions(-) diff --git a/README.md b/README.md index 5c388875..9734a64d 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,7 @@ You can see the full [features](#features) and learn more details in the [How-To Happy testing! ## Releases -- **Next** ([v6.1.1](https://github.com/jest-community/vscode-jest/releases/tag/v6.1.1-next)): [release note](release-notes/release-note-v6.md#v610-pre-release) +- **Next** ([v6.1.2](https://github.com/jest-community/vscode-jest/releases/tag/v6.1.2-next)): [release note](release-notes/release-note-v6.md#v610-pre-release) - **Current** ([v5.2.3](https://github.com/jest-community/vscode-jest/releases/tag/v5.2.3)): [release note](release-notes/release-note-v5.x.md#v523) - **Previous** ([v5.1.0](https://github.com/jest-community/vscode-jest/releases/tag/v5.1.0)): [release note](release-notes/release-note-v5.x.md#v510) diff --git a/package.json b/package.json index dd74cc15..04a04c6f 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "vscode-jest", "displayName": "Jest", "description": "Use Facebook's Jest With Pleasure.", - "version": "6.1.1", + "version": "6.1.2", "publisher": "Orta", "engines": { "vscode": "^1.68.1" diff --git a/src/JestExt/core.ts b/src/JestExt/core.ts index 6cfa2e0b..9d2337d0 100644 --- a/src/JestExt/core.ts +++ b/src/JestExt/core.ts @@ -280,9 +280,7 @@ export class JestExt { this.testProvider = new JestTestProvider(this.getExtExplorerContext()); this.resetStatusBar(); - vscode.window.visibleTextEditors.forEach((editor) => { - this.triggerUpdateActiveEditor(editor); - }); + this.updateVisibleTextEditors(); return; } @@ -310,9 +308,7 @@ export class JestExt { await this.updateTestFileList(); // update visible editors that belong to this folder - vscode.window.visibleTextEditors.forEach((editor) => { - this.triggerUpdateActiveEditor(editor); - }); + this.updateVisibleTextEditors(); } catch (e) { this.outputActionMessages( `Failed to start jest session: ${e}`, @@ -372,13 +368,8 @@ export class JestExt { updateCurrentDiagnostics(sortedResults.fail, this.failDiagnostics, editor); } - public triggerUpdateActiveEditor(editor: vscode.TextEditor): void { - // there is use case that the active editor is not in the workspace but is in jest test file list - if (!this.isInWorkspaceFolder(editor) && !this.isTestFileEditor(editor)) { - return; - } - this.coverageOverlay.updateVisibleEditors(); - + private triggerUpdateActiveEditor(editor: vscode.TextEditor): void { + this.coverageOverlay.update(editor); this.updateTestFileEditor(editor); } @@ -420,6 +411,25 @@ export class JestExt { await this.startSession(true); } + /** + * Updates the valid text editors based on the specified document. + * If a document is provided, it triggers an update for the active editor matches the document. + * If no document is provided, it triggers an update for all editors that are in the workspace folder + * + * @param document The document to match against the active editor. Optional. + */ + private updateVisibleTextEditors(document?: vscode.TextDocument): void { + vscode.window.visibleTextEditors.forEach((editor) => { + if (document) { + if (editor.document === document) { + this.triggerUpdateActiveEditor(editor); + } + } else if (this.isInWorkspaceFolder(editor)) { + this.triggerUpdateActiveEditor(editor); + } + }); + } + private isInWorkspaceFolder(editor: vscode.TextEditor): boolean { return isInFolder(editor.document.uri, this.extContext.workspace); } @@ -434,12 +444,7 @@ export class JestExt { return false; } - if (this.testResultProvider.isTestFile(editor.document.fileName) === 'no') { - return false; - } - - // if isTestFile returns unknown or true, treated it like a test file to give it best chance to display any test result if ever available - return true; + return this.testResultProvider.isTestFile(editor.document.fileName); } /** @@ -613,7 +618,7 @@ export class JestExt { } else { const name = editor.document.fileName; let pInfo; - if (this.testResultProvider.isTestFile(name) !== 'yes') { + if (!this.testResultProvider.isTestFile(name)) { // run related tests from source file pInfo = this.processSession.scheduleProcess({ type: 'by-file', @@ -670,14 +675,14 @@ export class JestExt { } const isTestFile = this.testResultProvider.isTestFile(document.fileName); - if (isTestFile === 'no' && this.extContext.settings.runMode.config.testFileOnly) { + if (!isTestFile && this.extContext.settings.runMode.config.testFileOnly) { // not a test file and configured not to re-run test for non-test files => mark the workspace dirty this.dirtyFiles.add(document.fileName); } else { this.processSession.scheduleProcess({ type: 'by-file', testFileName: document.fileName, - notTestFile: isTestFile !== 'yes', + notTestFile: !isTestFile, }); } } @@ -687,15 +692,7 @@ export class JestExt { * @param document refresh UI for the specific document. if undefined, refresh all active editors in the workspace. */ private refreshDocumentChange(document?: vscode.TextDocument): void { - for (const editor of vscode.window.visibleTextEditors) { - if ( - (document && editor.document === document) || - this.isInWorkspaceFolder(editor) || - this.isTestFileEditor(editor) - ) { - this.triggerUpdateActiveEditor(editor); - } - } + this.updateVisibleTextEditors(document); this.updateStatusBar({ stats: this.toSBStats(this.testResultProvider.getTestSuiteStats()), diff --git a/src/TestResults/TestResultProvider.ts b/src/TestResults/TestResultProvider.ts index 43244f22..fe268865 100644 --- a/src/TestResults/TestResultProvider.ts +++ b/src/TestResults/TestResultProvider.ts @@ -267,17 +267,28 @@ export class TestResultProvider { if (this.testFiles && this.testFiles.length > 0) { return this.testFiles; } - return Array.from(this.testSuites.keys()); + return Array.from(this.testSuites.keys()).filter((f) => this.isTestFile(f)); } - isTestFile(fileName: string): 'yes' | 'no' | 'maybe' { + isTestFile(fileName: string): boolean { if (this.testFiles?.includes(fileName) || this.testSuites.get(fileName)?.isTestFile) { - return 'yes'; + return true; } - if (!this.testFiles) { - return 'maybe'; + + //if we already have testFiles, then we can be certain that the file is not a test file + if (this.testFiles) { + return false; } - return 'no'; + + const _record = this.testSuites.get(fileName) ?? this.addTestSuiteRecord(fileName); + if (_record.isTestFile === false) { + return false; + } + + // check if the file is a test file by parsing the content + const isTestFile = _record.testBlocks !== 'failed'; + _record.update({ isTestFile }); + return isTestFile; } public getTestSuiteResult(filePath: string): TestSuiteResult | undefined { @@ -293,7 +304,8 @@ export class TestResultProvider { **/ private updateMatchedResults(filePath: string, record: TestSuiteRecord): void { let error: string | undefined; - // make sure we do not fire changeEvent since that will be proceeded with match or unmatch event anyway + let status = record.status; + // make sure we do not fire changeEvent since that will be proceeded with match or unmatched event anyway const testBlocks = record.testBlocks; if (testBlocks === 'failed') { record.update({ status: 'KnownFail', message: 'test file parse error', results: [] }); @@ -321,14 +333,16 @@ export class TestResultProvider { } catch (e) { console.warn(`failed to match test results for ${filePath}:`, e); error = `encountered internal match error: ${e}`; + status = 'KnownFail'; } } else { + // there might be many reasons for this, for example the test is not yet run, so leave it as unknown error = 'no assertion generated for file'; } // no need to do groupByRange as the source block will not have blocks under the same location record.update({ - status: 'KnownFail', + status, message: error, results: itBlocks.map((t) => match.toMatchResult(t, 'no assertion found', 'match-failed')), }); @@ -347,7 +361,7 @@ export class TestResultProvider { * @returns valid test result list or an empty array if the source file is not a test or can not be parsed. */ getResults(filePath: string, record?: TestSuiteRecord): TestResult[] | undefined { - if (this.isTestFile(filePath) === 'no') { + if (!this.isTestFile(filePath)) { return; } @@ -367,7 +381,7 @@ export class TestResultProvider { */ getSortedResults(filePath: string): SortedTestResults | undefined { - if (this.isTestFile(filePath) === 'no') { + if (!this.isTestFile(filePath)) { return; } diff --git a/tests/JestExt/core.test.ts b/tests/JestExt/core.test.ts index 4c066627..49f87b23 100644 --- a/tests/JestExt/core.test.ts +++ b/tests/JestExt/core.test.ts @@ -494,18 +494,15 @@ describe('JestExt', () => { describe('onDidSaveTextDocument', () => { describe('should handle onSave run', () => { it.each` - runConfig | languageId | isTestFile | shouldSchedule | isDirty - ${'on-demand'} | ${'javascript'} | ${'yes'} | ${false} | ${false} - ${'watch'} | ${'javascript'} | ${'yes'} | ${false} | ${false} - ${'on-save'} | ${'javascript'} | ${'no'} | ${true} | ${false} - ${'on-save'} | ${'javascript'} | ${'unknown'} | ${true} | ${false} - ${'on-save'} | ${'javascript'} | ${'yes'} | ${true} | ${false} - ${'on-save'} | ${'json'} | ${'no'} | ${false} | ${false} - ${{ type: 'on-save', testFileOnly: true }} | ${'javascript'} | ${'no'} | ${false} | ${true} - ${{ type: 'on-save', testFileOnly: true }} | ${'javascript'} | ${'unknown'} | ${true} | ${false} - ${{ type: 'on-save', testFileOnly: true }} | ${'javascript'} | ${'yes'} | ${true} | ${false} - ${{ type: 'on-save', testFileOnly: true }} | ${'javascript'} | ${'unknown'} | ${true} | ${false} - ${{ type: 'on-save', testFileOnly: true }} | ${'json'} | ${'unknown'} | ${false} | ${false} + runConfig | languageId | isTestFile | shouldSchedule | isDirty + ${'on-demand'} | ${'javascript'} | ${true} | ${false} | ${false} + ${'watch'} | ${'javascript'} | ${true} | ${false} | ${false} + ${'on-save'} | ${'javascript'} | ${false} | ${true} | ${false} + ${'on-save'} | ${'javascript'} | ${true} | ${true} | ${false} + ${'on-save'} | ${'json'} | ${false} | ${false} | ${false} + ${{ type: 'on-save', testFileOnly: true }} | ${'javascript'} | ${false} | ${false} | ${true} + ${{ type: 'on-save', testFileOnly: true }} | ${'javascript'} | ${true} | ${true} | ${false} + ${{ type: 'on-save', testFileOnly: true }} | ${'json'} | ${true} | ${false} | ${false} `( 'with runMode: $runConfig $languageId $isTestFile => $shouldSchedule, $isDirty', ({ runConfig, languageId, isTestFile, shouldSchedule, isDirty }) => { @@ -529,7 +526,7 @@ describe('JestExt', () => { expect.objectContaining({ type: 'by-file', testFileName: fileName, - notTestFile: isTestFile !== 'yes', + notTestFile: !isTestFile, }) ); } else { @@ -575,19 +572,19 @@ describe('JestExt', () => { }); }); - describe('triggerUpdateActiveEditor', () => { + describe('when active text editor changed', () => { beforeEach(() => { mockIsInFolder.mockReturnValueOnce(true); }); - it('should update the coverage overlay in visible editors', () => { + it('should update the coverage overlay in the given editor', () => { const editor: any = { document: { uri: 'whatever', languageId: 'javascript' } }; const sut = newJestExt(); - sut.triggerUpdateActiveEditor(editor); + sut.onDidChangeActiveTextEditor(editor); - expect(sut.coverageOverlay.updateVisibleEditors).toHaveBeenCalled(); + expect(sut.coverageOverlay.update).toHaveBeenCalled(); }); - it('should update both decorators and diagnostics for valid editor', () => { + it('should update both decorators and diagnostics for the given editor', () => { const sut = newJestExt(); const editor = mockEditor('file://a/b/c.ts'); @@ -597,7 +594,9 @@ describe('JestExt', () => { skip: [], unknown: [], }); - sut.triggerUpdateActiveEditor(editor); + (sut.testResultProvider.isTestFile as jest.Mocked).mockReturnValueOnce(true); + + sut.onDidChangeActiveTextEditor(editor); expect(updateCurrentDiagnostics).toHaveBeenCalled(); }); @@ -607,8 +606,9 @@ describe('JestExt', () => { (sut.testResultProvider.getSortedResults as jest.Mocked).mockImplementation(() => { throw new Error('force error'); }); + (sut.testResultProvider.isTestFile as jest.Mocked).mockReturnValueOnce(true); - sut.triggerUpdateActiveEditor(editor); + sut.onDidChangeActiveTextEditor(editor); expect(mockOutputTerminal.write).toHaveBeenCalledWith( expect.stringContaining('force error'), 'error' @@ -643,6 +643,7 @@ describe('JestExt', () => { ${'vue'} | ${false} `('if languageId=languageId => skip? $shouldSkip', ({ languageId, shouldSkip }) => { const editor = mockEditor('file', languageId); + (sut.testResultProvider.isTestFile as jest.Mocked).mockReturnValueOnce(true); sut.triggerUpdateActiveEditor(editor); if (shouldSkip) { expect(updateCurrentDiagnostics).not.toHaveBeenCalled(); @@ -652,10 +653,9 @@ describe('JestExt', () => { }); it.each` - isTestFile | shouldUpdate - ${'yes'} | ${true} - ${'no'} | ${false} - ${'unknown'} | ${true} + isTestFile | shouldUpdate + ${true} | ${true} + ${false} | ${false} `( 'isTestFile: $isTestFile => shouldUpdate? $shouldUpdate', async ({ isTestFile, shouldUpdate }) => { @@ -682,7 +682,7 @@ describe('JestExt', () => { const editor: any = { document: { uri: 'whatever' } }; (vscode.workspace.getWorkspaceFolder as jest.Mocked).mockReturnValue({}); const sut = newJestExt(); - sut.triggerUpdateActiveEditor(editor); + sut.onDidChangeActiveTextEditor(editor); expect(updateCurrentDiagnostics).not.toHaveBeenCalled(); }); }); @@ -705,7 +705,7 @@ describe('JestExt', () => { expect.objectContaining({ session: mockProcessSession }) ); }); - it('will refresh the visible editors, if any', async () => { + it('will refresh the visible editors for the given workspace, if any', async () => { const sut = createJestExt(); const spy = jest.spyOn(sut, 'triggerUpdateActiveEditor').mockImplementation(() => {}); @@ -719,9 +719,17 @@ describe('JestExt', () => { { document: { uri: 'whatever' }, }, + { + document: { uri: 'whatever' }, + }, + { + document: { uri: 'whatever' }, + }, ]; + + mockIsInFolder.mockReturnValueOnce(true).mockReturnValueOnce(true); await sut.startSession(); - expect(spy).toHaveBeenCalled(); + expect(spy).toHaveBeenCalledTimes(2); }); it('if failed to start session, show error message and quick fix link', async () => { mockProcessSession.start.mockReturnValueOnce(Promise.reject('forced error')); @@ -893,12 +901,11 @@ describe('JestExt', () => { }); }); it.each` - isTestFile | notTestFile - ${'yes'} | ${false} - ${'no'} | ${true} - ${'unknown'} | ${true} + isTestFile | notTestFile + ${true} | ${false} + ${false} | ${true} `( - 'treat unknown as notTestFile: isTestFile=$isTestFile => notTestFile=$notTestFile', + 'pass testFile status: isTestFile=$isTestFile => notTestFile=$notTestFile', async ({ isTestFile, notTestFile }) => { const sut = newJestExt(); const editor: any = { document: { fileName: 'whatever' } }; @@ -1402,6 +1409,7 @@ describe('JestExt', () => { doc = { uri: 'whatever', languageId: 'javascript' }; editor = { document: doc }; vscode.window.visibleTextEditors = [editor]; + mockIsInFolder.mockReturnValueOnce(true); }); it('will still create testProvider and parse test blocks while skipping the rest', async () => { expect.hasAssertions(); @@ -1410,13 +1418,15 @@ describe('JestExt', () => { const jestExt = newJestExt({ settings: { runMode } }); const validateJestCommandLineSpy = jest.spyOn(jestExt, 'validateJestCommandLine'); - - jestExt.triggerUpdateActiveEditor = jest.fn(); + (jestExt.testResultProvider.isTestFile as jest.Mocked).mockReturnValueOnce(true); + (jestExt.testResultProvider.getSortedResults as jest.Mocked).mockReturnValueOnce([]); await jestExt.startSession(); expect(JestTestProvider).toHaveBeenCalledTimes(1); - expect(jestExt.triggerUpdateActiveEditor).toHaveBeenCalledWith(editor); + expect(jestExt.testResultProvider.getSortedResults).toHaveBeenCalled(); + expect(jestExt.coverageOverlay.update).toHaveBeenCalled(); + expect(updateCurrentDiagnostics).toHaveBeenCalled(); expect(validateJestCommandLineSpy).not.toHaveBeenCalled(); expect(mockProcessSession.scheduleProcess).not.toHaveBeenCalled(); diff --git a/tests/TestResults/TestResultProvider.test.ts b/tests/TestResults/TestResultProvider.test.ts index 6c2429cb..2ec93b1f 100644 --- a/tests/TestResults/TestResultProvider.test.ts +++ b/tests/TestResults/TestResultProvider.test.ts @@ -937,31 +937,43 @@ describe('TestResultProvider', () => { mockReconciler.updateFileWithJestStatus.mockClear(); }); it.each` - testFiles | testResults | expected - ${undefined} | ${undefined} | ${'maybe'} - ${undefined} | ${['file-2']} | ${'maybe'} - ${[]} | ${[]} | ${'no'} - ${[]} | ${['file-1']} | ${'yes'} - ${[]} | ${['file-2']} | ${'no'} - ${['file-1']} | ${undefined} | ${'yes'} - ${['file-2']} | ${undefined} | ${'no'} - ${['file-1', 'file-2']} | ${undefined} | ${'yes'} - ${['file-1']} | ${['file-1']} | ${'yes'} - ${['file-2']} | ${['file-1']} | ${'yes'} - ${['file-2']} | ${['file-2']} | ${'no'} - `('$testFiles, $testResults => $expected', ({ testFiles, testResults, expected }) => { - const sut = new TestResultProvider(eventsMock); - if (testFiles) { - sut.updateTestFileList(testFiles); - } - if (testResults) { - const mockResults = testResults.map((file) => ({ file, status: 'KnownSuccess' })); - mockReconciler.updateFileWithJestStatus.mockReturnValueOnce(mockResults); - sut.updateTestResults({} as any, {} as any); - } + testFiles | testResults | hasTestBlocks | expected + ${undefined} | ${undefined} | ${false} | ${false} + ${undefined} | ${['file-2']} | ${true} | ${true} + ${undefined} | ${['file-1']} | ${true} | ${true} + ${[]} | ${[]} | ${true} | ${false} + ${[]} | ${['file-1']} | ${true} | ${true} + ${[]} | ${['file-1']} | ${false} | ${true} + ${[]} | ${['file-2']} | ${true} | ${false} + ${['file-1']} | ${undefined} | ${true} | ${true} + ${['file-1']} | ${undefined} | ${false} | ${true} + ${['file-2']} | ${undefined} | ${true} | ${false} + ${['file-1', 'file-2']} | ${undefined} | ${true} | ${true} + ${['file-1']} | ${['file-1']} | ${true} | ${true} + ${['file-2']} | ${['file-1']} | ${true} | ${true} + ${['file-2']} | ${['file-2']} | ${true} | ${false} + `( + '$testFiles, $testResults, $hasTestBlocks => $expected', + ({ testFiles, testResults, hasTestBlocks, expected }) => { + if (hasTestBlocks) { + const tBlock = helper.makeItBlock('a test', [8, 0, 20, 20]); + setupMockParse([tBlock]); + } else { + forceParseError(); + } + const sut = new TestResultProvider(eventsMock); + if (testFiles) { + sut.updateTestFileList(testFiles); + } + if (testResults) { + const mockResults = testResults.map((file) => ({ file, status: 'KnownSuccess' })); + mockReconciler.updateFileWithJestStatus.mockReturnValueOnce(mockResults); + sut.updateTestResults({} as any, {} as any); + } - expect(sut.isTestFile(target)).toEqual(expected); - }); + expect(sut.isTestFile(target)).toEqual(expected); + } + ); }); describe('snapshot', () => { const testPath = 'test-file';