Skip to content

Add retries of CLI integration tests #1801

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

Merged
koesie10 merged 3 commits into from
Nov 30, 2022
Merged

Add retries of CLI integration tests #1801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
644bea2
Add retries of CLI integration tests
koesie10 Nov 28, 2022
9a9a8c5
Fix multiple VSCode instances launching
koesie10 Nov 28, 2022
7ba9027
Increase scenarios in which retries will be used
koesie10 Nov 28, 2022
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
110 changes: 110 additions & 0 deletions extensions/ql-vscode/patches/jest-runner-vscode+3.0.1.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,113 @@ index 0663c5c..4991663 100644
const options = JSON.parse(PARENT_JEST_OPTIONS);
const jestOptions = [
...options.args,
diff --git a/node_modules/jest-runner-vscode/dist/public-types.d.ts b/node_modules/jest-runner-vscode/dist/public-types.d.ts
index 57716e5..d8614af 100644
--- a/node_modules/jest-runner-vscode/dist/public-types.d.ts
+++ b/node_modules/jest-runner-vscode/dist/public-types.d.ts
@@ -59,4 +59,5 @@ export interface RunnerOptions {
* code, or download progress. Defaults to `false`.
*/
quiet?: boolean;
+ retries?: number;
}
diff --git a/node_modules/jest-runner-vscode/dist/run-vscode.d.ts b/node_modules/jest-runner-vscode/dist/run-vscode.d.ts
index 8657ace..4d35409 100644
--- a/node_modules/jest-runner-vscode/dist/run-vscode.d.ts
+++ b/node_modules/jest-runner-vscode/dist/run-vscode.d.ts
@@ -16,5 +16,7 @@ export declare type RunVSCodeOptions = {
onFailure: JestRunner.OnTestFailure;
ipc: InstanceType<typeof IPC>;
quiet?: boolean;
+ attempt?: number;
+ maxRetries?: number;
};
-export default function runVSCode({ vscodePath, args, jestArgs, env, tests, globalConfig, filterOutput, onStart, onResult, onFailure, ipc, quiet, }: RunVSCodeOptions): Promise<void>;
+export default function runVSCode(options: RunVSCodeOptions): Promise<void>;
Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu Nov 29, 2022

Choose a reason for hiding this comment

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

On adding changes to jest-runner-vscode, was there a reason to prefer a patch directly in the project over forking the package?

Copy link
Copy Markdown
Member Author

@koesie10 koesie10 Nov 29, 2022

Choose a reason for hiding this comment

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

The original idea was to upstream the changes (there's a PR open for the earlier patch), but it doesn't seem likely that the maintainer is actively checking PRs. I think we can definitely fork the package as a future improvement, although that makes it somewhat harder to make these sorts of changes quickly. We would also need to change the package to publish to our own npm package, and I'm not sure how involved it would be to publish it under the @github scope.

I'll create an issue for investigating whether this is something we can do, although I think it's better to keep it in the project for now so the Jest migration can be merged into main.

diff --git a/node_modules/jest-runner-vscode/dist/run-vscode.js b/node_modules/jest-runner-vscode/dist/run-vscode.js
index 5d8e513..7e556ee 100644
--- a/node_modules/jest-runner-vscode/dist/run-vscode.js
+++ b/node_modules/jest-runner-vscode/dist/run-vscode.js
@@ -5,8 +5,18 @@ var __importDefault = (this && this.__importDefault) || function (mod) {
Object.defineProperty(exports, "__esModule", { value: true });
const child_process_1 = __importDefault(require("child_process"));
const console_1 = __importDefault(require("console"));
-async function runVSCode({ vscodePath, args, jestArgs, env, tests, globalConfig, filterOutput, onStart, onResult, onFailure, ipc, quiet, }) {
- return await new Promise(resolve => {
+const fs_1 = __importDefault(require("fs"));
+const path_1 = __importDefault(require("path"));
+const os_1 = __importDefault(require("os"));
+async function runVSCode(options) {
+ const { vscodePath, args, jestArgs, env, tests, globalConfig, filterOutput, onStart, onResult, onFailure, ipc, quiet, attempt, maxRetries, } = options;
+ const tempUserDir = await fs_1.default.promises.mkdtemp(path_1.default.resolve(os_1.default.tmpdir(), 'jest-runner-vscode-user-data-'));
+ return await new Promise(promiseResolve => {
+ const resolve = () => {
+ fs_1.default.rm(tempUserDir, { recursive: true }, () => {
+ promiseResolve();
+ });
+ };
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg Nov 28, 2022

Choose a reason for hiding this comment

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

Reading a diff of a diff is making my brain hurt. 😅

const useStdErr = globalConfig.json || globalConfig.useStderr;
const log = useStdErr
? console_1.default.error.bind(console_1.default)
@@ -82,7 +92,11 @@ async function runVSCode({ vscodePath, args, jestArgs, env, tests, globalConfig,
ipc.server.on('stdout', onStdout);
ipc.server.on('stderr', onStderr);
ipc.server.on('error', onError);
- const vscode = child_process_1.default.spawn(vscodePath, args, { env: environment });
+ const launchArgs = args;
+ if (!hasArg('user-data-dir', launchArgs)) {
+ launchArgs.push(`--user-data-dir=${tempUserDir}`);
+ }
+ const vscode = child_process_1.default.spawn(vscodePath, launchArgs, { env: environment });
if (!silent && !filterOutput) {
vscode.stdout.pipe(process.stdout);
vscode.stderr.pipe(process.stderr);
@@ -99,6 +113,29 @@ async function runVSCode({ vscodePath, args, jestArgs, env, tests, globalConfig,
exited = true;
const exit = code ?? signal ?? '<unknown>';
const message = `VS Code exited with exit code ${exit}`;
+ const currentAttempt = attempt ?? 0;
+ const incompleteTests = tests.some(test => !completedTests.has(test));
+ if (maxRetries &&
+ maxRetries > 0 &&
+ currentAttempt < maxRetries &&
+ incompleteTests) {
+ silent || quiet || log(message);
+ const newAttempt = currentAttempt + 1;
+ const newTests = tests.filter(test => !completedTests.has(test));
+ ipc.server.off('testFileResult', onTestFileResult);
+ ipc.server.off('testStart', onTestStart);
+ ipc.server.off('testFileStart', onTestStart);
+ ipc.server.off('stdout', onStdout);
+ ipc.server.off('stderr', onStderr);
+ ipc.server.off('error', onError);
+ await runVSCode({
+ ...options,
+ tests: newTests,
+ attempt: newAttempt,
+ });
+ resolve();
+ return;
+ }
if (typeof code !== 'number' || code !== 0) {
silent || quiet || console_1.default.error(message);
const error = vscodeError ?? childError ?? new Error(message);
@@ -138,3 +175,6 @@ async function runVSCode({ vscodePath, args, jestArgs, env, tests, globalConfig,
});
}
exports.default = runVSCode;
+function hasArg(argName, argList) {
+ return argList.some(a => a === `--${argName}` || a.startsWith(`--${argName}=`));
+}
diff --git a/node_modules/jest-runner-vscode/dist/runner.js b/node_modules/jest-runner-vscode/dist/runner.js
index e24c976..c374022 100644
--- a/node_modules/jest-runner-vscode/dist/runner.js
+++ b/node_modules/jest-runner-vscode/dist/runner.js
@@ -107,6 +107,7 @@ class VSCodeTestRunner {
onFailure,
ipc,
quiet: vscodeOptions.quiet,
+ maxRetries: vscodeOptions.retries,
});
}
catch (error) {
1 change: 1 addition & 0 deletions extensions/ql-vscode/src/vscode-tests/cli-integration/jest-runner-vscode.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const config = {
...baseConfig.extensionTestsEnv,
INTEGRATION_TEST_MODE: "true",
},
retries: 3,
Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu Nov 28, 2022

Choose a reason for hiding this comment

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

Is there any guarantee that 3 retries will resolve the flakiness?

I know it's a lot to ask after so much effort doing the jest migration, but are we able to pinpoint what the failure reasons are at the moment for windows?

};

module.exports = config;
1 change: 0 additions & 1 deletion extensions/ql-vscode/src/vscode-tests/jest-runner-vscode.config.base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ const config = {
launchArgs: [
"--disable-gpu",
"--extensions-dir=" + path.join(rootDir, ".vscode-test", "extensions"),
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg Nov 28, 2022

Choose a reason for hiding this comment

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

Should we be disabling potentially conflicting extensions here as well? There was a list of them when running with mocha. Not sure if these extensions are being explicitly disabled any more.

Copy link
Copy Markdown
Member Author

@koesie10 koesie10 Nov 29, 2022

Choose a reason for hiding this comment

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

The extensions are being disabled in the configuration file specific to the cli-integration tests:

vscode-codeql/extensions/ql-vscode/src/vscode-tests/cli-integration/jest-runner-vscode.config.js

Lines 14 to 19 in 7ba9027

"--disable-extension",
"eamodio.gitlens",
"--disable-extension",
"github.codespaces",
"--disable-extension",
"github.copilot",

We are also disabling all extensions in the configuration for the no-workspace and minimal-workspace tests:

vscode-codeql/extensions/ql-vscode/src/vscode-tests/no-workspace/jest-runner-vscode.config.js

Line 6 in 7ba9027

launchArgs: [...(baseConfig.launchArgs ?? []), "--disable-extensions"],

This should match the previous Mocha setup in terms of which extensions are enabled/disabled.

"--user-data-dir=" + path.join(tmpDir.name, "user-data"),
],
extensionDevelopmentPath: rootDir,
};
Expand Down
1 change: 1 addition & 0 deletions extensions/ql-vscode/src/vscode-tests/minimal-workspace/jest-runner-vscode.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const config = {
"--disable-extensions",
path.resolve(rootDir, "test/data"),
],
retries: 1,
};

module.exports = config;
1 change: 1 addition & 0 deletions extensions/ql-vscode/src/vscode-tests/no-workspace/jest-runner-vscode.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const { config: baseConfig } = require("../jest-runner-vscode.config.base");
const config = {
...baseConfig,
launchArgs: [...(baseConfig.launchArgs ?? []), "--disable-extensions"],
retries: 1,
};

module.exports = config;