Skip to content

Commit da07bbd

Browse files
plemarquandplemarquand
authored
Fix test explorer tests not updating on document modification (#1663)
* Fix test explorer tests not updating on document modification The LSP client was being created too early, before the imperative set of the `documentSymbolWatcher` on the client. Consequently we weren't making the `textDocument/tests` request when symbols updated in a test file.
1 parent 5572877 commit da07bbd

File tree

7 files changed

+224
-65
lines changed

7 files changed

+224
-65
lines changed

‎src/TestExplorer/TestExplorer.ts

Lines changed: 39 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -133,16 +133,31 @@ export class TestExplorer {
133133
*/
134134
static observeFolders(workspaceContext: WorkspaceContext): vscode.Disposable {
135135
const tokenSource = new vscode.CancellationTokenSource();
136-
const disposable = workspaceContext.onDidChangeFolders(
137-
({ folder, operation, workspace }) => {
138-
switch (operation) {
139-
case FolderOperation.add:
140-
if (folder) {
141-
void folder.swiftPackage.getTargets(TargetType.test).then(targets => {
142-
if (targets.length === 0) {
143-
return;
144-
}
136+
const disposable = workspaceContext.onDidChangeFolders(({ folder, operation }) => {
137+
switch (operation) {
138+
case FolderOperation.add:
139+
if (folder) {
140+
void folder.swiftPackage.getTargets(TargetType.test).then(targets => {
141+
if (targets.length === 0) {
142+
return;
143+
}
145144

145+
folder.addTestExplorer();
146+
// discover tests in workspace but only if disableAutoResolve is not on.
147+
// discover tests will kick off a resolve if required
148+
if (!configuration.folder(folder.workspaceFolder).disableAutoResolve) {
149+
void folder.testExplorer?.discoverTestsInWorkspace(
150+
tokenSource.token
151+
);
152+
}
153+
});
154+
}
155+
break;
156+
case FolderOperation.packageUpdated:
157+
if (folder) {
158+
void folder.swiftPackage.getTargets(TargetType.test).then(targets => {
159+
const hasTestTargets = targets.length > 0;
160+
if (hasTestTargets && !folder.hasTestExplorer()) {
146161
folder.addTestExplorer();
147162
// discover tests in workspace but only if disableAutoResolve is not on.
148163
// discover tests will kick off a resolve if required
@@ -153,43 +168,15 @@ export class TestExplorer {
153168
tokenSource.token
154169
);
155170
}
156-
});
157-
}
158-
break;
159-
case FolderOperation.packageUpdated:
160-
if (folder) {
161-
void folder.swiftPackage.getTargets(TargetType.test).then(targets => {
162-
const hasTestTargets = targets.length > 0;
163-
if (hasTestTargets && !folder.hasTestExplorer()) {
164-
folder.addTestExplorer();
165-
// discover tests in workspace but only if disableAutoResolve is not on.
166-
// discover tests will kick off a resolve if required
167-
if (
168-
!configuration.folder(folder.workspaceFolder)
169-
.disableAutoResolve
170-
) {
171-
void folder.testExplorer?.discoverTestsInWorkspace(
172-
tokenSource.token
173-
);
174-
}
175-
} else if (!hasTestTargets && folder.hasTestExplorer()) {
176-
folder.removeTestExplorer();
177-
} else if (folder.hasTestExplorer()) {
178-
folder.refreshTestExplorer();
179-
}
180-
});
181-
}
182-
break;
183-
case FolderOperation.focus:
184-
if (folder) {
185-
const languageClientManager =
186-
workspace.languageClientManager.get(folder);
187-
languageClientManager.documentSymbolWatcher = (document, symbols) =>
188-
TestExplorer.onDocumentSymbols(folder, document, symbols);
189-
}
190-
}
171+
} else if (!hasTestTargets && folder.hasTestExplorer()) {
172+
folder.removeTestExplorer();
173+
} else if (folder.hasTestExplorer()) {
174+
folder.refreshTestExplorer();
175+
}
176+
});
177+
}
191178
}
192-
);
179+
});
193180
return {
194181
dispose: () => {
195182
tokenSource.dispose();
@@ -212,7 +199,7 @@ export class TestExplorer {
212199
/**
213200
* Called whenever we have new document symbols
214201
*/
215-
private static onDocumentSymbols(
202+
static onDocumentSymbols(
216203
folder: FolderContext,
217204
document: vscode.TextDocument,
218205
symbols: vscode.DocumentSymbol[] | null | undefined
@@ -225,14 +212,17 @@ export class TestExplorer {
225212
if (target && target.type === "test") {
226213
testExplorer.lspTestDiscovery
227214
.getDocumentTests(folder.swiftPackage, uri)
228-
.then(tests =>
215+
.then(tests => {
229216
TestDiscovery.updateTestsForTarget(
230217
testExplorer.controller,
231218
{ id: target.c99name, label: target.name },
232219
tests,
233220
uri
234-
)
235-
)
221+
);
222+
testExplorer.onTestItemsDidChangeEmitter.fire(
223+
testExplorer.controller
224+
);
225+
})
236226
// Fallback to parsing document symbols for XCTests only
237227
.catch(() => {
238228
const tests = parseTestsFromDocumentSymbols(

‎src/WorkspaceContext.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import { isValidWorkspaceFolder, searchForPackages } from "./utilities/workspace
3737
import { SwiftPluginTaskProvider } from "./tasks/SwiftPluginTaskProvider";
3838
import { SwiftTaskProvider } from "./tasks/SwiftTaskProvider";
3939
import { LLDBDebugConfigurationProvider } from "./debugger/debugAdapterFactory";
40+
import { TestExplorer } from "./TestExplorer/TestExplorer";
4041

4142
/**
4243
* Context for whole workspace. Holds array of contexts for each workspace folder
@@ -79,7 +80,11 @@ export class WorkspaceContext implements vscode.Disposable {
7980
) {
8081
this.statusItem = new StatusItem();
8182
this.buildStatus = new SwiftBuildStatus(this.statusItem);
82-
this.languageClientManager = new LanguageClientToolchainCoordinator(this);
83+
this.languageClientManager = new LanguageClientToolchainCoordinator(this, {
84+
onDocumentSymbols: (folder, document, symbols) => {
85+
TestExplorer.onDocumentSymbols(folder, document, symbols);
86+
},
87+
});
8388
this.tasks = new TaskManager(this);
8489
this.diagnostics = new DiagnosticsManager(this);
8590
this.taskProvider = new SwiftTaskProvider(this);

‎src/sourcekit-lsp/LanguageClientManager.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,17 @@ import { LSPActiveDocumentManager } from "./didChangeActiveDocument";
4242
import { DidChangeActiveDocumentNotification } from "./extensions/DidChangeActiveDocumentRequest";
4343
import { lspClientOptions } from "./LanguageClientConfiguration";
4444

45+
interface LanguageClientManageOptions {
46+
/**
47+
* Options for the LanguageClientManager
48+
*/
49+
onDocumentSymbols?: (
50+
folder: FolderContext,
51+
document: vscode.TextDocument,
52+
symbols: vscode.DocumentSymbol[] | null | undefined
53+
) => void;
54+
}
55+
4556
/**
4657
* Manages the creation and destruction of Language clients as we move between
4758
* workspace folders
@@ -101,6 +112,7 @@ export class LanguageClientManager implements vscode.Disposable {
101112

102113
constructor(
103114
public folderContext: FolderContext,
115+
private options: LanguageClientManageOptions = {},
104116
private languageClientFactory: LanguageClientFactory = new LanguageClientFactory()
105117
) {
106118
this.namedOutputChannels.set(
@@ -427,7 +439,9 @@ export class LanguageClientManager implements vscode.Disposable {
427439
workspaceFolder,
428440
this.activeDocumentManager,
429441
errorHandler,
430-
this.documentSymbolWatcher
442+
(document, symbols) => {
443+
this.options.onDocumentSymbols?.(this.folderContext, document, symbols);
444+
}
431445
);
432446

433447
return {

‎src/sourcekit-lsp/LanguageClientToolchainCoordinator.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,13 @@ export class LanguageClientToolchainCoordinator implements vscode.Disposable {
3232

3333
public constructor(
3434
workspaceContext: WorkspaceContext,
35+
private options: {
36+
onDocumentSymbols?: (
37+
folder: FolderContext,
38+
document: vscode.TextDocument,
39+
symbols: vscode.DocumentSymbol[] | null | undefined
40+
) => void;
41+
} = {},
3542
languageClientFactory: LanguageClientFactory = new LanguageClientFactory() // used for testing only
3643
) {
3744
this.subscriptions.push(
@@ -124,7 +131,7 @@ export class LanguageClientToolchainCoordinator implements vscode.Disposable {
124131
const versionString = folder.swiftVersion.toString();
125132
let client = this.clients.get(versionString);
126133
if (!client) {
127-
client = new LanguageClientManager(folder, languageClientFactory);
134+
client = new LanguageClientManager(folder, this.options, languageClientFactory);
128135
this.clients.set(versionString, client);
129136
// Callers that must restart when switching folders will call setLanguageClientFolder themselves.
130137
if (singleServerSupport) {

‎test/integration-tests/testexplorer/TestExplorerIntegration.test.ts

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
assertContainsTrimmed,
2424
assertTestControllerHierarchy,
2525
assertTestResults,
26+
buildStateFromController,
2627
eventPromise,
2728
gatherTests,
2829
runTest,
@@ -50,6 +51,7 @@ import { executeTaskAndWaitForResult } from "../../utilities/tasks";
5051
import { createBuildAllTask } from "../../../src/tasks/SwiftTaskProvider";
5152
import { FolderContext } from "../../../src/FolderContext";
5253
import { lineBreakRegex } from "../../../src/utilities/tasks";
54+
import { randomString } from "../../../src/utilities/utilities";
5355

5456
suite("Test Explorer Suite", function () {
5557
const MAX_TEST_RUN_TIME_MINUTES = 6;
@@ -81,6 +83,126 @@ suite("Test Explorer Suite", function () {
8183
requiresDebugger: true,
8284
});
8385

86+
suite("Modifying", function () {
87+
let sourceFile: string;
88+
let originalSource: string;
89+
90+
suiteSetup(function () {
91+
if (
92+
(process.platform === "win32" &&
93+
workspaceContext.globalToolchainSwiftVersion.isLessThan(
94+
new Version(6, 1, 0)
95+
)) ||
96+
workspaceContext.globalToolchainSwiftVersion.isLessThan(new Version(6, 0, 2))
97+
) {
98+
this.skip();
99+
}
100+
101+
// FIXME: Both Linux and Windows aren't triggering the onTestItemsDidChange event
102+
// at the expected time for these tests.
103+
this.skip();
104+
});
105+
106+
beforeEach(() => {
107+
sourceFile = path.join(
108+
folderContext.folder.fsPath,
109+
"Tests",
110+
"PackageTests",
111+
"PackageTests.swift"
112+
);
113+
originalSource = fs.readFileSync(sourceFile, "utf8");
114+
});
115+
116+
async function appendSource(newContent: string) {
117+
const document = await vscode.workspace.openTextDocument(sourceFile);
118+
await vscode.window.showTextDocument(document);
119+
const edit = new vscode.WorkspaceEdit();
120+
const lastLine = document.lineAt(document.lineCount - 1);
121+
edit.insert(document.uri, lastLine.range.end, newContent);
122+
await vscode.workspace.applyEdit(edit);
123+
return document;
124+
}
125+
126+
async function setSource(content: string) {
127+
const document = await vscode.workspace.openTextDocument(sourceFile);
128+
await vscode.window.showTextDocument(document);
129+
const edit = new vscode.WorkspaceEdit();
130+
edit.replace(
131+
document.uri,
132+
document.validateRange(new vscode.Range(0, 0, 10000000, 0)),
133+
content
134+
);
135+
await vscode.workspace.applyEdit(edit);
136+
return document;
137+
}
138+
139+
type TestHierarchy = string | TestHierarchy[];
140+
141+
// Because we're at the whim of how often VS Code/the LSP provide document symbols
142+
// we can't assume that changes to test items will be reflected in the next onTestItemsDidChange
143+
// so poll until the condition is met.
144+
async function validate(validator: (testItems: TestHierarchy) => boolean) {
145+
let testItems: TestHierarchy = [];
146+
const startTime = Date.now();
147+
while (Date.now() - startTime < 5000) {
148+
testItems = buildStateFromController(testExplorer.controller.items);
149+
if (validator(testItems)) {
150+
return;
151+
}
152+
await new Promise(resolve => setTimeout(resolve, 100));
153+
}
154+
assert.fail("Expected test items to be updated, but they were not: " + testItems);
155+
}
156+
157+
test("Test explorer updates when a test is added and removed", async () => {
158+
const testName = `newTest${randomString()}()`;
159+
const newTest = `\n@Test func ${testName} {\n #expect(1 == 1)\n}\n`;
160+
161+
console.log(
162+
">>> Appending new test to the source file and waiting for test items to change"
163+
);
164+
await Promise.all([
165+
eventPromise(testExplorer.onTestItemsDidChange),
166+
appendSource(newTest),
167+
]);
168+
169+
console.log(">>> Validating that the new test appears in the test items");
170+
await validate(testItems => testItems[1].includes(testName));
171+
172+
console.log(
173+
">>> Restoring the original source file and waiting for test items to change"
174+
);
175+
await Promise.all([
176+
eventPromise(testExplorer.onTestItemsDidChange),
177+
setSource(originalSource),
178+
]);
179+
180+
console.log(">>> Validating that the new test no longer appears in the test items");
181+
await validate(testItems => !testItems[1].includes(testName));
182+
});
183+
184+
test("Test explorer updates when a suite is added and removed", async () => {
185+
const suiteName = `newSuite${randomString()}`;
186+
const newSuite = `\n@Suite\nstruct ${suiteName} {\n @Test\n func testPassing() throws {\n #expect(1 == 1)\n }\n}\n`;
187+
await Promise.all([
188+
eventPromise(testExplorer.onTestItemsDidChange),
189+
appendSource(newSuite),
190+
]);
191+
await validate(testItems => testItems[1].includes(suiteName));
192+
193+
await Promise.all([
194+
eventPromise(testExplorer.onTestItemsDidChange),
195+
setSource(originalSource),
196+
]);
197+
await validate(testItems => !testItems[1].includes(suiteName));
198+
});
199+
200+
afterEach(async () => {
201+
const document = await setSource(originalSource);
202+
await document.save();
203+
});
204+
});
205+
84206
suite("Debugging", function () {
85207
async function runXCTest() {
86208
const suiteId = "PackageTests.PassingXCTestSuite";

‎test/integration-tests/testexplorer/utilities.ts

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,19 @@ export function testExplorerFor(
8080

8181
type TestHierarchy = string | TestHierarchy[];
8282

83+
/**
84+
* Builds a tree of text items from a TestItemCollection
85+
*/
86+
export const buildStateFromController = (items: vscode.TestItemCollection): TestHierarchy =>
87+
reduceTestItemChildren(
88+
items,
89+
(acc, item) => {
90+
const children = buildStateFromController(item.children);
91+
return [...acc, item.label, ...(children.length ? [children] : [])];
92+
},
93+
[] as TestHierarchy
94+
);
95+
8396
/**
8497
* Asserts that the test item hierarchy matches the description provided by a collection
8598
* of `TestControllerState`s.
@@ -88,16 +101,6 @@ export function assertTestControllerHierarchy(
88101
controller: vscode.TestController,
89102
state: TestHierarchy
90103
) {
91-
const buildStateFromController = (items: vscode.TestItemCollection): TestHierarchy =>
92-
reduceTestItemChildren(
93-
items,
94-
(acc, item) => {
95-
const children = buildStateFromController(item.children);
96-
return [...acc, item.label, ...(children.length ? [children] : [])];
97-
},
98-
[] as TestHierarchy
99-
);
100-
101104
assert.deepEqual(
102105
buildStateFromController(controller.items),
103106
state,

0 commit comments

Comments
 (0)