Skip to content

Fix flaky activation test by polling instead of fixed delay #4283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cklin wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −6
Open

Fix flaky activation test by polling instead of fixed delay #4283

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions extensions/ql-vscode/test/vscode-tests/minimal-workspace/activation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,29 @@ describe("launching with a minimal workspace", () => {
});

it("should activate the extension when a .ql file is opened", async () => {
await delay();

const folders = workspace.workspaceFolders;
expect(folders?.length).toEqual(1);
const folderPath = folders![0].uri.fsPath;
const documentPath = resolve(folderPath, "query.ql");
const document = await workspace.openTextDocument(documentPath);
expect(document.languageId).toEqual("ql");
// Delay slightly so that the extension has time to activate.
await delay();
// Wait for the extension to activate, polling with a timeout.
await waitForActivation(ext, 30_000);
expect(ext!.isActive).toBeTruthy();
}, 60_000);

async function delay() {
await new Promise((resolve) => setTimeout(resolve, 4000));
async function waitForActivation(
extension: typeof ext,
timeoutMs: number,
): Promise<void> {
const pollIntervalMs = 100;
const maxAttempts = timeoutMs / pollIntervalMs;
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation 'timeoutMs / pollIntervalMs' on line 35 may result in a non-integer value if timeoutMs is not evenly divisible by pollIntervalMs. While the for loop will handle this by truncating to an integer, it means the actual timeout may be shorter than intended. Consider using 'Math.ceil(timeoutMs / pollIntervalMs)' to ensure the full timeout duration is respected.

Suggested change
const maxAttempts = timeoutMs / pollIntervalMs;
const maxAttempts = Math.ceil(timeoutMs / pollIntervalMs);

Copilot uses AI. Check for mistakes.
for (let i = 0; i < maxAttempts; i++) {
if (extension!.isActive) {
Comment on lines +34 to +37
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-null assertion operator on 'extension' (line 37) assumes the parameter is always defined, but the parameter type 'typeof ext' can be 'Extension | undefined'. Consider adding a check at the beginning of the function to validate that extension is defined, or update the function signature to ensure extension cannot be undefined.

Suggested change
const pollIntervalMs = 100;
const maxAttempts = timeoutMs / pollIntervalMs;
for (let i = 0; i < maxAttempts; i++) {
if (extension!.isActive) {
if (!extension) {
throw new Error("Extension is not defined.");
}
const pollIntervalMs = 100;
const maxAttempts = timeoutMs / pollIntervalMs;
for (let i = 0; i < maxAttempts; i++) {
if (extension.isActive) {

Copilot uses AI. Check for mistakes.
return;
}
await new Promise((resolve) => setTimeout(resolve, pollIntervalMs));
}
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The waitForActivation function should throw an error if the extension fails to activate within the timeout period. Currently, if the timeout is reached without activation, the function returns silently, and the test will fail on the subsequent assertion at line 27 with a less informative error message. Consider throwing a descriptive error after the loop completes to make timeout failures clearer and easier to diagnose.

Suggested change
}
}
throw new Error(
`Extension "GitHub.vscode-codeql" did not activate within ${timeoutMs} ms.`,
);

Copilot uses AI. Check for mistakes.
}
});

Expand Down
Loading