From 301149fd323d9f3ddf75969278bcf6b9a791f3d3 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 24 May 2023 17:49:41 +0100 Subject: [PATCH 1/4] Don't create array entry if workspace contains no queries --- .../src/queries-panel/query-discovery.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/extensions/ql-vscode/src/queries-panel/query-discovery.ts b/extensions/ql-vscode/src/queries-panel/query-discovery.ts index 00322d634..b9369a3a3 100644 --- a/extensions/ql-vscode/src/queries-panel/query-discovery.ts +++ b/extensions/ql-vscode/src/queries-panel/query-discovery.ts @@ -99,22 +99,26 @@ export class QueryDiscovery ): Promise { const rootDirectories = []; for (const workspaceFolder of workspaceFolders) { - rootDirectories.push( - await this.discoverQueriesInWorkspace(workspaceFolder), - ); + const root = await this.discoverQueriesInWorkspace(workspaceFolder); + if (root !== undefined) { + rootDirectories.push(root); + } } return rootDirectories; } private async discoverQueriesInWorkspace( workspaceFolder: WorkspaceFolder, - ): Promise { + ): Promise { const fullPath = workspaceFolder.uri.fsPath; const name = workspaceFolder.name; - const rootDirectory = new FileTreeDirectory(fullPath, name); - const resolvedQueries = await this.cliServer.resolveQueries(fullPath); + if (resolvedQueries.length === 0) { + return undefined; + } + + const rootDirectory = new FileTreeDirectory(fullPath, name); for (const queryPath of resolvedQueries) { const relativePath = normalize(relative(fullPath, queryPath)); const dirName = dirname(relativePath); From f236e65f684697d836d7c239f112d3a17df9af92 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 24 May 2023 16:23:25 +0100 Subject: [PATCH 2/4] Add integration tests of QueryDiscovery --- .../src/queries-panel/query-discovery.ts | 2 +- .../queries-panel/query-discovery.test.ts | 221 ++++++++++++++++++ 2 files changed, 222 insertions(+), 1 deletion(-) create mode 100644 extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts diff --git a/extensions/ql-vscode/src/queries-panel/query-discovery.ts b/extensions/ql-vscode/src/queries-panel/query-discovery.ts index b9369a3a3..bde646fbb 100644 --- a/extensions/ql-vscode/src/queries-panel/query-discovery.ts +++ b/extensions/ql-vscode/src/queries-panel/query-discovery.ts @@ -12,7 +12,7 @@ import { QueryDiscoverer } from "./query-tree-data-provider"; /** * The results of discovering queries. */ -interface QueryDiscoveryResults { +export interface QueryDiscoveryResults { /** * A tree of directories and query files. * May have multiple roots because of multiple workspaces. diff --git a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts new file mode 100644 index 000000000..0496f8712 --- /dev/null +++ b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts @@ -0,0 +1,221 @@ +import { + EventEmitter, + FileSystemWatcher, + Uri, + WorkspaceFoldersChangeEvent, + workspace, +} from "vscode"; +import { CodeQLCliServer } from "../../../../src/codeql-cli/cli"; +import { + QueryDiscovery, + QueryDiscoveryResults, +} from "../../../../src/queries-panel/query-discovery"; +import { createMockApp } from "../../../__mocks__/appMock"; +import { mockedObject } from "../../utils/mocking.helpers"; +import { basename, join } from "path"; +import { sleep } from "../../../../src/pure/time"; + +describe("QueryDiscovery", () => { + beforeEach(() => { + expect(workspace.workspaceFolders?.length).toEqual(1); + }); + + describe("queries", () => { + it("should return empty list when no QL files are present", async () => { + const resolveQueries = jest.fn().mockResolvedValue([]); + const cli = mockedObject({ + resolveQueries, + }); + + const discovery = new QueryDiscovery(createMockApp({}), cli); + const results: QueryDiscoveryResults = await ( + discovery as any + ).discover(); + + expect(results.queries).toEqual([]); + expect(resolveQueries).toHaveBeenCalledTimes(1); + }); + + it("should organise query files into directories", async () => { + const workspaceRoot = workspace.workspaceFolders![0].uri.fsPath; + const cli = mockedObject({ + resolveQueries: jest + .fn() + .mockResolvedValue([ + join(workspaceRoot, "dir1/query1.ql"), + join(workspaceRoot, "dir2/query2.ql"), + join(workspaceRoot, "query3.ql"), + ]), + }); + + const discovery = new QueryDiscovery(createMockApp({}), cli); + const results: QueryDiscoveryResults = await ( + discovery as any + ).discover(); + + expect(results.queries[0].children.length).toEqual(3); + expect(results.queries[0].children[0].name).toEqual("dir1"); + expect(results.queries[0].children[0].children.length).toEqual(1); + expect(results.queries[0].children[0].children[0].name).toEqual( + "query1.ql", + ); + expect(results.queries[0].children[1].name).toEqual("dir2"); + expect(results.queries[0].children[1].children.length).toEqual(1); + expect(results.queries[0].children[1].children[0].name).toEqual( + "query2.ql", + ); + expect(results.queries[0].children[2].name).toEqual("query3.ql"); + }); + + it("should collapse directories containing only a single element", async () => { + const workspaceRoot = workspace.workspaceFolders![0].uri.fsPath; + const cli = mockedObject({ + resolveQueries: jest + .fn() + .mockResolvedValue([ + join(workspaceRoot, "dir1/query1.ql"), + join(workspaceRoot, "dir1/dir2/dir3/dir3/query2.ql"), + ]), + }); + + const discovery = new QueryDiscovery(createMockApp({}), cli); + const results: QueryDiscoveryResults = await ( + discovery as any + ).discover(); + + expect(results.queries[0].children.length).toEqual(1); + expect(results.queries[0].children[0].name).toEqual("dir1"); + expect(results.queries[0].children[0].children.length).toEqual(2); + expect(results.queries[0].children[0].children[0].name).toEqual( + "dir2 / dir3 / dir3", + ); + expect( + results.queries[0].children[0].children[0].children.length, + ).toEqual(1); + expect( + results.queries[0].children[0].children[0].children[0].name, + ).toEqual("query2.ql"); + expect(results.queries[0].children[0].children[1].name).toEqual( + "query1.ql", + ); + }); + + it("calls resolveQueries once for each workspace", async () => { + const workspaceRoots = ["/workspace1", "/workspace2", "/workspace3"]; + jest.spyOn(workspace, "workspaceFolders", "get").mockReturnValueOnce( + workspaceRoots.map((root, index) => ({ + uri: Uri.file(root), + name: basename(root), + index, + })), + ); + + const resolveQueries = jest.fn().mockImplementation((queryDir) => { + const workspaceIndex = workspaceRoots.indexOf(queryDir); + if (workspaceIndex === -1) { + throw new Error("Unexpected workspace"); + } + return Promise.resolve([ + join(queryDir, `query${workspaceIndex + 1}.ql`), + ]); + }); + const cli = mockedObject({ + resolveQueries, + }); + + const discovery = new QueryDiscovery(createMockApp({}), cli); + const results: QueryDiscoveryResults = await ( + discovery as any + ).discover(); + + expect(results.queries.length).toEqual(3); + expect(results.queries[0].children[0].name).toEqual("query1.ql"); + expect(results.queries[1].children[0].name).toEqual("query2.ql"); + expect(results.queries[2].children[0].name).toEqual("query3.ql"); + + expect(resolveQueries).toHaveBeenCalledTimes(3); + }); + }); + + describe("onDidChangeQueries", () => { + it("should fire onDidChangeQueries when a watcher fires", async () => { + const onWatcherDidChangeEvent = new EventEmitter(); + const watcher: FileSystemWatcher = { + ignoreCreateEvents: false, + ignoreChangeEvents: false, + ignoreDeleteEvents: false, + onDidCreate: onWatcherDidChangeEvent.event, + onDidChange: onWatcherDidChangeEvent.event, + onDidDelete: onWatcherDidChangeEvent.event, + dispose: () => undefined, + }; + const createFileSystemWatcherSpy = jest.spyOn( + workspace, + "createFileSystemWatcher", + ); + createFileSystemWatcherSpy.mockReturnValue(watcher); + + const workspaceRoot = workspace.workspaceFolders![0].uri.fsPath; + const cli = mockedObject({ + resolveQueries: jest + .fn() + .mockResolvedValue([join(workspaceRoot, "query1.ql")]), + }); + + const discovery = new QueryDiscovery( + createMockApp({ + createEventEmitter: () => new EventEmitter(), + }), + cli, + ); + + const onDidChangeQueriesSpy = jest.fn(); + discovery.onDidChangeQueries(onDidChangeQueriesSpy); + + const results = await (discovery as any).discover(); + (discovery as any).update(results); + + expect(createFileSystemWatcherSpy).toHaveBeenCalledTimes(2); + expect(onDidChangeQueriesSpy).toHaveBeenCalledTimes(1); + + onWatcherDidChangeEvent.fire(workspace.workspaceFolders![0].uri); + + // Wait for refresh to finish + await sleep(100); + + expect(onDidChangeQueriesSpy).toHaveBeenCalledTimes(2); + }); + }); + + describe("onDidChangeWorkspaceFolders", () => { + it("should refresh when workspace folders change", async () => { + const onDidChangeWorkspaceFoldersEvent = + new EventEmitter(); + + const discovery = new QueryDiscovery( + createMockApp({ + createEventEmitter: () => new EventEmitter(), + onDidChangeWorkspaceFolders: onDidChangeWorkspaceFoldersEvent.event, + }), + mockedObject({ + resolveQueries: jest.fn().mockResolvedValue([]), + }), + ); + + const onDidChangeQueriesSpy = jest.fn(); + discovery.onDidChangeQueries(onDidChangeQueriesSpy); + + const results = await (discovery as any).discover(); + (discovery as any).update(results); + + expect(onDidChangeQueriesSpy).toHaveBeenCalledTimes(1); + + onDidChangeWorkspaceFoldersEvent.fire({ added: [], removed: [] }); + + // Wait for refresh to finish + await sleep(100); + + expect(onDidChangeQueriesSpy).toHaveBeenCalledTimes(2); + }); + }); +}); From 8be2dd805b179345730e5b361783fc2e4ee09a23 Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 25 May 2023 14:44:05 +0100 Subject: [PATCH 3/4] Use path.sep --- .../queries-panel/query-discovery.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts index 0496f8712..de1c79690 100644 --- a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts @@ -12,7 +12,7 @@ import { } from "../../../../src/queries-panel/query-discovery"; import { createMockApp } from "../../../__mocks__/appMock"; import { mockedObject } from "../../utils/mocking.helpers"; -import { basename, join } from "path"; +import { basename, join, sep } from "path"; import { sleep } from "../../../../src/pure/time"; describe("QueryDiscovery", () => { @@ -101,7 +101,11 @@ describe("QueryDiscovery", () => { }); it("calls resolveQueries once for each workspace", async () => { - const workspaceRoots = ["/workspace1", "/workspace2", "/workspace3"]; + const workspaceRoots = [ + `${sep}workspace1`, + `${sep}workspace2`, + `${sep}workspace3`, + ]; jest.spyOn(workspace, "workspaceFolders", "get").mockReturnValueOnce( workspaceRoots.map((root, index) => ({ uri: Uri.file(root), From 472b1769c5491fd91fbe17080242b1da8bc5bd32 Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 25 May 2023 15:38:49 +0100 Subject: [PATCH 4/4] Update extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- .../minimal-workspace/queries-panel/query-discovery.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts index de1c79690..76052b5bd 100644 --- a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts @@ -100,7 +100,7 @@ describe("QueryDiscovery", () => { ); }); - it("calls resolveQueries once for each workspace", async () => { + it("calls resolveQueries once for each workspace folder", async () => { const workspaceRoots = [ `${sep}workspace1`, `${sep}workspace2`,