From dbba6972e147bdfc0e0f4c6105788b7b86635465 Mon Sep 17 00:00:00 2001 From: Nora Date: Mon, 5 Dec 2022 15:07:48 +0100 Subject: [PATCH 1/5] Run MRVA against remote DB --- .../ql-vscode/src/databases/db-module.ts | 20 ++++++++---- extensions/ql-vscode/src/extension.ts | 10 +++--- .../remote-queries/repository-selection.ts | 31 ++++++++++++++++++- .../src/remote-queries/run-remote-query.ts | 4 ++- .../variant-analysis-manager.ts | 3 ++ .../variant-analysis-manager.test.ts | 3 ++ 6 files changed, 59 insertions(+), 12 deletions(-) diff --git a/extensions/ql-vscode/src/databases/db-module.ts b/extensions/ql-vscode/src/databases/db-module.ts index ed7bef0d289..52bec696b9b 100644 --- a/extensions/ql-vscode/src/databases/db-module.ts +++ b/extensions/ql-vscode/src/databases/db-module.ts @@ -9,6 +9,16 @@ import { DbPanel } from "./ui/db-panel"; import { DbSelectionDecorationProvider } from "./ui/db-selection-decoration-provider"; export class DbModule extends DisposableObject { + public readonly dbManager: DbManager; + private readonly dbConfigStore: DbConfigStore; + + constructor(app: App) { + super(); + + this.dbConfigStore = new DbConfigStore(app); + this.dbManager = new DbManager(app, this.dbConfigStore); + } + public async initialize(app: App): Promise { if ( app.mode !== AppMode.Development || @@ -23,15 +33,13 @@ export class DbModule extends DisposableObject { void extLogger.log("Initializing database module"); - const dbConfigStore = new DbConfigStore(app); - await dbConfigStore.initialize(); + await this.dbConfigStore.initialize(); - const dbManager = new DbManager(app, dbConfigStore); - const dbPanel = new DbPanel(dbManager); + const dbPanel = new DbPanel(this.dbManager); await dbPanel.initialize(); this.push(dbPanel); - this.push(dbConfigStore); + this.push(this.dbConfigStore); const dbSelectionDecorationProvider = new DbSelectionDecorationProvider(); @@ -40,7 +48,7 @@ export class DbModule extends DisposableObject { } export async function initializeDbModule(app: App): Promise { - const dbModule = new DbModule(); + const dbModule = new DbModule(app); await dbModule.initialize(app); return dbModule; } diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index 30a2adbb959..51c9a027442 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -622,6 +622,11 @@ async function activateWithInstalledDistribution( ctx.subscriptions.push(localQueryResultsView); void extLogger.log("Initializing variant analysis manager."); + + const app = new ExtensionApp(ctx); + const dbModule = await initializeDbModule(app); + ctx.subscriptions.push(dbModule); + const variantAnalysisStorageDir = join( ctx.globalStorageUri.fsPath, "variant-analyses", @@ -636,6 +641,7 @@ async function activateWithInstalledDistribution( cliServer, variantAnalysisStorageDir, variantAnalysisResultsManager, + dbModule.dbManager, ); ctx.subscriptions.push(variantAnalysisManager); ctx.subscriptions.push(variantAnalysisResultsManager); @@ -1580,10 +1586,6 @@ async function activateWithInstalledDistribution( void extLogger.log("Reading query history"); await qhm.readQueryHistory(); - const app = new ExtensionApp(ctx); - const dbModule = await initializeDbModule(app); - ctx.subscriptions.push(dbModule); - void extLogger.log("Successfully finished extension initialization."); return { diff --git a/extensions/ql-vscode/src/remote-queries/repository-selection.ts b/extensions/ql-vscode/src/remote-queries/repository-selection.ts index 6aa8d4b558d..aab34e0f25c 100644 --- a/extensions/ql-vscode/src/remote-queries/repository-selection.ts +++ b/extensions/ql-vscode/src/remote-queries/repository-selection.ts @@ -4,9 +4,12 @@ import { extLogger } from "../common"; import { getRemoteRepositoryLists, getRemoteRepositoryListsPath, + isNewQueryRunExperienceEnabled, } from "../config"; import { OWNER_REGEX, REPO_REGEX } from "../pure/helpers-pure"; import { UserCancellationException } from "../commandRunner"; +import { DbManager } from "../databases/db-manager"; +import { DbItemKind } from "../databases/db-item"; export interface RepositorySelection { repositories?: string[]; @@ -30,7 +33,33 @@ interface RepoList { * Gets the repositories or repository lists to run the query against. * @returns The user selection. */ -export async function getRepositorySelection(): Promise { +export async function getRepositorySelection( + dbManager?: DbManager, +): Promise { + if (isNewQueryRunExperienceEnabled()) { + const selectedDbItem = await dbManager?.selectedDbItem(); + if (selectedDbItem) { + switch (selectedDbItem.kind) { + case DbItemKind.LocalDatabase || DbItemKind.LocalList: + throw new Error("Local databases and lists are not supported"); + case DbItemKind.RemoteSystemDefinedList: + return { repositoryLists: [selectedDbItem.listName] }; + case DbItemKind.RemoteUserDefinedList: + return { + repositories: selectedDbItem.repos.map((repo) => repo.repoFullName), + }; + case DbItemKind.RemoteOwner: + return { owners: [selectedDbItem.ownerName] }; + case DbItemKind.RemoteRepo: + return { repositories: [selectedDbItem.repoFullName] }; + } + } else { + throw new Error( + "Please select a remote database item to run the query against.", + ); + } + } + const quickPickItems = [ createCustomRepoQuickPickItem(), createAllReposOfOwnerQuickPickItem(), diff --git a/extensions/ql-vscode/src/remote-queries/run-remote-query.ts b/extensions/ql-vscode/src/remote-queries/run-remote-query.ts index 88d23035542..efb74960053 100644 --- a/extensions/ql-vscode/src/remote-queries/run-remote-query.ts +++ b/extensions/ql-vscode/src/remote-queries/run-remote-query.ts @@ -29,6 +29,7 @@ import { RepositorySelection, } from "./repository-selection"; import { Repository } from "./shared/repository"; +import { DbManager } from "../databases/db-manager"; export interface QlPack { name: string; @@ -213,6 +214,7 @@ export async function prepareRemoteQueryRun( uri: Uri | undefined, progress: ProgressCallback, token: CancellationToken, + dbManager?: DbManager, // the dbManager is only needed when the newQueryRunExperience is enabled ): Promise { if (!(await cliServer.cliConstraints.supportsRemoteQueries())) { throw new Error( @@ -232,7 +234,7 @@ export async function prepareRemoteQueryRun( message: "Determining query target language", }); - const repoSelection = await getRepositorySelection(); + const repoSelection = await getRepositorySelection(dbManager); if (!isValidSelection(repoSelection)) { throw new UserCancellationException("No repositories to query."); } diff --git a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts index 2d376a8a7da..5adc285064f 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts @@ -59,6 +59,7 @@ import { RepositoriesFilterSortStateWithIds, } from "../pure/variant-analysis-filter-sort"; import { URLSearchParams } from "url"; +import { DbManager } from "../databases/db-manager"; export class VariantAnalysisManager extends DisposableObject @@ -100,6 +101,7 @@ export class VariantAnalysisManager private readonly cliServer: CodeQLCliServer, private readonly storagePath: string, private readonly variantAnalysisResultsManager: VariantAnalysisResultsManager, + private readonly dbManager: DbManager, ) { super(); this.variantAnalysisMonitor = this.push( @@ -140,6 +142,7 @@ export class VariantAnalysisManager uri, progress, token, + this.dbManager, ); const queryName = getQueryName(queryMetadata, queryFile); diff --git a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-manager.test.ts b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-manager.test.ts index 185f008f328..784e59def82 100644 --- a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-manager.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-manager.test.ts @@ -56,6 +56,7 @@ import { defaultFilterSortState, SortKey, } from "../../../pure/variant-analysis-filter-sort"; +import { DbManager } from "../../../databases/db-manager"; // up to 3 minutes per test jest.setTimeout(3 * 60 * 1000); @@ -69,6 +70,7 @@ describe("Variant Analysis Manager", () => { let cancellationTokenSource: CancellationTokenSource; let variantAnalysisManager: VariantAnalysisManager; let variantAnalysisResultsManager: VariantAnalysisResultsManager; + let dbManager: DbManager; let variantAnalysis: VariantAnalysis; let scannedRepos: VariantAnalysisScannedRepository[]; @@ -107,6 +109,7 @@ describe("Variant Analysis Manager", () => { cli, storagePath, variantAnalysisResultsManager, + dbManager, ); }); From 728f80189bc3bb8dc6a0bec9dfade94b6fc82b46 Mon Sep 17 00:00:00 2001 From: Nora Date: Mon, 5 Dec 2022 16:21:35 +0100 Subject: [PATCH 2/5] Refactor selection method to allow all types of DbItem lists --- extensions/ql-vscode/src/remote-queries/repository-selection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/remote-queries/repository-selection.ts b/extensions/ql-vscode/src/remote-queries/repository-selection.ts index aab34e0f25c..a82ca44a3b6 100644 --- a/extensions/ql-vscode/src/remote-queries/repository-selection.ts +++ b/extensions/ql-vscode/src/remote-queries/repository-selection.ts @@ -37,7 +37,7 @@ export async function getRepositorySelection( dbManager?: DbManager, ): Promise { if (isNewQueryRunExperienceEnabled()) { - const selectedDbItem = await dbManager?.selectedDbItem(); + const selectedDbItem = await dbManager?.getSelectedDbItem(); if (selectedDbItem) { switch (selectedDbItem.kind) { case DbItemKind.LocalDatabase || DbItemKind.LocalList: From 18c3ce237eec3e2965d04d942996e24ffa940905 Mon Sep 17 00:00:00 2001 From: Nora Date: Tue, 6 Dec 2022 15:25:33 +0100 Subject: [PATCH 3/5] Reorder existing tests in new describe blog --- .../repository-selection.test.ts | 503 +++++++++--------- 1 file changed, 252 insertions(+), 251 deletions(-) diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts index 0fc2d12d662..408c7ff2f85 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts @@ -6,197 +6,215 @@ import * as config from "../../../config"; import { getRepositorySelection } from "../../../remote-queries/repository-selection"; describe("repository selection", () => { - let quickPickSpy: jest.SpiedFunction; - let showInputBoxSpy: jest.SpiedFunction; - - let getRemoteRepositoryListsSpy: jest.SpiedFunction< - typeof config.getRemoteRepositoryLists - >; - let getRemoteRepositoryListsPathSpy: jest.SpiedFunction< - typeof config.getRemoteRepositoryListsPath - >; - - let pathExistsStub: jest.SpiedFunction; - let fsStatStub: jest.SpiedFunction; - let fsReadFileStub: jest.SpiedFunction; - - beforeEach(() => { - quickPickSpy = jest - .spyOn(window, "showQuickPick") - .mockResolvedValue(undefined); - showInputBoxSpy = jest - .spyOn(window, "showInputBox") - .mockResolvedValue(undefined); - - getRemoteRepositoryListsSpy = jest - .spyOn(config, "getRemoteRepositoryLists") - .mockReturnValue(undefined); - getRemoteRepositoryListsPathSpy = jest - .spyOn(config, "getRemoteRepositoryListsPath") - .mockReturnValue(undefined); - - pathExistsStub = jest - .spyOn(fs, "pathExists") - .mockImplementation(() => false); - fsStatStub = jest - .spyOn(fs, "stat") - .mockRejectedValue(new Error("not found")); - fsReadFileStub = jest - .spyOn(fs, "readFile") - .mockRejectedValue(new Error("not found")); - }); - - describe("repo lists from settings", () => { - it("should allow selection from repo lists from your pre-defined config", async () => { - // Fake return values - quickPickSpy.mockResolvedValue({ - repositories: ["foo/bar", "foo/baz"], - } as unknown as QuickPickItem); - getRemoteRepositoryListsSpy.mockReturnValue({ - list1: ["foo/bar", "foo/baz"], - list2: [], - }); - - // Make the function call - const repoSelection = await getRepositorySelection(); - - // Check that the return value is correct - expect(repoSelection.repositoryLists).toBeUndefined(); - expect(repoSelection.owners).toBeUndefined(); - expect(repoSelection.repositories).toEqual(["foo/bar", "foo/baz"]); + describe("newQueryRunExperience false", () => { + let quickPickSpy: jest.SpiedFunction; + let showInputBoxSpy: jest.SpiedFunction; + + let getRemoteRepositoryListsSpy: jest.SpiedFunction< + typeof config.getRemoteRepositoryLists + >; + let getRemoteRepositoryListsPathSpy: jest.SpiedFunction< + typeof config.getRemoteRepositoryListsPath + >; + + let pathExistsStub: jest.SpiedFunction; + let fsStatStub: jest.SpiedFunction; + let fsReadFileStub: jest.SpiedFunction; + + beforeEach(() => { + quickPickSpy = jest + .spyOn(window, "showQuickPick") + .mockResolvedValue(undefined); + showInputBoxSpy = jest + .spyOn(window, "showInputBox") + .mockResolvedValue(undefined); + + getRemoteRepositoryListsSpy = jest + .spyOn(config, "getRemoteRepositoryLists") + .mockReturnValue(undefined); + getRemoteRepositoryListsPathSpy = jest + .spyOn(config, "getRemoteRepositoryListsPath") + .mockReturnValue(undefined); + + pathExistsStub = jest + .spyOn(fs, "pathExists") + .mockImplementation(() => false); + fsStatStub = jest + .spyOn(fs, "stat") + .mockRejectedValue(new Error("not found")); + fsReadFileStub = jest + .spyOn(fs, "readFile") + .mockRejectedValue(new Error("not found")); }); - }); - - describe("system level repo lists", () => { - it("should allow selection from repo lists defined at the system level", async () => { - // Fake return values - quickPickSpy.mockResolvedValue({ - repositoryList: "top_100", - } as unknown as QuickPickItem); - getRemoteRepositoryListsSpy.mockReturnValue({ - list1: ["foo/bar", "foo/baz"], - list2: [], - }); + describe("repo lists from settings", () => { + it("should allow selection from repo lists from your pre-defined config", async () => { + // Fake return values + quickPickSpy.mockResolvedValue({ + repositories: ["foo/bar", "foo/baz"], + } as unknown as QuickPickItem); + getRemoteRepositoryListsSpy.mockReturnValue({ + list1: ["foo/bar", "foo/baz"], + list2: [], + }); - // Make the function call - const repoSelection = await getRepositorySelection(); + // Make the function call + const repoSelection = await getRepositorySelection(); - // Check that the return value is correct - expect(repoSelection.repositories).toBeUndefined(); - expect(repoSelection.owners).toBeUndefined(); - expect(repoSelection.repositoryLists).toEqual(["top_100"]); + // Check that the return value is correct + expect(repoSelection.repositoryLists).toBeUndefined(); + expect(repoSelection.owners).toBeUndefined(); + expect(repoSelection.repositories).toEqual(["foo/bar", "foo/baz"]); + }); }); - }); - describe("custom owner", () => { - // Test the owner regex in various "good" cases - const goodOwners = [ - "owner", - "owner-with-hyphens", - "ownerWithNumbers58", - "owner_with_underscores", - "owner.with.periods.", - ]; - goodOwners.forEach((owner) => { - it(`should run on a valid owner that you enter in the text box: ${owner}`, async () => { + describe("system level repo lists", () => { + it("should allow selection from repo lists defined at the system level", async () => { // Fake return values quickPickSpy.mockResolvedValue({ - useAllReposOfOwner: true, + repositoryList: "top_100", } as unknown as QuickPickItem); - getRemoteRepositoryListsSpy.mockReturnValue({}); // no pre-defined repo lists - showInputBoxSpy.mockResolvedValue(owner); + getRemoteRepositoryListsSpy.mockReturnValue({ + list1: ["foo/bar", "foo/baz"], + list2: [], + }); // Make the function call const repoSelection = await getRepositorySelection(); // Check that the return value is correct expect(repoSelection.repositories).toBeUndefined(); - expect(repoSelection.repositoryLists).toBeUndefined(); - expect(repoSelection.owners).toEqual([owner]); + expect(repoSelection.owners).toBeUndefined(); + expect(repoSelection.repositoryLists).toEqual(["top_100"]); }); }); - // Test the owner regex in various "bad" cases - const badOwners = ["invalid&owner", "owner-with-repo/repo"]; - badOwners.forEach((owner) => { - it(`should show an error message if you enter an invalid owner in the text box: ${owner}`, async () => { - // Fake return values + describe("custom owner", () => { + // Test the owner regex in various "good" cases + const goodOwners = [ + "owner", + "owner-with-hyphens", + "ownerWithNumbers58", + "owner_with_underscores", + "owner.with.periods.", + ]; + goodOwners.forEach((owner) => { + it(`should run on a valid owner that you enter in the text box: ${owner}`, async () => { + // Fake return values + quickPickSpy.mockResolvedValue({ + useAllReposOfOwner: true, + } as unknown as QuickPickItem); + getRemoteRepositoryListsSpy.mockReturnValue({}); // no pre-defined repo lists + showInputBoxSpy.mockResolvedValue(owner); + + // Make the function call + const repoSelection = await getRepositorySelection(); + + // Check that the return value is correct + expect(repoSelection.repositories).toBeUndefined(); + expect(repoSelection.repositoryLists).toBeUndefined(); + expect(repoSelection.owners).toEqual([owner]); + }); + }); + + // Test the owner regex in various "bad" cases + const badOwners = ["invalid&owner", "owner-with-repo/repo"]; + badOwners.forEach((owner) => { + it(`should show an error message if you enter an invalid owner in the text box: ${owner}`, async () => { + // Fake return values + quickPickSpy.mockResolvedValue({ + useAllReposOfOwner: true, + } as unknown as QuickPickItem); + getRemoteRepositoryListsSpy.mockReturnValue({}); // no pre-defined repo lists + showInputBoxSpy.mockResolvedValue(owner); + + // Function call should throw a UserCancellationException + await expect(getRepositorySelection()).rejects.toThrow( + `Invalid user or organization: ${owner}`, + ); + }); + }); + + it("should be ok for the user to change their mind", async () => { quickPickSpy.mockResolvedValue({ useAllReposOfOwner: true, } as unknown as QuickPickItem); - getRemoteRepositoryListsSpy.mockReturnValue({}); // no pre-defined repo lists - showInputBoxSpy.mockResolvedValue(owner); + getRemoteRepositoryListsSpy.mockReturnValue({}); + + // The user pressed escape to cancel the operation + showInputBoxSpy.mockResolvedValue(undefined); - // Function call should throw a UserCancellationException await expect(getRepositorySelection()).rejects.toThrow( - `Invalid user or organization: ${owner}`, + "No repositories selected", + ); + await expect(getRepositorySelection()).rejects.toThrow( + UserCancellationException, ); }); }); - it("should be ok for the user to change their mind", async () => { - quickPickSpy.mockResolvedValue({ - useAllReposOfOwner: true, - } as unknown as QuickPickItem); - getRemoteRepositoryListsSpy.mockReturnValue({}); - - // The user pressed escape to cancel the operation - showInputBoxSpy.mockResolvedValue(undefined); - - await expect(getRepositorySelection()).rejects.toThrow( - "No repositories selected", - ); - await expect(getRepositorySelection()).rejects.toThrow( - UserCancellationException, - ); - }); - }); - - describe("custom repo", () => { - // Test the repo regex in various "good" cases - const goodRepos = [ - "owner/repo", - "owner_with.symbols-/repo.with-symbols_", - "ownerWithNumbers58/repoWithNumbers37", - ]; - goodRepos.forEach((repo) => { - it(`should run on a valid repo that you enter in the text box: ${repo}`, async () => { - // Fake return values - quickPickSpy.mockResolvedValue({ - useCustomRepo: true, - } as unknown as QuickPickItem); - getRemoteRepositoryListsSpy.mockReturnValue({}); // no pre-defined repo lists - showInputBoxSpy.mockResolvedValue(repo); - - // Make the function call - const repoSelection = await getRepositorySelection(); + describe("custom repo", () => { + // Test the repo regex in various "good" cases + const goodRepos = [ + "owner/repo", + "owner_with.symbols-/repo.with-symbols_", + "ownerWithNumbers58/repoWithNumbers37", + ]; + goodRepos.forEach((repo) => { + it(`should run on a valid repo that you enter in the text box: ${repo}`, async () => { + // Fake return values + quickPickSpy.mockResolvedValue({ + useCustomRepo: true, + } as unknown as QuickPickItem); + getRemoteRepositoryListsSpy.mockReturnValue({}); // no pre-defined repo lists + showInputBoxSpy.mockResolvedValue(repo); + + // Make the function call + const repoSelection = await getRepositorySelection(); + + // Check that the return value is correct + expect(repoSelection.repositoryLists).toBeUndefined(); + expect(repoSelection.owners).toBeUndefined(); + expect(repoSelection.repositories).toEqual([repo]); + }); + }); - // Check that the return value is correct - expect(repoSelection.repositoryLists).toBeUndefined(); - expect(repoSelection.owners).toBeUndefined(); - expect(repoSelection.repositories).toEqual([repo]); + // Test the repo regex in various "bad" cases + const badRepos = [ + "invalid*owner/repo", + "owner/repo+some&invalid&stuff", + "owner-with-no-repo/", + "/repo-with-no-owner", + ]; + badRepos.forEach((repo) => { + it(`should show an error message if you enter an invalid repo in the text box: ${repo}`, async () => { + // Fake return values + quickPickSpy.mockResolvedValue({ + useCustomRepo: true, + } as unknown as QuickPickItem); + getRemoteRepositoryListsSpy.mockReturnValue({}); // no pre-defined repo lists + showInputBoxSpy.mockResolvedValue(repo); + + // Function call should throw a UserCancellationException + await expect(getRepositorySelection()).rejects.toThrow( + "Invalid repository format", + ); + await expect(getRepositorySelection()).rejects.toThrow( + UserCancellationException, + ); + }); }); - }); - // Test the repo regex in various "bad" cases - const badRepos = [ - "invalid*owner/repo", - "owner/repo+some&invalid&stuff", - "owner-with-no-repo/", - "/repo-with-no-owner", - ]; - badRepos.forEach((repo) => { - it(`should show an error message if you enter an invalid repo in the text box: ${repo}`, async () => { - // Fake return values + it("should be ok for the user to change their mind", async () => { quickPickSpy.mockResolvedValue({ useCustomRepo: true, } as unknown as QuickPickItem); - getRemoteRepositoryListsSpy.mockReturnValue({}); // no pre-defined repo lists - showInputBoxSpy.mockResolvedValue(repo); + getRemoteRepositoryListsSpy.mockReturnValue({}); + + // The user pressed escape to cancel the operation + showInputBoxSpy.mockResolvedValue(undefined); - // Function call should throw a UserCancellationException await expect(getRepositorySelection()).rejects.toThrow( - "Invalid repository format", + "No repositories selected", ); await expect(getRepositorySelection()).rejects.toThrow( UserCancellationException, @@ -204,113 +222,96 @@ describe("repository selection", () => { }); }); - it("should be ok for the user to change their mind", async () => { - quickPickSpy.mockResolvedValue({ - useCustomRepo: true, - } as unknown as QuickPickItem); - getRemoteRepositoryListsSpy.mockReturnValue({}); - - // The user pressed escape to cancel the operation - showInputBoxSpy.mockResolvedValue(undefined); - - await expect(getRepositorySelection()).rejects.toThrow( - "No repositories selected", - ); - await expect(getRepositorySelection()).rejects.toThrow( - UserCancellationException, - ); - }); - }); - - describe("external repository lists file", () => { - it("should fail if path does not exist", async () => { - const fakeFilePath = "/path/that/does/not/exist.json"; - getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); - pathExistsStub.mockImplementation(() => false); + describe("external repository lists file", () => { + it("should fail if path does not exist", async () => { + const fakeFilePath = "/path/that/does/not/exist.json"; + getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); + pathExistsStub.mockImplementation(() => false); - await expect(getRepositorySelection()).rejects.toThrow( - `External repository lists file does not exist at ${fakeFilePath}`, - ); - }); + await expect(getRepositorySelection()).rejects.toThrow( + `External repository lists file does not exist at ${fakeFilePath}`, + ); + }); - it("should fail if path points to directory", async () => { - const fakeFilePath = "/path/to/dir"; - getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); - pathExistsStub.mockImplementation(() => true); - fsStatStub.mockResolvedValue({ isDirectory: () => true } as any); + it("should fail if path points to directory", async () => { + const fakeFilePath = "/path/to/dir"; + getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); + pathExistsStub.mockImplementation(() => true); + fsStatStub.mockResolvedValue({ isDirectory: () => true } as any); - await expect(getRepositorySelection()).rejects.toThrow( - "External repository lists path should not point to a directory", - ); - }); + await expect(getRepositorySelection()).rejects.toThrow( + "External repository lists path should not point to a directory", + ); + }); - it("should fail if file does not have valid JSON", async () => { - const fakeFilePath = "/path/to/file.json"; - getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); - pathExistsStub.mockImplementation(() => true); - fsStatStub.mockResolvedValue({ isDirectory: () => false } as any); - fsReadFileStub.mockResolvedValue("not-json" as any as Buffer); + it("should fail if file does not have valid JSON", async () => { + const fakeFilePath = "/path/to/file.json"; + getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); + pathExistsStub.mockImplementation(() => true); + fsStatStub.mockResolvedValue({ isDirectory: () => false } as any); + fsReadFileStub.mockResolvedValue("not-json" as any as Buffer); - await expect(getRepositorySelection()).rejects.toThrow( - "Invalid repository lists file. It should contain valid JSON.", - ); - }); + await expect(getRepositorySelection()).rejects.toThrow( + "Invalid repository lists file. It should contain valid JSON.", + ); + }); - it("should fail if file contains array", async () => { - const fakeFilePath = "/path/to/file.json"; - getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); - pathExistsStub.mockImplementation(() => true); - fsStatStub.mockResolvedValue({ isDirectory: () => false } as any); - fsReadFileStub.mockResolvedValue("[]" as any as Buffer); + it("should fail if file contains array", async () => { + const fakeFilePath = "/path/to/file.json"; + getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); + pathExistsStub.mockImplementation(() => true); + fsStatStub.mockResolvedValue({ isDirectory: () => false } as any); + fsReadFileStub.mockResolvedValue("[]" as any as Buffer); - await expect(getRepositorySelection()).rejects.toThrow( - "Invalid repository lists file. It should be an object mapping names to a list of repositories.", - ); - }); + await expect(getRepositorySelection()).rejects.toThrow( + "Invalid repository lists file. It should be an object mapping names to a list of repositories.", + ); + }); - it("should fail if file does not contain repo lists in the right format", async () => { - const fakeFilePath = "/path/to/file.json"; - getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); - pathExistsStub.mockImplementation(() => true); - fsStatStub.mockResolvedValue({ isDirectory: () => false } as any); - const repoLists = { - list1: "owner1/repo1", - }; - fsReadFileStub.mockResolvedValue( - JSON.stringify(repoLists) as any as Buffer, - ); - - await expect(getRepositorySelection()).rejects.toThrow( - "Invalid repository lists file. It should contain an array of repositories for each list.", - ); - }); + it("should fail if file does not contain repo lists in the right format", async () => { + const fakeFilePath = "/path/to/file.json"; + getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); + pathExistsStub.mockImplementation(() => true); + fsStatStub.mockResolvedValue({ isDirectory: () => false } as any); + const repoLists = { + list1: "owner1/repo1", + }; + fsReadFileStub.mockResolvedValue( + JSON.stringify(repoLists) as any as Buffer, + ); - it("should get repo lists from file", async () => { - const fakeFilePath = "/path/to/file.json"; - getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); - pathExistsStub.mockImplementation(() => true); - fsStatStub.mockResolvedValue({ isDirectory: () => false } as any); - const repoLists = { - list1: ["owner1/repo1", "owner2/repo2"], - list2: ["owner3/repo3"], - }; - fsReadFileStub.mockResolvedValue( - JSON.stringify(repoLists) as any as Buffer, - ); - getRemoteRepositoryListsSpy.mockReturnValue({ - list3: ["onwer4/repo4"], - list4: [], + await expect(getRepositorySelection()).rejects.toThrow( + "Invalid repository lists file. It should contain an array of repositories for each list.", + ); }); - quickPickSpy.mockResolvedValue({ - repositories: ["owner3/repo3"], - } as unknown as QuickPickItem); + it("should get repo lists from file", async () => { + const fakeFilePath = "/path/to/file.json"; + getRemoteRepositoryListsPathSpy.mockReturnValue(fakeFilePath); + pathExistsStub.mockImplementation(() => true); + fsStatStub.mockResolvedValue({ isDirectory: () => false } as any); + const repoLists = { + list1: ["owner1/repo1", "owner2/repo2"], + list2: ["owner3/repo3"], + }; + fsReadFileStub.mockResolvedValue( + JSON.stringify(repoLists) as any as Buffer, + ); + getRemoteRepositoryListsSpy.mockReturnValue({ + list3: ["onwer4/repo4"], + list4: [], + }); - const repoSelection = await getRepositorySelection(); + quickPickSpy.mockResolvedValue({ + repositories: ["owner3/repo3"], + } as unknown as QuickPickItem); - expect(repoSelection.repositoryLists).toBeUndefined(); - expect(repoSelection.owners).toBeUndefined(); - expect(repoSelection.repositories).toEqual(["owner3/repo3"]); + const repoSelection = await getRepositorySelection(); + + expect(repoSelection.repositoryLists).toBeUndefined(); + expect(repoSelection.owners).toBeUndefined(); + expect(repoSelection.repositories).toEqual(["owner3/repo3"]); + }); }); }); }); From 7a3d5c1925fbb238d8e84c7d61e0e05996195bc7 Mon Sep 17 00:00:00 2001 From: Nora Date: Tue, 6 Dec 2022 15:28:14 +0100 Subject: [PATCH 4/5] Add unit tests --- .../remote-queries/repository-selection.ts | 2 +- .../repository-selection.test.ts | 108 ++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/remote-queries/repository-selection.ts b/extensions/ql-vscode/src/remote-queries/repository-selection.ts index a82ca44a3b6..c052315b114 100644 --- a/extensions/ql-vscode/src/remote-queries/repository-selection.ts +++ b/extensions/ql-vscode/src/remote-queries/repository-selection.ts @@ -41,7 +41,7 @@ export async function getRepositorySelection( if (selectedDbItem) { switch (selectedDbItem.kind) { case DbItemKind.LocalDatabase || DbItemKind.LocalList: - throw new Error("Local databases and lists are not supported"); + throw new Error("Local databases and lists are not supported yet."); case DbItemKind.RemoteSystemDefinedList: return { repositoryLists: [selectedDbItem.listName] }; case DbItemKind.RemoteUserDefinedList: diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts index 408c7ff2f85..b7392e48d0d 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts @@ -4,8 +4,116 @@ import { UserCancellationException } from "../../../commandRunner"; import * as config from "../../../config"; import { getRepositorySelection } from "../../../remote-queries/repository-selection"; +import { DbManager } from "../../../databases/db-manager"; +import { DbItemKind } from "../../../databases/db-item"; describe("repository selection", () => { + describe("newQueryRunExperience true", () => { + beforeEach(() => { + jest + .spyOn(config, "isNewQueryRunExperienceEnabled") + .mockReturnValue(true); + }); + + it("should throw error when no database item is selected", async () => { + const dbManager = { + getSelectedDbItem: jest.fn(() => undefined), + } as any as DbManager; + + await expect(getRepositorySelection(dbManager)).rejects.toThrow( + Error("Please select a remote database item to run the query against."), + ); + }); + + it("should throw error when local database item is selected", async () => { + const dbManager = { + getSelectedDbItem: jest.fn(() => { + return { + kind: DbItemKind.LocalDatabase, + }; + }), + } as any as DbManager; + + await expect(getRepositorySelection(dbManager)).rejects.toThrow( + Error("Local databases and lists are not supported yet."), + ); + }); + + it("should return correct selection when remote system defined list is selected", async () => { + const dbManager = { + getSelectedDbItem: jest.fn(() => { + return { + kind: DbItemKind.RemoteSystemDefinedList, + listName: "top_10", + }; + }), + } as any as DbManager; + + const repoSelection = await getRepositorySelection(dbManager); + + expect(repoSelection.repositoryLists).toEqual(["top_10"]); + expect(repoSelection.owners).toBeUndefined(); + expect(repoSelection.repositories).toBeUndefined(); + }); + + it("should return correct selection when remote user defined list is selected", async () => { + const dbManager = { + getSelectedDbItem: jest.fn(() => { + return { + kind: DbItemKind.RemoteUserDefinedList, + repos: [ + { repoFullName: "owner1/repo1" }, + { repoFullName: "owner1/repo2" }, + ], + }; + }), + } as any as DbManager; + + const repoSelection = await getRepositorySelection(dbManager); + + expect(repoSelection.repositoryLists).toBeUndefined(); + expect(repoSelection.owners).toBeUndefined(); + expect(repoSelection.repositories).toEqual([ + "owner1/repo1", + "owner1/repo2", + ]); + }); + + it("should return correct selection when remote owner is selected", async () => { + const dbManager = { + getSelectedDbItem: jest.fn(() => { + return { + kind: DbItemKind.RemoteOwner, + ownerName: "owner2", + }; + }), + } as any as DbManager; + + const repoSelection = await getRepositorySelection(dbManager); + + expect(repoSelection.repositoryLists).toBeUndefined(); + expect(repoSelection.owners).toEqual(["owner2"]); + expect(repoSelection.repositories).toBeUndefined(); + }); + + it("should return correct selection when remote repo is selected", async () => { + const dbManager = { + getSelectedDbItem: jest.fn(() => { + return { + kind: DbItemKind.RemoteRepo, + repoFullName: "owner1/repo2", + }; + }), + } as any as DbManager; + + const repoSelection = await getRepositorySelection(dbManager); + + expect(repoSelection.repositoryLists).toBeUndefined(); + expect(repoSelection.owners).toBeUndefined(); + expect(repoSelection.repositories).toEqual(["owner1/repo2"]); + }); + }); + describe("newQueryRunExperience false", () => { let quickPickSpy: jest.SpiedFunction; let showInputBoxSpy: jest.SpiedFunction; From 323862a828cd22f24958ce258ee557283a85ccf4 Mon Sep 17 00:00:00 2001 From: Nora Date: Wed, 7 Dec 2022 12:22:47 +0100 Subject: [PATCH 5/5] Merge comments --- .../remote-queries/repository-selection.ts | 4 +- .../repository-selection.test.ts | 80 ++++++++----------- 2 files changed, 35 insertions(+), 49 deletions(-) diff --git a/extensions/ql-vscode/src/remote-queries/repository-selection.ts b/extensions/ql-vscode/src/remote-queries/repository-selection.ts index c052315b114..76c59414434 100644 --- a/extensions/ql-vscode/src/remote-queries/repository-selection.ts +++ b/extensions/ql-vscode/src/remote-queries/repository-selection.ts @@ -37,7 +37,7 @@ export async function getRepositorySelection( dbManager?: DbManager, ): Promise { if (isNewQueryRunExperienceEnabled()) { - const selectedDbItem = await dbManager?.getSelectedDbItem(); + const selectedDbItem = dbManager?.getSelectedDbItem(); if (selectedDbItem) { switch (selectedDbItem.kind) { case DbItemKind.LocalDatabase || DbItemKind.LocalList: @@ -55,7 +55,7 @@ export async function getRepositorySelection( } } else { throw new Error( - "Please select a remote database item to run the query against.", + "Please select a remote database to run the query against.", ); } } diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts index b7392e48d0d..c307bcd2b93 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts @@ -5,7 +5,7 @@ import { UserCancellationException } from "../../../commandRunner"; import * as config from "../../../config"; import { getRepositorySelection } from "../../../remote-queries/repository-selection"; import { DbManager } from "../../../databases/db-manager"; -import { DbItemKind } from "../../../databases/db-item"; +import { DbItem, DbItemKind } from "../../../databases/db-item"; describe("repository selection", () => { describe("newQueryRunExperience true", () => { @@ -16,23 +16,17 @@ describe("repository selection", () => { }); it("should throw error when no database item is selected", async () => { - const dbManager = { - getSelectedDbItem: jest.fn(() => undefined), - } as any as DbManager; + const dbManager = setUpDbManager(undefined); await expect(getRepositorySelection(dbManager)).rejects.toThrow( - Error("Please select a remote database item to run the query against."), + Error("Please select a remote database to run the query against."), ); }); it("should throw error when local database item is selected", async () => { - const dbManager = { - getSelectedDbItem: jest.fn(() => { - return { - kind: DbItemKind.LocalDatabase, - }; - }), - } as any as DbManager; + const dbManager = setUpDbManager({ + kind: DbItemKind.LocalDatabase, + } as DbItem); await expect(getRepositorySelection(dbManager)).rejects.toThrow( Error("Local databases and lists are not supported yet."), @@ -40,14 +34,10 @@ describe("repository selection", () => { }); it("should return correct selection when remote system defined list is selected", async () => { - const dbManager = { - getSelectedDbItem: jest.fn(() => { - return { - kind: DbItemKind.RemoteSystemDefinedList, - listName: "top_10", - }; - }), - } as any as DbManager; + const dbManager = setUpDbManager({ + kind: DbItemKind.RemoteSystemDefinedList, + listName: "top_10", + } as DbItem); const repoSelection = await getRepositorySelection(dbManager); @@ -57,17 +47,13 @@ describe("repository selection", () => { }); it("should return correct selection when remote user defined list is selected", async () => { - const dbManager = { - getSelectedDbItem: jest.fn(() => { - return { - kind: DbItemKind.RemoteUserDefinedList, - repos: [ - { repoFullName: "owner1/repo1" }, - { repoFullName: "owner1/repo2" }, - ], - }; - }), - } as any as DbManager; + const dbManager = setUpDbManager({ + kind: DbItemKind.RemoteUserDefinedList, + repos: [ + { repoFullName: "owner1/repo1" }, + { repoFullName: "owner1/repo2" }, + ], + } as DbItem); const repoSelection = await getRepositorySelection(dbManager); @@ -80,14 +66,10 @@ describe("repository selection", () => { }); it("should return correct selection when remote owner is selected", async () => { - const dbManager = { - getSelectedDbItem: jest.fn(() => { - return { - kind: DbItemKind.RemoteOwner, - ownerName: "owner2", - }; - }), - } as any as DbManager; + const dbManager = setUpDbManager({ + kind: DbItemKind.RemoteOwner, + ownerName: "owner2", + } as DbItem); const repoSelection = await getRepositorySelection(dbManager); @@ -97,14 +79,10 @@ describe("repository selection", () => { }); it("should return correct selection when remote repo is selected", async () => { - const dbManager = { - getSelectedDbItem: jest.fn(() => { - return { - kind: DbItemKind.RemoteRepo, - repoFullName: "owner1/repo2", - }; - }), - } as any as DbManager; + const dbManager = setUpDbManager({ + kind: DbItemKind.RemoteRepo, + repoFullName: "owner1/repo2", + } as DbItem); const repoSelection = await getRepositorySelection(dbManager); @@ -112,6 +90,14 @@ describe("repository selection", () => { expect(repoSelection.owners).toBeUndefined(); expect(repoSelection.repositories).toEqual(["owner1/repo2"]); }); + + function setUpDbManager(response: DbItem | undefined): DbManager { + return { + getSelectedDbItem: jest.fn(() => { + return response; + }), + } as any as DbManager; + } }); describe("newQueryRunExperience false", () => {