Test cleanups

- Avoid Installing `xvfb` since it is already available.
- Ensure `supportsNewQueryServer()` takes the CLI version into account
- Always run the new query server tests on v2.11.1 and later
- Avoid printing directory contents in `run-remote-query-tests`
- Run tests with `--disable-workspace-trust` to avoid a non-fatal error
  being thrown from the dialog service.
- Ensure the exit code of the extension host while running integration
  tests is the exit code of the actual process. Otherwise, there is
  a possibility that an error exit code is swallowed up and ignored.
- Remove a duplicate unhandledRejection handler.
- Handle Exit code 7 from windows. This appears to be a failure on
  exiting and unrelated to the tests.
- Fix handling of configuration in tests:
    1. It is not possible to update a configuration setting for internal
       settings like `codeql.canary`.
    2. On windows CI, updating configuration after global teardown. So,
       on tests avoid resetting test configuration when tests are over.

Also, I tried to remove all those pesky errors in the logs like:

> [2094:1017/235357.424002:ERROR:bus.cc(398)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")

I was following advice from here, but I can't get it working.

- https://github.com/microsoft/vscode-test/issues/127
- https://github.com/electron/electron/issues/31981

.github/workflows/main.yml

@@ -121,7 +121,7 @@ jobs:
         env:
           VSCODE_CODEQL_GITHUB_TOKEN: '${{ secrets.GITHUB_TOKEN }}'
         run: |
-          sudo apt-get install xvfb
+          unset DBUS_SESSION_BUS_ADDRESS
           /usr/bin/xvfb-run npm run integration
 
       - name: Run integration tests (Windows)
@@ -188,6 +188,7 @@ jobs:
         working-directory: extensions/ql-vscode
         if: matrix.os == 'ubuntu-latest'
         run: |
+          unset DBUS_SESSION_BUS_ADDRESS
           /usr/bin/xvfb-run npm run cli-integration
 
       - name: Run CLI tests (Windows)

extensions/ql-vscode/src/cli.ts

@@ -1333,7 +1333,7 @@ export class CliVersionConstraint {
   /**
    * CLI version that supports the new query server.
    */
-  public static CLI_VERSION_WITH_NEW_QUERY_SERVER = new SemVer('2.11.0');
+  public static CLI_VERSION_WITH_NEW_QUERY_SERVER = new SemVer('2.11.1');
 
   constructor(private readonly cli: CodeQLCliServer) {
     /**/
@@ -1414,8 +1414,11 @@ export class CliVersionConstraint {
   async supportsNewQueryServer() {
     // TODO while under development, users _must_ opt-in to the new query server
     // by setting the `codeql.canaryQueryServer` setting to `true`.
-    // Ignore the version check for now.
-    return allowCanaryQueryServer();
-    // return this.isVersionAtLeast(CliVersionConstraint.CLI_VERSION_WITH_NEW_QUERY_SERVER);
+    return allowCanaryQueryServer() &&
+      this.isVersionAtLeast(CliVersionConstraint.CLI_VERSION_WITH_NEW_QUERY_SERVER);
+  }
+
+  async supportsNewQueryServerForTests() {
+    return this.isVersionAtLeast(CliVersionConstraint.CLI_VERSION_WITH_NEW_QUERY_SERVER);
   }
 }

extensions/ql-vscode/src/config.ts

@@ -11,9 +11,10 @@ export class Setting {
   name: string;
   parent?: Setting;
 
-  constructor(name: string, parent?: Setting) {
+  constructor(name: string, parent?: Setting, public readonly isHidden = false) {
     this.name = name;
     this.parent = parent;
+
     ALL_SETTINGS.push(this);
   }
 
@@ -327,7 +328,7 @@ export function isQuickEvalCodelensEnabled() {
 /**
  * Enables canary features of this extension. Recommended for all internal users.
  */
-export const CANARY_FEATURES = new Setting('canary', ROOT_SETTING);
+export const CANARY_FEATURES = new Setting('canary', ROOT_SETTING, true);
 
 export function isCanary() {
   return !!CANARY_FEATURES.getValue<boolean>();
@@ -336,7 +337,7 @@ export function isCanary() {
 /**
  * Enables the experimental query server
  */
-export const CANARY_QUERY_SERVER = new Setting('canaryQueryServer', ROOT_SETTING);
+export const CANARY_QUERY_SERVER = new Setting('canaryQueryServer', ROOT_SETTING, true);
 
 
 export function allowCanaryQueryServer() {

extensions/ql-vscode/src/helpers.ts

@@ -22,7 +22,11 @@ fs.ensureDirSync(upgradesTmpDir);
 
 export const tmpDirDisposal = {
   dispose: () => {
-    tmpDir.removeCallback();
+    try {
+      tmpDir.removeCallback();
+    } catch (e) {
+      void logger.log(`Failed to remove temporary directory ${tmpDir.name}: ${e}`);
+    }
   }
 };
 

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

@@ -14,15 +14,6 @@ import { CUSTOM_CODEQL_PATH_SETTING } from '../../config';
 
 export const DB_URL = 'https://github.com/github/vscode-codeql/files/5586722/simple-db.zip';
 
-process.addListener('unhandledRejection', (reason) => {
-  if (reason instanceof Error && reason.message === 'Canceled') {
-    console.log('Cancellation requested after the test has ended.');
-    process.exit(0);
-  } else {
-    fail(String(reason));
-  }
-});
-
 // We need to resolve the path, but the final three segments won't exist until later, so we only resolve the
 // first portion of the path.
 export const dbLoc = path.join(fs.realpathSync(path.join(__dirname, '../../../')), 'build/tests/db.zip');
@@ -84,7 +75,11 @@ export default function(mocha: Mocha) {
       // This shuts down the extension and can only be run after all tests have completed.
       // If this is not called, then the test process will hang.
       if ('dispose' in extension) {
-        extension.dispose();
+        try {
+          extension.dispose();
+        } catch (e) {
+          console.warn('Failed to dispose extension', e);
+        }
       }
     }
   );
@@ -92,7 +87,13 @@ export default function(mocha: Mocha) {
   // ensure temp directory is cleaned up.
   (mocha.options as any).globalTeardown.push(
     () => {
-      removeStorage?.();
+      try {
+        removeStorage?.();
+      } catch (e) {
+        // we are exiting anyway so don't worry about it.
+        // most likely the directory this is a test on Windows and some files are locked.
+        console.warn(`Failed to remove storage directory '${storagePath}': ${e}`);
+      }
     }
   );
 

extensions/ql-vscode/src/vscode-tests/cli-integration/new-query.test.ts

@@ -16,7 +16,6 @@ import { QueryResultType } from '../../pure/new-messages';
 import { cleanDatabases, dbLoc, storagePath } from './global.helper';
 import { importArchiveDatabase } from '../../databaseFetcher';
 
-
 const baseDir = path.join(__dirname, '../../../test/data');
 
 const tmpDir = tmp.dirSync({ prefix: 'query_test_', keep: false, unsafeCleanup: true });
@@ -106,7 +105,7 @@ describe('using the new query server', function() {
         cliServer = extension.cliServer;
 
         cliServer.quiet = true;
-        if (!(await cliServer.cliConstraints.supportsNewQueryServer())) {
+        if (!(await cliServer.cliConstraints.supportsNewQueryServerForTests())) {
           this.ctx.skip();
         }
         qs = new QueryServerClient({

extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/run-remote-query.test.ts

@@ -100,7 +100,6 @@ describe('Remote queries', function() {
       const querySubmissionResult = await runRemoteQuery(cli, credentials, fileUri, true, progress, cancellationTokenSource.token, variantAnalysisManager);
       expect(querySubmissionResult).to.be.ok;
       const queryPackRootDir = querySubmissionResult!.queryDirPath!;
-      printDirectoryContents(queryPackRootDir);
 
       // to retrieve the list of repositories
       expect(showQuickPickSpy).to.have.been.calledOnce;
@@ -113,7 +112,6 @@ describe('Remote queries', function() {
       expect(fs.readdirSync(queryPackRootDir).find(f => f.startsWith('qlpack-') && f.endsWith('-generated.tgz'))).not.to.be.undefined;
 
       const queryPackDir = path.join(queryPackRootDir, 'query-pack');
-      printDirectoryContents(queryPackDir);
 
       expect(fs.existsSync(path.join(queryPackDir, 'in-pack.ql'))).to.be.true;
       expect(fs.existsSync(path.join(queryPackDir, 'lib.qll'))).to.be.true;
@@ -128,7 +126,6 @@ describe('Remote queries', function() {
 
       // the compiled pack
       const compiledPackDir = path.join(queryPackDir, '.codeql/pack/codeql-remote/query/0.0.0/');
-      printDirectoryContents(compiledPackDir);
 
       expect(fs.existsSync(path.join(compiledPackDir, 'in-pack.ql'))).to.be.true;
       expect(fs.existsSync(path.join(compiledPackDir, 'lib.qll'))).to.be.true;
@@ -171,11 +168,9 @@ describe('Remote queries', function() {
       // check a few files that we know should exist and others that we know should not
 
       // the tarball to deliver to the server
-      printDirectoryContents(queryPackRootDir);
       expect(fs.readdirSync(queryPackRootDir).find(f => f.startsWith('qlpack-') && f.endsWith('-generated.tgz'))).not.to.be.undefined;
 
       const queryPackDir = path.join(queryPackRootDir, 'query-pack');
-      printDirectoryContents(queryPackDir);
 
       expect(fs.existsSync(path.join(queryPackDir, 'in-pack.ql'))).to.be.true;
       expect(fs.existsSync(path.join(queryPackDir, 'qlpack.yml'))).to.be.true;
@@ -189,7 +184,6 @@ describe('Remote queries', function() {
 
       // the compiled pack
       const compiledPackDir = path.join(queryPackDir, '.codeql/pack/codeql-remote/query/0.0.0/');
-      printDirectoryContents(compiledPackDir);
       expect(fs.existsSync(path.join(compiledPackDir, 'in-pack.ql'))).to.be.true;
       expect(fs.existsSync(path.join(compiledPackDir, 'qlpack.yml'))).to.be.true;
       verifyQlPack(path.join(compiledPackDir, 'qlpack.yml'), 'in-pack.ql', '0.0.0', await pathSerializationBroken());
@@ -208,7 +202,6 @@ describe('Remote queries', function() {
       expect(qlpackContents.dependencies?.['codeql/javascript-all']).to.equal('*');
 
       const libraryDir = path.join(compiledPackDir, '.codeql/libraries/codeql');
-      printDirectoryContents(libraryDir);
       const packNames = fs.readdirSync(libraryDir).sort();
 
       // check dependencies.
@@ -233,11 +226,9 @@ describe('Remote queries', function() {
       // check a few files that we know should exist and others that we know should not
 
       // the tarball to deliver to the server
-      printDirectoryContents(queryPackRootDir);
       expect(fs.readdirSync(queryPackRootDir).find(f => f.startsWith('qlpack-') && f.endsWith('-generated.tgz'))).not.to.be.undefined;
 
       const queryPackDir = path.join(queryPackRootDir, 'query-pack');
-      printDirectoryContents(queryPackDir);
 
       expect(fs.existsSync(path.join(queryPackDir, 'subfolder/in-pack.ql'))).to.be.true;
       expect(fs.existsSync(path.join(queryPackDir, 'qlpack.yml'))).to.be.true;
@@ -251,7 +242,6 @@ describe('Remote queries', function() {
 
       // the compiled pack
       const compiledPackDir = path.join(queryPackDir, '.codeql/pack/codeql-remote/query/0.0.0/');
-      printDirectoryContents(compiledPackDir);
       expect(fs.existsSync(path.join(compiledPackDir, 'otherfolder/lib.qll'))).to.be.true;
       expect(fs.existsSync(path.join(compiledPackDir, 'subfolder/in-pack.ql'))).to.be.true;
       expect(fs.existsSync(path.join(compiledPackDir, 'qlpack.yml'))).to.be.true;
@@ -270,7 +260,6 @@ describe('Remote queries', function() {
       expect(qlpackContents.dependencies?.['codeql/javascript-all']).to.equal('*');
 
       const libraryDir = path.join(compiledPackDir, '.codeql/libraries/codeql');
-      printDirectoryContents(libraryDir);
       const packNames = fs.readdirSync(libraryDir).sort();
 
       // check dependencies.
@@ -399,12 +388,4 @@ describe('Remote queries', function() {
   function getFile(file: string): Uri {
     return Uri.file(path.join(baseDir, file));
   }
-
-  function printDirectoryContents(dir: string) {
-    console.log(`DIR ${dir}`);
-    if (!fs.existsSync(dir)) {
-      console.log(`DIR ${dir} does not exist`);
-    }
-    fs.readdirSync(dir).sort().forEach(f => console.log(`  ${f}`));
-  }
 });

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

@@ -30,15 +30,6 @@ import { workspace } from 'vscode';
  * exists. And the cli will not be re-downloaded if the zip already exists.
  */
 
-process.on('unhandledRejection', e => {
-  console.error('Unhandled rejection.');
-  console.error(e);
-  // Must use a setTimeout in order to ensure the log is fully flushed before exiting
-  setTimeout(() => {
-    process.exit(-1);
-  }, 2000);
-});
-
 const _1MB = 1024 * 1024;
 const _10MB = _1MB * 10;
 

extensions/ql-vscode/src/vscode-tests/index-template.ts

@@ -16,6 +16,15 @@ process.on('unhandledRejection', e => {
   }, 2000);
 });
 
+process.on('exit', code => {
+  // If the exit code is 7, then the test runner has completed, but
+  // there was an error in exiting vscode.
+  if (code === 7) {
+    console.warn('Attempted to exit with code 7. This is likely due to a failure to exit vscode. Ignoring this and exiting with code 0.');
+    process.exit(0);
+  }
+});
+
 /**
  * Helper function that runs all Mocha tests found in the
  * given test root directory.

extensions/ql-vscode/src/vscode-tests/minimal-workspace/index.ts

@@ -13,12 +13,3 @@ chai.use(sinonChai);
 export function run(): Promise<void> {
   return runTestsInDirectory(__dirname);
 }
-
-process.addListener('unhandledRejection', (reason) => {
-  if (reason instanceof Error && reason.message === 'Canceled') {
-    console.log('Cancellation requested after the test has ended.');
-    process.exit(0);
-  } else {
-    fail(String(reason));
-  }
-});

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

@@ -13,7 +13,6 @@ import * as tmp from 'tmp-promise';
 // but we can be tricky and import directly from the out file.
 import { TestOptions } from 'vscode-test/out/runTest';
 
-
 // For CI purposes we want to leave this at 'stable' to catch any bugs
 // that might show up with new vscode versions released, even though
 // this makes testing not-quite-pure, but it can be changed for local
@@ -34,24 +33,21 @@ enum TestDir {
  * Run an integration test suite `suite`, retrying if it segfaults, at
  * most `tries` times.
  */
-async function runTestsWithRetryOnSegfault(suite: TestOptions, tries: number): Promise<void> {
+async function runTestsWithRetryOnSegfault(suite: TestOptions, tries: number): Promise<number> {
   for (let t = 0; t < tries; t++) {
     try {
       // Download and unzip VS Code if necessary, and run the integration test suite.
-      await runTests(suite);
-      return;
+      return await runTests(suite);
     } catch (err) {
       if (err === 'SIGSEGV') {
         console.error('Test runner segfaulted.');
         if (t < tries - 1)
           console.error('Retrying...');
-      }
-      else if (os.platform() === 'win32') {
+      } else if (os.platform() === 'win32') {
         console.error(`Test runner caught exception (${err})`);
         if (t < tries - 1)
           console.error('Retrying...');
-      }
-      else {
+      } else {
         throw err;
       }
     }
@@ -67,6 +63,7 @@ const tmpDir = tmp.dirSync({ unsafeCleanup: true });
  * See https://github.com/microsoft/vscode-test/blob/master/sample/test/runTest.ts
  */
 async function main() {
+  let exitCode = 0;
   try {
     const extensionDevelopmentPath = path.resolve(__dirname, '../..');
     const vscodeExecutablePath = await downloadAndUnzipVSCode(VSCODE_VERSION);
@@ -100,7 +97,7 @@ async function main() {
       const launchArgs = getLaunchArgs(dir as TestDir);
       console.log(`Next integration test dir: ${dir}`);
       console.log(`Launch args: ${launchArgs}`);
-      await runTestsWithRetryOnSegfault({
+      exitCode = await runTestsWithRetryOnSegfault({
         version: VSCODE_VERSION,
         vscodeExecutablePath,
         extensionDevelopmentPath,
@@ -111,8 +108,12 @@ async function main() {
     }
   } catch (err) {
     console.error(`Unexpected exception while running tests: ${err}`);
-    console.error((err as Error).stack);
-    process.exit(1);
+    if (err instanceof Error) {
+      console.error(err.stack);
+    }
+    exitCode = 1;
+  } finally {
+    process.exit(exitCode);
   }
 }
 
@@ -125,6 +126,7 @@ function getLaunchArgs(dir: TestDir) {
       return [
         '--disable-extensions',
         '--disable-gpu',
+        '--disable-workspace-trust',
         '--user-data-dir=' + path.join(tmpDir.name, dir, 'user-data')
       ];
 
@@ -132,6 +134,7 @@ function getLaunchArgs(dir: TestDir) {
       return [
         '--disable-extensions',
         '--disable-gpu',
+        '--disable-workspace-trust',
         '--user-data-dir=' + path.join(tmpDir.name, dir, 'user-data'),
         path.resolve(__dirname, '../../test/data')
       ];
@@ -139,6 +142,7 @@ function getLaunchArgs(dir: TestDir) {
     case TestDir.CliIntegration:
       // CLI integration tests requires a multi-root workspace so that the data and the QL sources are accessible.
       return [
+        '--disable-workspace-trust',
         '--disable-gpu',
         path.resolve(__dirname, '../../test/data'),
 
@@ -155,5 +159,4 @@ function getLaunchArgs(dir: TestDir) {
     default:
       assertNever(dir);
   }
-  return undefined;
 }

extensions/ql-vscode/src/vscode-tests/test-config.ts

@@ -60,8 +60,11 @@ class TestSetting<T> {
 }
 
 // The test settings are all settings in ALL_SETTINGS which don't have any children
+// and are also not hiddent settings like the Canary setting.
 const TEST_SETTINGS = ALL_SETTINGS
-  .filter(setting => ALL_SETTINGS.filter(s => s.parent === setting).length === 0)
+  .filter(setting => !setting.isHidden &&
+    ALL_SETTINGS.filter(s =>
+      s.parent === setting).length === 0)
   .map(setting => new TestSetting(setting));
 
 export const getTestSetting = (setting: Setting): TestSetting<unknown> | undefined => {
@@ -79,7 +82,10 @@ export const testConfigHelper = async (mocha: Mocha) => {
     },
     async afterAll() {
       // Restore all settings to their default values after each test suite
-      await Promise.all(TEST_SETTINGS.map(setting => setting.restoreToInitialValues()));
+      // Only do this outside of CI since the sometimes hangs on CI.
+      if (process.env.CI !== 'true') {
+        await Promise.all(TEST_SETTINGS.map(setting => setting.restoreToInitialValues()));
+      }
     }
   });
 };