Minor refactoring of the db config store and tests (#1924)

* Remove unnecessary type castings
* Extract validation to separate functions
* Nest tests inside a 'describe' block

extensions/ql-vscode/src/databases/config/db-config-store.ts

@@ -94,7 +94,7 @@ export class DbConfigStore extends DisposableObject {
       );
     }
 
-    const config: DbConfig = cloneDbConfig(this.config);
+    const config = cloneDbConfig(this.config);
     if (parentList) {
       const parent = config.databases.remote.repositoryLists.find(
         (list) => list.name === parentList,
@@ -123,7 +123,7 @@ export class DbConfigStore extends DisposableObject {
       throw Error(`A remote owner with the name '${owner}' already exists`);
     }
 
-    const config: DbConfig = cloneDbConfig(this.config);
+    const config = cloneDbConfig(this.config);
     config.databases.remote.owners.push(owner);
 
     await this.writeConfig(config);
@@ -134,15 +134,9 @@ export class DbConfigStore extends DisposableObject {
       throw Error("Cannot add local list if config is not loaded");
     }
 
-    if (listName === "") {
-      throw Error("List name cannot be empty");
-    }
+    this.validateLocalListName(listName);
 
-    if (this.doesLocalListExist(listName)) {
-      throw Error(`A local list with the name '${listName}' already exists`);
-    }
-
-    const config: DbConfig = cloneDbConfig(this.config);
+    const config = cloneDbConfig(this.config);
     config.databases.local.lists.push({
       name: listName,
       databases: [],
@@ -156,15 +150,9 @@ export class DbConfigStore extends DisposableObject {
       throw Error("Cannot add remote list if config is not loaded");
     }
 
-    if (listName === "") {
-      throw Error("List name cannot be empty");
-    }
+    this.validateRemoteListName(listName);
 
-    if (this.doesRemoteListExist(listName)) {
-      throw Error(`A remote list with the name '${listName}' already exists`);
-    }
-
-    const config: DbConfig = cloneDbConfig(this.config);
+    const config = cloneDbConfig(this.config);
     config.databases.remote.repositoryLists.push({
       name: listName,
       repositories: [],
@@ -336,4 +324,24 @@ export class DbConfigStore extends DisposableObject {
       },
     };
   }
+
+  private validateLocalListName(listName: string): void {
+    if (listName === "") {
+      throw Error("List name cannot be empty");
+    }
+
+    if (this.doesLocalListExist(listName)) {
+      throw Error(`A local list with the name '${listName}' already exists`);
+    }
+  }
+
+  private validateRemoteListName(listName: string): void {
+    if (listName === "") {
+      throw Error("List name cannot be empty");
+    }
+
+    if (this.doesRemoteListExist(listName)) {
+      throw Error(`A remote list with the name '${listName}' already exists`);
+    }
+  }
 }

extensions/ql-vscode/test/unit-tests/databases/config/db-config-store.test.ts

@@ -16,151 +16,155 @@ describe("db config store", () => {
     await remove(tempWorkspaceStoragePath);
   });
 
-  it("should create a new config if one does not exist", async () => {
-    const app = createMockApp({
-      extensionPath,
-      workspaceStoragePath: tempWorkspaceStoragePath,
+  describe("initialize", () => {
+    it("should create a new config if one does not exist", async () => {
+      const app = createMockApp({
+        extensionPath,
+        workspaceStoragePath: tempWorkspaceStoragePath,
+      });
+
+      const configPath = join(
+        tempWorkspaceStoragePath,
+        "workspace-databases.json",
+      );
+
+      const configStore = new DbConfigStore(app);
+      await configStore.initialize();
+
+      expect(await pathExists(configPath)).toBe(true);
+
+      const config = configStore.getConfig().value;
+      expect(config.databases.remote.repositoryLists).toHaveLength(0);
+      expect(config.databases.remote.owners).toHaveLength(0);
+      expect(config.databases.remote.repositories).toHaveLength(0);
+      expect(config.databases.local.lists).toHaveLength(0);
+      expect(config.databases.local.databases).toHaveLength(0);
+      expect(config.selected).toBeUndefined();
+
+      configStore.dispose();
     });
 
-    const configPath = join(
-      tempWorkspaceStoragePath,
-      "workspace-databases.json",
-    );
+    it("should load an existing config", async () => {
+      const app = createMockApp({
+        extensionPath,
+        workspaceStoragePath: testDataStoragePath,
+      });
+      const configStore = new DbConfigStore(app);
+      await configStore.initialize();
 
-    const configStore = new DbConfigStore(app);
-    await configStore.initialize();
+      const config = configStore.getConfig().value;
+      expect(config.databases.remote.repositoryLists).toHaveLength(1);
+      expect(config.databases.remote.repositoryLists[0]).toEqual({
+        name: "repoList1",
+        repositories: ["foo/bar", "foo/baz"],
+      });
+      expect(config.databases.remote.owners).toHaveLength(0);
+      expect(config.databases.remote.repositories).toHaveLength(3);
+      expect(config.databases.remote.repositories).toEqual([
+        "owner/repo1",
+        "owner/repo2",
+        "owner/repo3",
+      ]);
+      expect(config.databases.local.lists).toHaveLength(2);
+      expect(config.databases.local.lists[0]).toEqual({
+        name: "localList1",
+        databases: [
+          {
+            name: "foo/bar",
+            dateAdded: 1668096745193,
+            language: "go",
+            storagePath: "/path/to/database/",
+          },
+        ],
+      });
+      expect(config.databases.local.databases).toHaveLength(1);
+      expect(config.databases.local.databases[0]).toEqual({
+        name: "example-db",
+        dateAdded: 1668096927267,
+        language: "ruby",
+        storagePath: "/path/to/database/",
+      });
+      expect(config.selected).toEqual({
+        kind: "remoteUserDefinedList",
+        listName: "repoList1",
+      });
 
-    expect(await pathExists(configPath)).toBe(true);
-
-    const config = configStore.getConfig().value;
-    expect(config.databases.remote.repositoryLists).toHaveLength(0);
-    expect(config.databases.remote.owners).toHaveLength(0);
-    expect(config.databases.remote.repositories).toHaveLength(0);
-    expect(config.databases.local.lists).toHaveLength(0);
-    expect(config.databases.local.databases).toHaveLength(0);
-    expect(config.selected).toBeUndefined();
-
-    configStore.dispose();
-  });
-
-  it("should load an existing config", async () => {
-    const app = createMockApp({
-      extensionPath,
-      workspaceStoragePath: testDataStoragePath,
-    });
-    const configStore = new DbConfigStore(app);
-    await configStore.initialize();
-
-    const config = configStore.getConfig().value;
-    expect(config.databases.remote.repositoryLists).toHaveLength(1);
-    expect(config.databases.remote.repositoryLists[0]).toEqual({
-      name: "repoList1",
-      repositories: ["foo/bar", "foo/baz"],
-    });
-    expect(config.databases.remote.owners).toHaveLength(0);
-    expect(config.databases.remote.repositories).toHaveLength(3);
-    expect(config.databases.remote.repositories).toEqual([
-      "owner/repo1",
-      "owner/repo2",
-      "owner/repo3",
-    ]);
-    expect(config.databases.local.lists).toHaveLength(2);
-    expect(config.databases.local.lists[0]).toEqual({
-      name: "localList1",
-      databases: [
-        {
-          name: "foo/bar",
-          dateAdded: 1668096745193,
-          language: "go",
-          storagePath: "/path/to/database/",
-        },
-      ],
-    });
-    expect(config.databases.local.databases).toHaveLength(1);
-    expect(config.databases.local.databases[0]).toEqual({
-      name: "example-db",
-      dateAdded: 1668096927267,
-      language: "ruby",
-      storagePath: "/path/to/database/",
-    });
-    expect(config.selected).toEqual({
-      kind: "remoteUserDefinedList",
-      listName: "repoList1",
+      configStore.dispose();
     });
 
-    configStore.dispose();
-  });
+    it("should load an existing config without selected db", async () => {
+      const testDataStoragePathWithout = join(
+        __dirname,
+        "data",
+        "without-selected",
+      );
 
-  it("should load an existing config without selected db", async () => {
-    const testDataStoragePathWithout = join(
-      __dirname,
-      "data",
-      "without-selected",
-    );
+      const app = createMockApp({
+        extensionPath,
+        workspaceStoragePath: testDataStoragePathWithout,
+      });
 
-    const app = createMockApp({
-      extensionPath,
-      workspaceStoragePath: testDataStoragePathWithout,
+      const configStore = new DbConfigStore(app);
+      await configStore.initialize();
+
+      const config = configStore.getConfig().value;
+      expect(config.selected).toBeUndefined();
+
+      configStore.dispose();
     });
 
-    const configStore = new DbConfigStore(app);
-    await configStore.initialize();
+    it("should not allow modification of the config", async () => {
+      const app = createMockApp({
+        extensionPath,
+        workspaceStoragePath: testDataStoragePath,
+      });
+      const configStore = new DbConfigStore(app);
+      await configStore.initialize();
 
-    const config = configStore.getConfig().value;
-    expect(config.selected).toBeUndefined();
+      const config = configStore.getConfig().value;
+      config.databases.remote.repositoryLists = [];
 
-    configStore.dispose();
-  });
+      const reRetrievedConfig = configStore.getConfig().value;
+      expect(reRetrievedConfig.databases.remote.repositoryLists).toHaveLength(
+        1,
+      );
 
-  it("should not allow modification of the config", async () => {
-    const app = createMockApp({
-      extensionPath,
-      workspaceStoragePath: testDataStoragePath,
+      configStore.dispose();
     });
-    const configStore = new DbConfigStore(app);
-    await configStore.initialize();
 
-    const config = configStore.getConfig().value;
-    config.databases.remote.repositoryLists = [];
+    it("should set codeQLDatabasesExperimental.configError to true when config has error", async () => {
+      const testDataStoragePathInvalid = join(__dirname, "data", "invalid");
 
-    const reRetrievedConfig = configStore.getConfig().value;
-    expect(reRetrievedConfig.databases.remote.repositoryLists).toHaveLength(1);
+      const app = createMockApp({
+        extensionPath,
+        workspaceStoragePath: testDataStoragePathInvalid,
+      });
+      const configStore = new DbConfigStore(app);
+      await configStore.initialize();
 
-    configStore.dispose();
-  });
-
-  it("should set codeQLDatabasesExperimental.configError to true when config has error", async () => {
-    const testDataStoragePathInvalid = join(__dirname, "data", "invalid");
-
-    const app = createMockApp({
-      extensionPath,
-      workspaceStoragePath: testDataStoragePathInvalid,
+      expect(app.executeCommand).toBeCalledWith(
+        "setContext",
+        "codeQLDatabasesExperimental.configError",
+        true,
+      );
+      configStore.dispose();
     });
-    const configStore = new DbConfigStore(app);
-    await configStore.initialize();
 
-    expect(app.executeCommand).toBeCalledWith(
-      "setContext",
-      "codeQLDatabasesExperimental.configError",
-      true,
-    );
-    configStore.dispose();
-  });
+    it("should set codeQLDatabasesExperimental.configError to false when config is valid", async () => {
+      const app = createMockApp({
+        extensionPath,
+        workspaceStoragePath: testDataStoragePath,
+      });
+      const configStore = new DbConfigStore(app);
+      await configStore.initialize();
 
-  it("should set codeQLDatabasesExperimental.configError to false when config is valid", async () => {
-    const app = createMockApp({
-      extensionPath,
-      workspaceStoragePath: testDataStoragePath,
+      expect(app.executeCommand).toBeCalledWith(
+        "setContext",
+        "codeQLDatabasesExperimental.configError",
+        false,
+      );
+
+      configStore.dispose();
     });
-    const configStore = new DbConfigStore(app);
-    await configStore.initialize();
-
-    expect(app.executeCommand).toBeCalledWith(
-      "setContext",
-      "codeQLDatabasesExperimental.configError",
-      false,
-    );
-
-    configStore.dispose();
   });
 });