Merge pull request #2191 from github/robertbrignull/variant_analysis_commands

Convert all variant analysis commands to typed commands

extensions/ql-vscode/src/common/commands.ts

@@ -3,6 +3,12 @@ import type { Uri, Range } from "vscode";
 import type { DbTreeViewItem } from "../databases/ui/db-tree-view-item";
 import type { DatabaseItem } from "../local-databases";
 import type { QueryHistoryInfo } from "../query-history/query-history-info";
+import type { RepositoriesFilterSortStateWithIds } from "../pure/variant-analysis-filter-sort";
+import type {
+  VariantAnalysis,
+  VariantAnalysisScannedRepository,
+  VariantAnalysisScannedRepositoryResult,
+} from "../variant-analysis/shared/variant-analysis";
 
 // A command function matching the signature that VS Code calls when
 // a command on a selection is invoked.
@@ -123,9 +129,27 @@ export type LocalDatabasesCommands = {
 
 // Commands tied to variant analysis
 export type VariantAnalysisCommands = {
+  "codeQL.autoDownloadVariantAnalysisResult": (
+    scannedRepo: VariantAnalysisScannedRepository,
+    variantAnalysisSummary: VariantAnalysis,
+  ) => Promise<void>;
+  "codeQL.copyVariantAnalysisRepoList": (
+    variantAnalysisId: number,
+    filterSort?: RepositoriesFilterSortStateWithIds,
+  ) => Promise<void>;
+  "codeQL.loadVariantAnalysisRepoResults": (
+    variantAnalysisId: number,
+    repositoryFullName: string,
+  ) => Promise<VariantAnalysisScannedRepositoryResult>;
+  "codeQL.monitorVariantAnalysis": (
+    variantAnalysis: VariantAnalysis,
+  ) => Promise<void>;
   "codeQL.openVariantAnalysisLogs": (
     variantAnalysisId: number,
   ) => Promise<void>;
+  "codeQL.openVariantAnalysisView": (
+    variantAnalysisId: number,
+  ) => Promise<void>;
   "codeQL.runVariantAnalysis": (uri?: Uri) => Promise<void>;
   "codeQL.runVariantAnalysisContextEditor": (uri?: Uri) => Promise<void>;
 };

extensions/ql-vscode/src/extension.ts

@@ -110,16 +110,11 @@ import { NewQueryRunner } from "./query-server/query-runner";
 import { QueryRunner } from "./queryRunner";
 import { VariantAnalysisView } from "./variant-analysis/variant-analysis-view";
 import { VariantAnalysisViewSerializer } from "./variant-analysis/variant-analysis-view-serializer";
-import {
-  VariantAnalysis,
-  VariantAnalysisScannedRepository,
-} from "./variant-analysis/shared/variant-analysis";
 import { VariantAnalysisManager } from "./variant-analysis/variant-analysis-manager";
 import { createVariantAnalysisContentProvider } from "./variant-analysis/variant-analysis-content-provider";
 import { VSCodeMockGitHubApiServer } from "./mocks/vscode-mock-gh-api-server";
 import { VariantAnalysisResultsManager } from "./variant-analysis/variant-analysis-results-manager";
 import { ExtensionApp } from "./common/vscode/vscode-app";
-import { RepositoriesFilterSortStateWithIds } from "./pure/variant-analysis-filter-sort";
 import { DbModule } from "./databases/db-module";
 import { redactableError } from "./pure/errors";
 import { QueryHistoryDirs } from "./query-history/query-history-dirs";
@@ -847,72 +842,6 @@ async function activateWithInstalledDistribution(
     );
   }
 
-  ctx.subscriptions.push(
-    commandRunner(
-      "codeQL.copyVariantAnalysisRepoList",
-      async (
-        variantAnalysisId: number,
-        filterSort?: RepositoriesFilterSortStateWithIds,
-      ) => {
-        await variantAnalysisManager.copyRepoListToClipboard(
-          variantAnalysisId,
-          filterSort,
-        );
-      },
-    ),
-  );
-
-  ctx.subscriptions.push(
-    commandRunner(
-      "codeQL.monitorVariantAnalysis",
-      async (variantAnalysis: VariantAnalysis, token: CancellationToken) => {
-        await variantAnalysisManager.monitorVariantAnalysis(
-          variantAnalysis,
-          token,
-        );
-      },
-    ),
-  );
-
-  ctx.subscriptions.push(
-    commandRunner(
-      "codeQL.autoDownloadVariantAnalysisResult",
-      async (
-        scannedRepo: VariantAnalysisScannedRepository,
-        variantAnalysisSummary: VariantAnalysis,
-        token: CancellationToken,
-      ) => {
-        await variantAnalysisManager.enqueueDownload(
-          scannedRepo,
-          variantAnalysisSummary,
-          token,
-        );
-      },
-    ),
-  );
-
-  ctx.subscriptions.push(
-    commandRunner(
-      "codeQL.loadVariantAnalysisRepoResults",
-      async (variantAnalysisId: number, repositoryFullName: string) => {
-        await variantAnalysisManager.loadResults(
-          variantAnalysisId,
-          repositoryFullName,
-        );
-      },
-    ),
-  );
-
-  // The "openVariantAnalysisView" command is internal-only.
-  ctx.subscriptions.push(
-    commandRunner(
-      "codeQL.openVariantAnalysisView",
-      async (variantAnalysisId: number) => {
-        await variantAnalysisManager.showView(variantAnalysisId);
-      },
-    ),
-  );
-
   ctx.subscriptions.push(
     commandRunner("codeQL.openReferencedFile", async (selectedQuery: Uri) => {
       await openReferencedFile(qs, cliServer, selectedQuery);

extensions/ql-vscode/src/variant-analysis/variant-analysis-manager.ts

@@ -131,14 +131,19 @@ export class VariantAnalysisManager
 
   getCommands(): VariantAnalysisCommands {
     return {
-      "codeQL.openVariantAnalysisLogs": async (variantAnalysisId: number) => {
+      "codeQL.autoDownloadVariantAnalysisResult":
-        await this.openVariantAnalysisLogs(variantAnalysisId);
+        this.enqueueDownload.bind(this),
-      },
+      "codeQL.copyVariantAnalysisRepoList":
-      "codeQL.runVariantAnalysis": async (uri?: Uri) =>
+        this.copyRepoListToClipboard.bind(this),
-        this.runVariantAnalysisFromCommand(uri),
+      "codeQL.loadVariantAnalysisRepoResults": this.loadResults.bind(this),
+      "codeQL.monitorVariantAnalysis": this.monitorVariantAnalysis.bind(this),
+      "codeQL.openVariantAnalysisLogs": this.openVariantAnalysisLogs.bind(this),
+      "codeQL.openVariantAnalysisView": this.showView.bind(this),
+      "codeQL.runVariantAnalysis":
+        this.runVariantAnalysisFromCommand.bind(this),
       // Since we are tracking extension usage through commands, this command mirrors the "codeQL.runVariantAnalysis" command
-      "codeQL.runVariantAnalysisContextEditor": async (uri?: Uri) =>
+      "codeQL.runVariantAnalysisContextEditor":
-        this.runVariantAnalysisFromCommand(uri),
+        this.runVariantAnalysisFromCommand.bind(this),
     };
   }
 
@@ -496,19 +501,16 @@ export class VariantAnalysisManager
 
   public async monitorVariantAnalysis(
     variantAnalysis: VariantAnalysis,
-    cancellationToken: CancellationToken,
   ): Promise<void> {
     await this.variantAnalysisMonitor.monitorVariantAnalysis(
       variantAnalysis,
       this.app.credentials,
-      cancellationToken,
     );
   }
 
   public async autoDownloadVariantAnalysisResult(
     scannedRepo: VariantAnalysisScannedRepository,
     variantAnalysis: VariantAnalysis,
-    cancellationToken: CancellationToken,
   ): Promise<void> {
     if (
       this.repoStates.get(variantAnalysis.id)?.[scannedRepo.repository.id]
@@ -525,13 +527,6 @@ export class VariantAnalysisManager
 
     await this.onRepoStateUpdated(variantAnalysis.id, repoState);
 
-    if (cancellationToken && cancellationToken.isCancellationRequested) {
-      repoState.downloadStatus =
-        VariantAnalysisScannedRepositoryDownloadStatus.Failed;
-      await this.onRepoStateUpdated(variantAnalysis.id, repoState);
-      return;
-    }
-
     let repoTask: VariantAnalysisRepositoryTask;
     try {
       const repoTaskResponse = await getVariantAnalysisRepo(
@@ -606,14 +601,9 @@ export class VariantAnalysisManager
   public async enqueueDownload(
     scannedRepo: VariantAnalysisScannedRepository,
     variantAnalysis: VariantAnalysis,
-    token: CancellationToken,
   ): Promise<void> {
     await this.queue.add(() =>
-      this.autoDownloadVariantAnalysisResult(
+      this.autoDownloadVariantAnalysisResult(scannedRepo, variantAnalysis),
-        scannedRepo,
-        variantAnalysis,
-        token,
-      ),
     );
   }
 

extensions/ql-vscode/src/variant-analysis/variant-analysis-monitor.ts

@@ -1,4 +1,4 @@
-import { CancellationToken, commands, EventEmitter } from "vscode";
+import { commands, EventEmitter } from "vscode";
 import { getVariantAnalysis } from "./gh-api/gh-api-client";
 
 import {
@@ -37,7 +37,6 @@ export class VariantAnalysisMonitor extends DisposableObject {
   public async monitorVariantAnalysis(
     variantAnalysis: VariantAnalysis,
     credentials: Credentials,
-    cancellationToken: CancellationToken,
   ): Promise<void> {
     let attemptCount = 0;
     const scannedReposDownloaded: number[] = [];
@@ -45,10 +44,6 @@ export class VariantAnalysisMonitor extends DisposableObject {
     while (attemptCount <= VariantAnalysisMonitor.maxAttemptCount) {
       await sleep(VariantAnalysisMonitor.sleepTime);
 
-      if (cancellationToken && cancellationToken.isCancellationRequested) {
-        return;
-      }
-
       if (await this.shouldCancelMonitor(variantAnalysis.id)) {
         return;
       }

extensions/ql-vscode/test/vscode-tests/activated-extension/variant-analysis/variant-analysis-manager.test.ts

@@ -1,5 +1,4 @@
 import {
-  CancellationTokenSource,
   commands,
   env,
   extensions,
@@ -54,7 +53,6 @@ jest.setTimeout(3 * 60 * 1000);
 
 describe("Variant Analysis Manager", () => {
   let app: App;
-  let cancellationTokenSource: CancellationTokenSource;
   let variantAnalysisManager: VariantAnalysisManager;
   let variantAnalysisResultsManager: VariantAnalysisResultsManager;
   let variantAnalysis: VariantAnalysis;
@@ -63,8 +61,6 @@ describe("Variant Analysis Manager", () => {
   beforeEach(async () => {
     jest.spyOn(extLogger, "log").mockResolvedValue(undefined);
 
-    cancellationTokenSource = new CancellationTokenSource();
-
     scannedRepos = createMockScannedRepos();
     variantAnalysis = createMockVariantAnalysis({
       status: VariantAnalysisStatus.InProgress,
@@ -203,7 +199,6 @@ describe("Variant Analysis Manager", () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         expect(getVariantAnalysisRepoResultStub).not.toHaveBeenCalled();
@@ -228,23 +223,10 @@ describe("Variant Analysis Manager", () => {
         getVariantAnalysisRepoResultStub.mockResolvedValue(response);
       });
 
-      it("should return early if variant analysis is cancelled", async () => {
-        cancellationTokenSource.cancel();
-
-        await variantAnalysisManager.autoDownloadVariantAnalysisResult(
-          scannedRepos[0],
-          variantAnalysis,
-          cancellationTokenSource.token,
-        );
-
-        expect(getVariantAnalysisRepoStub).not.toHaveBeenCalled();
-      });
-
       it("should fetch a repo task", async () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         expect(getVariantAnalysisRepoStub).toHaveBeenCalled();
@@ -254,7 +236,6 @@ describe("Variant Analysis Manager", () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         expect(getVariantAnalysisRepoResultStub).toHaveBeenCalled();
@@ -265,7 +246,6 @@ describe("Variant Analysis Manager", () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         getVariantAnalysisRepoStub.mockClear();
@@ -273,7 +253,6 @@ describe("Variant Analysis Manager", () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         expect(getVariantAnalysisRepoStub).not.toHaveBeenCalled();
@@ -283,7 +262,6 @@ describe("Variant Analysis Manager", () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         await expect(fs.readJson(repoStatesPath)).resolves.toEqual({
@@ -304,7 +282,6 @@ describe("Variant Analysis Manager", () => {
           variantAnalysisManager.autoDownloadVariantAnalysisResult(
             scannedRepos[0],
             variantAnalysis,
-            cancellationTokenSource.token,
           ),
         ).rejects.toThrow();
 
@@ -320,7 +297,6 @@ describe("Variant Analysis Manager", () => {
           variantAnalysisManager.autoDownloadVariantAnalysisResult(
             scannedRepos[0],
             variantAnalysis,
-            cancellationTokenSource.token,
           ),
         ).rejects.toThrow();
 
@@ -329,7 +305,6 @@ describe("Variant Analysis Manager", () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[1],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         await expect(fs.readJson(repoStatesPath)).resolves.toEqual({
@@ -355,7 +330,6 @@ describe("Variant Analysis Manager", () => {
           variantAnalysisManager.autoDownloadVariantAnalysisResult(
             scannedRepos[0],
             variantAnalysis,
-            cancellationTokenSource.token,
           ),
         ).rejects.toThrow();
 
@@ -364,7 +338,6 @@ describe("Variant Analysis Manager", () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[1],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         await expect(fs.readJson(repoStatesPath)).resolves.toEqual({
@@ -400,7 +373,6 @@ describe("Variant Analysis Manager", () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
           variantAnalysis,
-          cancellationTokenSource.token,
         );
 
         await expect(fs.readJson(repoStatesPath)).resolves.toEqual({
@@ -439,17 +411,14 @@ describe("Variant Analysis Manager", () => {
       await variantAnalysisManager.enqueueDownload(
         scannedRepos[0],
         variantAnalysis,
-        cancellationTokenSource.token,
       );
       await variantAnalysisManager.enqueueDownload(
         scannedRepos[1],
         variantAnalysis,
-        cancellationTokenSource.token,
       );
       await variantAnalysisManager.enqueueDownload(
         scannedRepos[2],
         variantAnalysis,
-        cancellationTokenSource.token,
       );
 
       expect(variantAnalysisManager.downloadsQueueSize()).toBe(0);

extensions/ql-vscode/test/vscode-tests/activated-extension/variant-analysis/variant-analysis-monitor.test.ts

@@ -1,4 +1,4 @@
-import { CancellationTokenSource, commands, extensions } from "vscode";
+import { commands, extensions } from "vscode";
 import { CodeQLExtensionInterface } from "../../../../src/extension";
 
 import * as ghApiClient from "../../../../src/variant-analysis/gh-api/gh-api-client";
@@ -33,7 +33,6 @@ describe("Variant Analysis Monitor", () => {
   let mockGetVariantAnalysis: jest.SpiedFunction<
     typeof ghApiClient.getVariantAnalysis
   >;
-  let cancellationTokenSource: CancellationTokenSource;
   let variantAnalysisMonitor: VariantAnalysisMonitor;
   let shouldCancelMonitor: jest.Mock<Promise<boolean>, [number]>;
   let variantAnalysis: VariantAnalysis;
@@ -45,8 +44,6 @@ describe("Variant Analysis Monitor", () => {
   const onVariantAnalysisChangeSpy = jest.fn();
 
   beforeEach(async () => {
-    cancellationTokenSource = new CancellationTokenSource();
-
     variantAnalysis = createMockVariantAnalysis({});
 
     shouldCancelMonitor = jest.fn();
@@ -71,25 +68,12 @@ describe("Variant Analysis Monitor", () => {
     limitNumberOfAttemptsToMonitor();
   });
 
-  it("should return early if variant analysis is cancelled", async () => {
-    cancellationTokenSource.cancel();
-
-    await variantAnalysisMonitor.monitorVariantAnalysis(
-      variantAnalysis,
-      testCredentialsWithStub(),
-      cancellationTokenSource.token,
-    );
-
-    expect(onVariantAnalysisChangeSpy).not.toHaveBeenCalled();
-  });
-
   it("should return early if variant analysis should be cancelled", async () => {
     shouldCancelMonitor.mockResolvedValue(true);
 
     await variantAnalysisMonitor.monitorVariantAnalysis(
       variantAnalysis,
       testCredentialsWithStub(),
-      cancellationTokenSource.token,
     );
 
     expect(onVariantAnalysisChangeSpy).not.toHaveBeenCalled();
@@ -107,7 +91,6 @@ describe("Variant Analysis Monitor", () => {
       await variantAnalysisMonitor.monitorVariantAnalysis(
         variantAnalysis,
         testCredentialsWithStub(),
-        cancellationTokenSource.token,
       );
 
       expect(mockGetVariantAnalysis).toHaveBeenCalledTimes(1);
@@ -157,7 +140,6 @@ describe("Variant Analysis Monitor", () => {
         await variantAnalysisMonitor.monitorVariantAnalysis(
           variantAnalysis,
           testCredentialsWithStub(),
-          cancellationTokenSource.token,
         );
 
         expect(commandSpy).toBeCalledTimes(succeededRepos.length);
@@ -176,7 +158,6 @@ describe("Variant Analysis Monitor", () => {
         await variantAnalysisMonitor.monitorVariantAnalysis(
           variantAnalysis,
           testCredentialsWithStub(),
-          cancellationTokenSource.token,
         );
 
         expect(mockGetDownloadResult).toBeCalledTimes(succeededRepos.length);
@@ -186,7 +167,6 @@ describe("Variant Analysis Monitor", () => {
             index + 1,
             processScannedRepository(succeededRepo),
             processUpdatedVariantAnalysis(variantAnalysis, mockApiResponse),
-            undefined,
           );
         });
       });
@@ -209,7 +189,6 @@ describe("Variant Analysis Monitor", () => {
         await variantAnalysisMonitor.monitorVariantAnalysis(
           variantAnalysis,
           testCredentialsWithStub(),
-          cancellationTokenSource.token,
         );
 
         expect(commandSpy).not.toHaveBeenCalled();
@@ -219,7 +198,6 @@ describe("Variant Analysis Monitor", () => {
         await variantAnalysisMonitor.monitorVariantAnalysis(
           variantAnalysis,
           testCredentialsWithStub(),
-          cancellationTokenSource.token,
         );
 
         expect(mockGetDownloadResult).not.toBeCalled();
@@ -278,7 +256,6 @@ describe("Variant Analysis Monitor", () => {
         await variantAnalysisMonitor.monitorVariantAnalysis(
           variantAnalysis,
           testCredentialsWithStub(),
-          cancellationTokenSource.token,
         );
 
         expect(mockGetVariantAnalysis).toBeCalledTimes(4);
@@ -297,7 +274,6 @@ describe("Variant Analysis Monitor", () => {
         await variantAnalysisMonitor.monitorVariantAnalysis(
           variantAnalysis,
           testCredentialsWithStub(),
-          cancellationTokenSource.token,
         );
 
         expect(mockGetDownloadResult).not.toBeCalled();