Add integration tests with the CLI #645

aeisenberg · 2020-10-26T15:36:23Z

Note that even though this PR is labeled as Complexity: High, it is not particularly dangerous in that most of the complexity is around how the tests run.

This commit adds integration tests that run commands using the CLI. This
change introduces a number of enhancements in order to get there.

Augments the index-template.ts file so that it downloads an appropriate cli version if requested.
Adds the ensureCli.ts that performs the download if a a suitable version is not already installed. See the comments in the file for how this is done.
Changes how run-integration-tests is done so that the directories run are specified through a cli argument.
Updates the main.yml workflow so that it also runs the cli-integration tests.
Takes advantage of the return value of the call to activate on the extension. This allows the integration tests to have access to internal variables of the extension like the context, cli, and query server.
And of course, adds a handful of simple tests that ensure we have a cli installed of the correct version.

Resolves #642.

Checklist

[n/a] CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[n/a] @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

aeisenberg · 2020-10-28T21:33:56Z

OK...I'm making progress here. Tests are still failing, but now it is a different failure. Uncovered by fixing the first failure.

aeisenberg

Added a bunch of comments that help explain things in the commit. Some of the comments are reminders for myself to change things later.

.github/workflows/main.yml

aeisenberg · 2020-10-30T15:37:55Z

.github/workflows/main.yml

+    strategy:
+      matrix:
+        os: [ubuntu-latest, windows-latest]
+        version: ['v2.3.1', 'v2.3.0']


We can choose as many versions as we like. Though, my suggestion is to use the latest as well as one for each supported version of LGTM.

We've had a few recent releases, so let's go with 2.2.6, 2.3.3, 2.4.0, latest.
https://github.com/github/codeql-cli-binaries/releases/latest will always point you to the latest release, so we should deliberately test against that even if we are slow to update the hardcoded list.

aeisenberg · 2020-10-30T15:38:39Z

.gitignore

@@ -4,6 +4,7 @@
 # Generated files
 /dist/
 out/
+build/


Over time, I'd like to move all build artifacts to the build directory. This will make it very clear which pieces we can easily clean up.

aeisenberg · 2020-10-30T15:39:27Z

extensions/ql-vscode/package.json

@@ -687,7 +687,8 @@
    "watch:extension": "tsc --watch",
    "test": "mocha --exit -r ts-node/register test/pure-tests/**/*.ts",
    "preintegration": "rm -rf ./out/vscode-tests && gulp",
-    "integration": "node ./out/vscode-tests/run-integration-tests.js",
+    "integration": "node ./out/vscode-tests/run-integration-tests.js no-workspace,minimal-workspace",


Note: must pass in the directories to test as part of the argument.

aeisenberg · 2020-10-30T15:40:03Z

extensions/ql-vscode/src/distribution.ts

+  /**
+   * Get the name of the codeql cli installation we prefer to install, based on our current platform.
+   */
+  public static getRequiredAssetName(): string {


Made static so we can use this in our integration tests.

aeisenberg · 2020-10-30T15:51:52Z

extensions/ql-vscode/src/vscode-tests/no-workspace/astViewer.test.ts

  beforeEach(async () => {
+    sandbox = sinon.createSandbox();


Use sinon.sandbox where possible. This ensures all stubs and mocks are cleaned up when sandbox.restore() is called. One problem with the integration tests earlier on was that mocks were not being properly restored and we were seeing confusing errors across tests.

aeisenberg · 2020-10-30T15:52:52Z

extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts

-    const manager = new (createModule().DistributionManager)(undefined as any, { customCodeQlPath: pathToCmd } as any, undefined as any);
+  it('should not warn when deprecated launcher is used, but no new launcher is available', async function() {
+    const manager = new (createModule().DistributionManager)(
+      { customCodeQlPath: pathToCmd } as any,


DistributionManager has changed. So, need to update these tests.

aeisenberg · 2020-10-30T15:53:58Z

extensions/ql-vscode/src/vscode-tests/run-integration-tests.ts

-    for (const integrationTestSuite of integrationTestSuites) {
-      await runTestsWithRetryOnSegfault(integrationTestSuite, 3);
+    // Which tests to run. Use a comma-separated list of directories.
+    const testDirsString = process.argv[2];


See package.json for where this is used.

aeisenberg · 2020-10-30T15:56:04Z

extensions/ql-vscode/src/vscode-tests/run-integration-tests.ts

@@ -94,3 +104,25 @@ async function main() {
 }

 main();
+
+
+function getLaunchArgs(dir: TestDir) {


This was uglier than I had hoped. '--disable-extensions' needs to be called for minimal and no workspace runs otherwise ambient extensions get in the way of the run. This may not be a problem on the build server, but it is definitely a problem locally.

Annoying but fine for now, since I see you manually install the Test Explorer.

aeisenberg · 2020-10-30T15:56:23Z

extensions/ql-vscode/test/pure-tests/command-lint.test.ts

      command.match(/^codeQLDatabases\./)
      || command.match(/^codeQLQueryHistory\./)
      || command.match(/^codeQLAstViewer\./)
    ) {
      scopedCmds.add(command);
      expect(title).not.to.be.undefined;
      commandTitles[command] = title!;
-    }
-    else {
+    } else if (


This change is no longer necessary.

adityasharad

This is great! Didn't go into the details of the test harness execution from Node (if it runs, I trust you) but the refactoring and tests themselves look good.

extensions/ql-vscode/src/extension.ts

adityasharad · 2020-12-02T23:46:35Z

extensions/ql-vscode/src/query-history.ts

@@ -59,16 +59,15 @@ interface QueryHistoryDataProvider extends vscode.TreeDataProvider<CompletedQuer
 /**
 * Tree data provider for the query history view.
 */
-class HistoryTreeDataProvider implements QueryHistoryDataProvider {
+class HistoryTreeDataProvider extends DisposableObject implements QueryHistoryDataProvider {
  /**
   * XXX: This idiom for how to get a `.fire()`-able event emitter was


Still planning to do this?

adityasharad · 2020-12-02T23:48:50Z

extensions/ql-vscode/src/vscode-tests/cli-integration/global.helper.ts

+
+  async () => {
+    // Set the CLI version here before activation to ensure we don't accidentally try to download a cli
+    await workspace.getConfiguration().update('codeQL.cli.executablePath', process.env.CLI_PATH, ConfigurationTarget.Global);


Good call. Would it make sense to also test picking up a CLI from the PATH? (Perhaps we have that tested elsewhere.)

adityasharad · 2020-12-02T23:51:45Z

extensions/ql-vscode/src/vscode-tests/run-integration-tests.ts

+import { assertNever } from '../pure/helpers-pure';
+
+// For some reason, `TestOptions` is not exported directly from `vscode-test`,
+// but we can be tricky and import directly from the out file.


adityasharad · 2020-12-02T23:52:00Z

extensions/ql-vscode/src/vscode-tests/run-integration-tests.ts

+  NoWorskspace = 'no-workspace',
+  MinimalWorskspace = 'minimal-workspace',


minor typos: Workspace

adityasharad · 2020-12-02T23:52:43Z

extensions/ql-vscode/src/vscode-tests/run-integration-tests.ts

@@ -94,3 +104,25 @@ async function main() {
 }

 main();
+
+
+function getLaunchArgs(dir: TestDir) {


Annoying but fine for now, since I see you manually install the Test Explorer.

adityasharad · 2020-12-02T23:54:42Z

.github/workflows/main.yml

+    strategy:
+      matrix:
+        os: [ubuntu-latest, windows-latest]
+        version: ['v2.3.1', 'v2.3.0']


We've had a few recent releases, so let's go with 2.2.6, 2.3.3, 2.4.0, latest.
https://github.com/github/codeql-cli-binaries/releases/latest will always point you to the latest release, so we should deliberately test against that even if we are slow to update the hardcoded list.

adityasharad · 2020-12-02T23:55:21Z

extensions/ql-vscode/src/vscode-tests/ensureCli.ts

+
+// CLI version to test. Hard code the latest as default. And be sure
+// to update the env if it is not otherwise set.
+const CLI_VERSION = process.env.CLI_VERSION || 'v2.3.1';


I think you can hardcode latest, though you should check the API docs to make sure the asset download URL still works the same.

Unfortunately,

This does not work:
https://github.com/github/codeql-cli-binaries/releases/download/latest/codeql-osx64.zip

But this does:
https://github.com/github/codeql-cli-binaries/releases/download/v2.3.1/codeql-osx64.zip

Also, a downside of using latest locally is that once you download latest once, you will never re-download. We check whether or not to re-download based on the presence of the directory name. The directory name is generated based on the version number. I could probably be more sophisticated about this, but that would add complexity.

This commit adds integration tests that run commands using the CLI. This change introduces a number of enhancements in order to get there. 1. Augments the index-template.ts file so that it downloads an appropriate cli version if requested. 2. Adds the ensureCli.ts that performs the download if a a suitable version is not already installed. See the comments in the file for how this is done. 3. Changes how run-integration-tests is done so that the directories run are specified through a cli argument. 4. Updates the main.yml workflow so that it also runs the cli-integration tests. 5. Takes advantage of the return value of the call to `activate` on the extension. This allows the integration tests to have access to internal variables of the extension like the context, cli, and query server. 6. And of course, adds a handful of simple tests that ensure we have a cli installed of the correct version.

In order to do this, needed to add a few extra pieces: * extracted the simple database download so that it only happens once and is shared across all tests. * needed to update mocha to latest version since that has the new API * But typings isn't updated yet, so submitted a PR into DefinitelyTyped for that. * Added a concept of helper files for test runs. These helper files will contain all the shared global setup. Unfortunately, at this point, we can't run using a language pack since we would also need to download the the ql repository from somewhere.

Also, fix a typo and remove comment.

aeisenberg marked this pull request as draft October 26, 2020 15:36

aeisenberg force-pushed the add-cli-integration-tests branch 11 times, most recently from 8d76a68 to 5769c14 Compare October 28, 2020 21:15

aeisenberg requested review from jcreedcmu and adityasharad October 28, 2020 21:16

aeisenberg marked this pull request as ready for review October 28, 2020 21:16

aeisenberg marked this pull request as draft October 28, 2020 21:34

aeisenberg force-pushed the add-cli-integration-tests branch 2 times, most recently from 1d88795 to f53d728 Compare October 30, 2020 15:36

aeisenberg marked this pull request as ready for review October 30, 2020 15:36

aeisenberg added the Complexity: High Requires careful design or review. label Oct 30, 2020

aeisenberg commented Oct 30, 2020

View reviewed changes

aeisenberg force-pushed the add-cli-integration-tests branch from f53d728 to 3ceaba9 Compare October 30, 2020 16:23

aeisenberg mentioned this pull request Nov 9, 2020

Integration tests to create for the extension #676

Open

16 tasks

aeisenberg force-pushed the add-cli-integration-tests branch 4 times, most recently from 4d566a9 to 7cf33a8 Compare November 24, 2020 19:20

aeisenberg requested review from alexet and removed request for jcreedcmu November 24, 2020 19:26

aeisenberg force-pushed the add-cli-integration-tests branch 10 times, most recently from 25a608f to 92c70f8 Compare December 1, 2020 16:26

aeisenberg force-pushed the add-cli-integration-tests branch 2 times, most recently from c345b10 to ce22b15 Compare December 2, 2020 01:52

adityasharad reviewed Dec 2, 2020

View reviewed changes

aeisenberg added 4 commits December 3, 2020 09:25

Add integration tests for database fetching

ab0859a

Update integration test version numbers

f3d7d07

Also, fix a typo and remove comment.

aeisenberg force-pushed the add-cli-integration-tests branch from ce22b15 to f3d7d07 Compare December 3, 2020 17:27

aeisenberg assigned adityasharad Dec 4, 2020

adityasharad approved these changes Dec 4, 2020

View reviewed changes

aeisenberg merged commit 370dbcb into github:main Dec 4, 2020

Apr	MAY	Jun
	21
2024	2025	2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration tests with the CLI #645

Add integration tests with the CLI #645

aeisenberg commented Oct 26, 2020 •

edited

Loading

aeisenberg commented Oct 28, 2020

aeisenberg left a comment

aeisenberg Oct 30, 2020

adityasharad Dec 2, 2020

aeisenberg Oct 30, 2020

aeisenberg Oct 30, 2020

aeisenberg Oct 30, 2020

aeisenberg Oct 30, 2020

aeisenberg Oct 30, 2020

aeisenberg Oct 30, 2020

aeisenberg Oct 30, 2020

adityasharad Dec 2, 2020

aeisenberg Oct 30, 2020

adityasharad left a comment

adityasharad Dec 2, 2020

adityasharad Dec 2, 2020

adityasharad Dec 2, 2020

adityasharad Dec 2, 2020

adityasharad Dec 2, 2020

adityasharad Dec 2, 2020

adityasharad Dec 2, 2020

aeisenberg Dec 3, 2020

aeisenberg Dec 3, 2020

		NoWorskspace = 'no-workspace',
		MinimalWorskspace = 'minimal-workspace',

Add integration tests with the CLI #645

Add integration tests with the CLI #645

Conversation

aeisenberg commented Oct 26, 2020 • edited Loading

Checklist

aeisenberg commented Oct 28, 2020

aeisenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityasharad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeisenberg commented Oct 26, 2020 •

edited

Loading