Mock config instead of writing settings files

In our tests, we were writing settings files to disk because we were
using the VSCode configuration API which writes settings to files. This
results in flaky tests because concurrency can cause the VSCode API to
misbehave.

This will switch the tests to use a mocked API by default. For some
tests the real implementation is used, but the large majority of tests
is now using a mocked version which only keeps track of the
configuration in memory. This makes it easier to reset the state between
tests since we can just empty out the in-memory configuration.

extensions/ql-vscode/src/config.ts

@@ -56,15 +56,6 @@ export class Setting {
       .getConfiguration(this.parent.qualifiedName)
       .update(this.name, value, target);
   }
-
-  inspect<T>(): InspectionResult<T> | undefined {
-    if (this.parent === undefined) {
-      throw new Error("Cannot update the value of a root setting.");
-    }
-    return workspace
-      .getConfiguration(this.parent.qualifiedName)
-      .inspect(this.name);
-  }
 }
 
 export interface InspectionResult<T> {

extensions/ql-vscode/test/vscode-tests/cli-integration/jest.setup.ts

@@ -8,23 +8,24 @@ import { dirname } from "path";
 import fetch from "node-fetch";
 import { DB_URL, dbLoc, setStoragePath, storagePath } from "./global.helper";
 import * as tmp from "tmp";
-import { getTestSetting } from "../test-config";
 import { CUSTOM_CODEQL_PATH_SETTING } from "../../../src/config";
-import { extensions, workspace } from "vscode";
+import { ConfigurationTarget, env, extensions, workspace } from "vscode";
+import { beforeEachAction } from "../test-config";
 
-import baseJestSetup from "../jest.setup";
-
-export default baseJestSetup;
+(env as any).openExternal = () => {
+  /**/
+};
 
 // create an extension storage location
 let removeStorage: tmp.DirResult["removeCallback"] | undefined;
 
 beforeAll(async () => {
   // Set the CLI version here before activation to ensure we don't accidentally try to download a cli
-  await getTestSetting(CUSTOM_CODEQL_PATH_SETTING)?.setInitialTestValue(
+  await beforeEachAction();
+  await CUSTOM_CODEQL_PATH_SETTING.updateValue(
     process.env.CLI_PATH,
+    ConfigurationTarget.Workspace,
   );
-  await getTestSetting(CUSTOM_CODEQL_PATH_SETTING)?.setup();
 
   // ensure the test database is downloaded
   mkdirpSync(dirname(dbLoc));
@@ -78,6 +79,15 @@ beforeAll(async () => {
   await extensions.getExtension("GitHub.vscode-codeql")?.activate();
 });
 
+beforeEach(async () => {
+  await beforeEachAction();
+
+  await CUSTOM_CODEQL_PATH_SETTING.updateValue(
+    process.env.CLI_PATH,
+    ConfigurationTarget.Global,
+  );
+});
+
 // ensure extension is cleaned up.
 afterAll(async () => {
   // ensure temp directory is cleaned up.

extensions/ql-vscode/test/vscode-tests/cli-integration/remote-queries/variant-analysis-submission-integration.test.ts

@@ -3,6 +3,7 @@ import { resolve } from "path";
 import {
   authentication,
   commands,
+  ConfigurationTarget,
   extensions,
   QuickPickItem,
   TextDocument,
@@ -12,7 +13,10 @@ import {
 
 import { CodeQLExtensionInterface } from "../../../../src/extension";
 import { MockGitHubApiServer } from "../../../../src/mocks/mock-gh-api-server";
-import { mockConfiguration } from "../../utils/configuration-helpers";
+import {
+  CANARY_FEATURES,
+  setRemoteControllerRepo,
+} from "../../../../src/config";
 
 jest.setTimeout(30_000);
 
@@ -36,17 +40,8 @@ describe("Variant Analysis Submission Integration", () => {
   let showErrorMessageSpy: jest.SpiedFunction<typeof window.showErrorMessage>;
 
   beforeEach(async () => {
-    mockConfiguration({
-      values: {
-        codeQL: {
-          canary: true,
-        },
-        "codeQL.variantAnalysis": {
-          liveResults: true,
-          controllerRepo: "github/vscode-codeql",
-        },
-      },
-    });
+    await CANARY_FEATURES.updateValue(true, ConfigurationTarget.Global);
+    await setRemoteControllerRepo("github/vscode-codeql");
 
     jest.spyOn(authentication, "getSession").mockResolvedValue({
       id: "test",

extensions/ql-vscode/test/vscode-tests/jest.setup.ts

@@ -1,10 +1,10 @@
 import { env } from "vscode";
-import { jestTestConfigHelper } from "./test-config";
+import { beforeEachAction } from "./test-config";
 
 (env as any).openExternal = () => {
   /**/
 };
 
-export default async function setupEnv() {
-  await jestTestConfigHelper();
-}
+beforeEach(async () => {
+  await beforeEachAction();
+});

extensions/ql-vscode/test/vscode-tests/minimal-workspace/config.test.ts

@@ -6,8 +6,13 @@ import {
   QueryHistoryConfigListener,
   QueryServerConfigListener,
 } from "../../../src/config";
+import { vscodeGetConfigurationMock } from "../test-config";
 
 describe("config listeners", () => {
+  beforeEach(() => {
+    vscodeGetConfigurationMock.mockRestore();
+  });
+
   interface TestConfig<T> {
     clazz: new () => ConfigListener;
     settings: Array<{

extensions/ql-vscode/test/vscode-tests/minimal-workspace/databases/db-panel-rendering.test.ts

@@ -13,7 +13,7 @@ import { DbTreeViewItem } from "../../../../src/databases/ui/db-tree-view-item";
 import { ExtensionApp } from "../../../../src/common/vscode/vscode-app";
 import { createMockExtensionContext } from "../../../factories/extension-context";
 import { createDbConfig } from "../../../factories/db-config-factories";
-import { mockConfiguration } from "../../utils/configuration-helpers";
+import { setRemoteControllerRepo } from "../../../../src/config";
 
 describe("db panel rendering nodes", () => {
   const workspaceStoragePath = join(__dirname, "test-workspace-storage");
@@ -50,12 +50,8 @@ describe("db panel rendering nodes", () => {
   });
 
   describe("when controller repo is not set", () => {
-    mockConfiguration({
-      values: {
-        "codeQL.variantAnalysis": {
-          controllerRepo: undefined,
-        },
-      },
+    beforeEach(async () => {
+      await setRemoteControllerRepo(undefined);
     });
 
     it("should not have any items", async () => {
@@ -81,14 +77,8 @@ describe("db panel rendering nodes", () => {
   });
 
   describe("when controller repo is set", () => {
-    beforeEach(() => {
-      mockConfiguration({
-        values: {
-          "codeQL.variantAnalysis": {
-            controllerRepo: "github/codeql",
-          },
-        },
-      });
+    beforeEach(async () => {
+      await setRemoteControllerRepo("github/codeql");
     });
 
     it("should render default remote nodes when the config is empty", async () => {

extensions/ql-vscode/test/vscode-tests/minimal-workspace/databases/db-panel.test.ts

@@ -9,7 +9,7 @@ import { DbTreeViewItem } from "../../../../src/databases/ui/db-tree-view-item";
 import { ExtensionApp } from "../../../../src/common/vscode/vscode-app";
 import { createMockExtensionContext } from "../../../factories/extension-context";
 import { createDbConfig } from "../../../factories/db-config-factories";
-import { mockConfiguration } from "../../utils/configuration-helpers";
+import { setRemoteControllerRepo } from "../../../../src/config";
 
 describe("db panel", () => {
   const workspaceStoragePath = join(__dirname, "test-workspace-storage");
@@ -40,13 +40,7 @@ describe("db panel", () => {
   beforeEach(async () => {
     await ensureDir(workspaceStoragePath);
 
-    mockConfiguration({
-      values: {
-        "codeQL.variantAnalysis": {
-          controllerRepo: "github/codeql",
-        },
-      },
-    });
+    await setRemoteControllerRepo("github/codeql");
   });
 
   afterEach(async () => {

extensions/ql-vscode/test/vscode-tests/no-workspace/databases/db-panel-selection.test.ts

@@ -15,7 +15,7 @@ import {
 import { ExtensionApp } from "../../../../src/common/vscode/vscode-app";
 import { createMockExtensionContext } from "../../../factories/extension-context";
 import { createDbConfig } from "../../../factories/db-config-factories";
-import { mockConfiguration } from "../../utils/configuration-helpers";
+import { setRemoteControllerRepo } from "../../../../src/config";
 
 describe("db panel selection", () => {
   const workspaceStoragePath = join(__dirname, "test-workspace-storage");
@@ -46,13 +46,7 @@ describe("db panel selection", () => {
   beforeEach(async () => {
     await ensureDir(workspaceStoragePath);
 
-    mockConfiguration({
-      values: {
-        "codeQL.variantAnalysis": {
-          controllerRepo: "github/codeql",
-        },
-      },
-    });
+    await setRemoteControllerRepo("github/codeql");
   });
 
   afterEach(async () => {

extensions/ql-vscode/test/vscode-tests/no-workspace/telemetry.test.ts

@@ -13,6 +13,7 @@ import { UserCancellationException } from "../../../src/commandRunner";
 import { ENABLE_TELEMETRY } from "../../../src/config";
 import * as Config from "../../../src/config";
 import { createMockExtensionContext } from "./index";
+import { vscodeGetConfigurationMock } from "../test-config";
 
 // setting preferences can trigger lots of background activity
 // so need to bump up the timeout of this test.
@@ -40,6 +41,8 @@ describe("telemetry reporting", () => {
   >;
 
   beforeEach(async () => {
+    vscodeGetConfigurationMock.mockRestore();
+
     try {
       // in case a previous test has accidentally activated this extension,
       // need to disable it first.

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

@@ -1,126 +1,225 @@
+import {
+  ConfigurationScope,
+  ConfigurationTarget,
+  workspace,
+  WorkspaceConfiguration as VSCodeWorkspaceConfiguration,
+} from "vscode";
 import { readFileSync } from "fs-extra";
 import { join } from "path";
-import { ConfigurationTarget } from "vscode";
-import { ALL_SETTINGS, InspectionResult, Setting } from "../../src/config";
 
-class TestSetting<T> {
-  private initialSettingState: InspectionResult<T> | undefined;
+function getIn(object: any, path: string): any {
+  const parts = path.split(".");
+  let current = object;
+  for (const part of parts) {
+    current = current[part];
+    if (current === undefined) {
+      return undefined;
+    }
+  }
+  return current;
+}
 
-  constructor(
-    public readonly setting: Setting,
-    private initialTestValue: T | undefined = undefined,
-  ) {}
+function setIn(object: any, path: string, value: any): void {
+  const parts = path.split(".");
+  let current = object;
+  for (const part of parts.slice(0, -1)) {
+    if (current[part] === undefined) {
+      current[part] = {};
+    }
+    current = current[part];
+  }
+  current[parts[parts.length - 1]] = value;
+}
 
-  public async get(): Promise<T | undefined> {
-    return this.setting.getValue();
+interface WorkspaceConfiguration {
+  scope?: ConfigurationTarget;
+
+  get<T>(section: string | undefined, key: string): T | undefined;
+  has(section: string | undefined, key: string): boolean;
+  update(section: string | undefined, key: string, value: unknown): void;
+}
+
+class InMemoryConfiguration implements WorkspaceConfiguration {
+  private readonly values: Record<string, unknown> = {};
+
+  public constructor(public readonly scope: ConfigurationTarget) {}
+
+  public get<T>(section: string | undefined, key: string): T | undefined {
+    return getIn(this.values, this.getKey(section, key)) as T | undefined;
   }
 
-  public async set(
-    value: T | undefined,
-    target: ConfigurationTarget = ConfigurationTarget.Global,
-  ): Promise<void> {
-    await this.setting.updateValue(value, target);
+  public has(section: string | undefined, key: string): boolean {
+    return getIn(this.values, this.getKey(section, key)) !== undefined;
   }
 
-  public async setInitialTestValue(value: T | undefined) {
-    this.initialTestValue = value;
+  public update(
+    section: string | undefined,
+    key: string,
+    value: unknown,
+  ): void {
+    setIn(this.values, this.getKey(section, key), value);
   }
 
-  public async initialSetup() {
-    this.initialSettingState = this.setting.inspect();
+  private getKey(section: string | undefined, key: string): string {
+    return section ? `${section}.${key}` : key;
+  }
+}
 
-    // Unfortunately it's not well-documented how to check whether we can write to a workspace
-    // configuration. This is the best I could come up with. It only fails for initial test values
-    // which are not undefined.
-    if (this.initialSettingState?.workspaceValue !== undefined) {
-      await this.set(this.initialTestValue, ConfigurationTarget.Workspace);
-    }
-    if (this.initialSettingState?.workspaceFolderValue !== undefined) {
-      await this.set(
-        this.initialTestValue,
-        ConfigurationTarget.WorkspaceFolder,
-      );
-    }
+class DefaultConfiguration implements WorkspaceConfiguration {
+  private readonly values: Record<string, unknown> = {};
 
-    await this.setup();
+  public constructor(configurations: Record<string, { default: unknown }>) {
+    for (const [section, config] of Object.entries(configurations)) {
+      setIn(this.values, section, config.default);
+    }
   }
 
-  public async setup() {
-    await this.set(this.initialTestValue, ConfigurationTarget.Global);
+  public get<T>(section: string | undefined, key: string): T | undefined {
+    return getIn(this.values, this.getKey(section, key)) as T | undefined;
   }
 
-  public async restoreToInitialValues() {
-    const state = this.setting.inspect();
+  public has(section: string | undefined, key: string): boolean {
+    return getIn(this.values, this.getKey(section, key)) !== undefined;
+  }
 
-    // We need to check the state of the setting before we restore it. This is less important for the global
-    // configuration target, but the workspace/workspace folder configuration might not even exist. If they
-    // don't exist, VSCode will error when trying to write the new value (even if that value is undefined).
-    if (state?.globalValue !== this.initialSettingState?.globalValue) {
-      await this.set(
-        this.initialSettingState?.globalValue,
-        ConfigurationTarget.Global,
-      );
+  public update(
+    _section: string | undefined,
+    _key: string,
+    _value: unknown,
+  ): void {
+    throw new Error("Cannot update default configuration");
+  }
+
+  private getKey(section: string | undefined, key: string): string {
+    return section ? `${section}.${key}` : key;
+  }
+}
+
+class ChainedInMemoryConfiguration {
+  constructor(private readonly configurations: WorkspaceConfiguration[]) {}
+
+  public getConfiguration(target: ConfigurationTarget) {
+    const configuration = this.configurations.find(
+      (configuration) => configuration.scope === target,
+    );
+
+    if (configuration === undefined) {
+      throw new Error(`Unknown configuration target ${target}`);
     }
-    if (state?.workspaceValue !== this.initialSettingState?.workspaceValue) {
-      await this.set(
-        this.initialSettingState?.workspaceValue,
-        ConfigurationTarget.Workspace,
-      );
-    }
-    if (
-      state?.workspaceFolderValue !==
-      this.initialSettingState?.workspaceFolderValue
-    ) {
-      await this.set(
-        this.initialSettingState?.workspaceFolderValue,
-        ConfigurationTarget.WorkspaceFolder,
-      );
+
+    return configuration;
+  }
+
+  public get<T>(section: string | undefined, key: string): T | undefined {
+    for (const configuration of this.configurations) {
+      if (configuration.has(section, key)) {
+        return configuration.get(section, key);
+      }
     }
+    return undefined;
+  }
+
+  public has(section: string | undefined, key: string): boolean {
+    return this.configurations.some((configuration) =>
+      configuration.has(section, key),
+    );
+  }
+
+  public update<T>(
+    section: string | undefined,
+    key: string,
+    value: T,
+    target: ConfigurationTarget,
+  ): void {
+    const configuration = this.getConfiguration(target);
+
+    configuration.update(section, key, value);
   }
 }
 
 // Public configuration keys are the ones defined in the package.json.
 // These keys are documented in the settings page. Other keys are
 // internal and not documented.
-const PKG_CONFIGURATION: Record<string, any> =
-  (function initConfigurationKeys() {
-    // Note we are using synchronous file reads here. This is fine because
-    // we are in tests.
-    const pkg = JSON.parse(
-      readFileSync(join(__dirname, "../../package.json"), "utf-8"),
+const packageConfiguration: Record<
+  string,
+  {
+    default: any | undefined;
+  }
+> = (function initConfigurationKeys() {
+  // Note we are using synchronous file reads here. This is fine because
+  // we are in tests.
+  const pkg = JSON.parse(
+    readFileSync(join(__dirname, "../../package.json"), "utf-8"),
+  );
+  return pkg.contributes.configuration.properties;
+})();
+
+export const vsCodeGetConfiguration = workspace.getConfiguration;
+export let vscodeGetConfigurationMock: jest.SpiedFunction<
+  typeof workspace.getConfiguration
+>;
+
+export const beforeEachAction = async () => {
+  const defaultConfiguration = new DefaultConfiguration(packageConfiguration);
+
+  const configuration = new ChainedInMemoryConfiguration([
+    new InMemoryConfiguration(ConfigurationTarget.WorkspaceFolder),
+    new InMemoryConfiguration(ConfigurationTarget.Workspace),
+    new InMemoryConfiguration(ConfigurationTarget.Global),
+    defaultConfiguration,
+  ]);
+
+  vscodeGetConfigurationMock = jest
+    .spyOn(workspace, "getConfiguration")
+    .mockImplementation(
+      (
+        section?: string,
+        scope?: ConfigurationScope | null,
+      ): VSCodeWorkspaceConfiguration => {
+        if (scope) {
+          throw new Error("Scope is not supported in tests");
+        }
+
+        return {
+          get(key: string, defaultValue?: unknown) {
+            return configuration.get(section, key) ?? defaultValue;
+          },
+          has(key: string) {
+            return configuration.has(section, key);
+          },
+          inspect(_key: string) {
+            throw new Error("inspect is not supported in tests");
+          },
+          async update(
+            key: string,
+            value: unknown,
+            configurationTarget?: ConfigurationTarget | boolean | null,
+            overrideInLanguage?: boolean,
+          ) {
+            if (overrideInLanguage) {
+              throw new Error("overrideInLanguage is not supported in tests");
+            }
+
+            function getActualConfigurationTarget(): ConfigurationTarget {
+              if (
+                configurationTarget === undefined ||
+                configurationTarget === null
+              ) {
+                return ConfigurationTarget.Global;
+              }
+              if (typeof configurationTarget === "boolean") {
+                return configurationTarget
+                  ? ConfigurationTarget.Workspace
+                  : ConfigurationTarget.Global;
+              }
+              return configurationTarget;
+            }
+
+            const target = getActualConfigurationTarget();
+
+            configuration.update(section, key, value, target);
+          },
+        };
+      },
     );
-    return pkg.contributes.configuration.properties;
-  })();
-
-// The test settings are all settings in ALL_SETTINGS which don't have any children
-// and are also not hidden settings like the codeQL.canary.
-const TEST_SETTINGS = ALL_SETTINGS.filter(
-  (setting) =>
-    setting.qualifiedName in PKG_CONFIGURATION && !setting.hasChildren,
-).map((setting) => new TestSetting(setting));
-
-export const getTestSetting = (
-  setting: Setting,
-): TestSetting<unknown> | undefined => {
-  return TEST_SETTINGS.find((testSetting) => testSetting.setting === setting);
-};
-
-export const jestTestConfigHelper = async () => {
-  // Read in all current settings
-  await Promise.all(TEST_SETTINGS.map((setting) => setting.initialSetup()));
-
-  beforeEach(async () => {
-    // Reset the settings to their initial values before each test
-    await Promise.all(TEST_SETTINGS.map((setting) => setting.setup()));
-  });
-
-  afterAll(async () => {
-    // Restore all settings to their default values after each test suite
-    // 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()),
-      );
-    }
-  });
 };

extensions/ql-vscode/test/vscode-tests/utils/configuration-helpers.ts

@@ -1,53 +0,0 @@
-import { workspace } from "vscode";
-
-type MockConfigurationConfig = {
-  values: {
-    [section: string]: {
-      [scope: string]: any | (() => any);
-    };
-  };
-};
-
-export function mockConfiguration(config: MockConfigurationConfig) {
-  const originalGetConfiguration = workspace.getConfiguration;
-
-  jest
-    .spyOn(workspace, "getConfiguration")
-    .mockImplementation((section, scope) => {
-      const configuration = originalGetConfiguration(section, scope);
-
-      return {
-        get(key: string, defaultValue?: unknown) {
-          if (
-            section &&
-            config.values[section] &&
-            config.values[section][key]
-          ) {
-            const value = config.values[section][key];
-            return typeof value === "function" ? value() : value;
-          }
-
-          return configuration.get(key, defaultValue);
-        },
-        has(key: string) {
-          return configuration.has(key);
-        },
-        inspect(key: string) {
-          return configuration.inspect(key);
-        },
-        update(
-          key: string,
-          value: unknown,
-          configurationTarget?: boolean,
-          overrideInLanguage?: boolean,
-        ) {
-          return configuration.update(
-            key,
-            value,
-            configurationTarget,
-            overrideInLanguage,
-          );
-        },
-      };
-    });
-}