Merge pull request #1690 from github/robertbrignull/handle_states_monitoring

Make the monitoring command slightly simpler and handle being called on any variant analysis

Author: robertbrignull

extensions/ql-vscode/src/remote-queries/shared/variant-analysis-monitor-result.ts

@@ -1,16 +1,11 @@
 import { VariantAnalysis } from './variant-analysis';
 
 export type VariantAnalysisMonitorStatus =
-  | 'InProgress'
-  | 'CompletedSuccessfully'
-  | 'CompletedUnsuccessfully'
-  | 'Failed'
-  | 'Cancelled'
-  | 'TimedOut';
+  | 'Completed'
+  | 'Canceled';
 
 export interface VariantAnalysisMonitorResult {
   status: VariantAnalysisMonitorStatus;
-  error?: string;
   scannedReposDownloaded?: number[],
   variantAnalysis?: VariantAnalysis
 }

extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts

@@ -47,6 +47,13 @@ export enum VariantAnalysisStatus {
   Canceled = 'canceled',
 }
 
+export function isFinalVariantAnalysisStatus(status: VariantAnalysisStatus): boolean {
+  return [
+    // All states that indicates the analysis has completed and cannot change status anymore.
+    VariantAnalysisStatus.Succeeded, VariantAnalysisStatus.Failed, VariantAnalysisStatus.Canceled,
+  ].includes(status);
+}
+
 export enum VariantAnalysisFailureReason {
   NoReposQueried = 'noReposQueried',
   InternalError = 'internalError',

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

@@ -2,13 +2,13 @@ import { ExtensionContext, CancellationToken, commands, EventEmitter } from 'vsc
 import { Credentials } from '../authentication';
 import * as ghApiClient from './gh-api/gh-api-client';
 
-import { VariantAnalysis, VariantAnalysisStatus } from './shared/variant-analysis';
+import { isFinalVariantAnalysisStatus, VariantAnalysis } from './shared/variant-analysis';
 import {
   VariantAnalysis as VariantAnalysisApiResponse,
   VariantAnalysisScannedRepository
 } from './gh-api/variant-analysis';
 import { VariantAnalysisMonitorResult } from './shared/variant-analysis-monitor-result';
-import { processFailureReason, processUpdatedVariantAnalysis } from './variant-analysis-processor';
+import { processUpdatedVariantAnalysis } from './variant-analysis-processor';
 import { DisposableObject } from '../pure/disposable-object';
 
 export class VariantAnalysisMonitor extends DisposableObject {
@@ -36,53 +36,37 @@ export class VariantAnalysisMonitor extends DisposableObject {
       throw Error('Error authenticating with GitHub');
     }
 
-    let variantAnalysisSummary: VariantAnalysisApiResponse;
     let attemptCount = 0;
     const scannedReposDownloaded: number[] = [];
 
-    this._onVariantAnalysisChange.fire(variantAnalysis);
-
     while (attemptCount <= VariantAnalysisMonitor.maxAttemptCount) {
       await this.sleep(VariantAnalysisMonitor.sleepTime);
 
       if (cancellationToken && cancellationToken.isCancellationRequested) {
-        return { status: 'Cancelled', error: 'Variant Analysis was canceled.' };
+        return { status: 'Canceled' };
       }
 
-      variantAnalysisSummary = await ghApiClient.getVariantAnalysis(
+      const variantAnalysisSummary = await ghApiClient.getVariantAnalysis(
         credentials,
         variantAnalysis.controllerRepo.id,
         variantAnalysis.id
       );
 
-      if (variantAnalysisSummary.failure_reason) {
-        variantAnalysis.status = VariantAnalysisStatus.Failed;
-        variantAnalysis.failureReason = processFailureReason(variantAnalysisSummary.failure_reason);
-
-        this._onVariantAnalysisChange.fire(variantAnalysis);
-
-        return {
-          status: 'Failed',
-          error: `Variant Analysis has failed: ${variantAnalysisSummary.failure_reason}`,
-          variantAnalysis: variantAnalysis
-        };
-      }
-
       variantAnalysis = processUpdatedVariantAnalysis(variantAnalysis, variantAnalysisSummary);
 
       this._onVariantAnalysisChange.fire(variantAnalysis);
 
       const downloadedRepos = this.downloadVariantAnalysisResults(variantAnalysisSummary, scannedReposDownloaded);
       scannedReposDownloaded.push(...downloadedRepos);
 
-      if (variantAnalysisSummary.status === 'completed') {
+      if (isFinalVariantAnalysisStatus(variantAnalysis.status) || variantAnalysis.failureReason) {
         break;
       }
 
       attemptCount++;
     }
 
-    return { status: 'CompletedSuccessfully', scannedReposDownloaded: scannedReposDownloaded };
+    return { status: 'Completed', scannedReposDownloaded, variantAnalysis };
   }
 
   private scheduleForDownload(

extensions/ql-vscode/src/remote-queries/variant-analysis-processor.ts

@@ -62,7 +62,7 @@ export function processUpdatedVariantAnalysis(
     executionStartTime: previousVariantAnalysis.executionStartTime,
     createdAt: response.created_at,
     updatedAt: response.updated_at,
-    status: processApiStatus(response.status),
+    status: processApiStatus(response.status, response.failure_reason),
     completedAt: response.completed_at,
     actionsWorkflowRunId: response.actions_workflow_run_id,
     scannedRepos: scannedRepos,
@@ -158,12 +158,13 @@ function processApiRepoStatus(analysisStatus: ApiVariantAnalysisRepoStatus): Var
   }
 }
 
-function processApiStatus(status: ApiVariantAnalysisStatus): VariantAnalysisStatus {
-  switch (status) {
-    case 'in_progress':
-      return VariantAnalysisStatus.InProgress;
-    case 'completed':
-      return VariantAnalysisStatus.Succeeded;
+function processApiStatus(status: ApiVariantAnalysisStatus, failureReason: string | undefined): VariantAnalysisStatus {
+  if (status === 'in_progress') {
+    return VariantAnalysisStatus.InProgress;
+  } else if (failureReason !== undefined) {
+    return VariantAnalysisStatus.Failed;
+  } else {
+    return VariantAnalysisStatus.Succeeded;
   }
 }
 

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

@@ -83,23 +83,22 @@ describe('Variant Analysis Monitor', async function() {
 
       const result = await variantAnalysisMonitor.monitorVariantAnalysis(variantAnalysis, cancellationTokenSource.token);
 
-      expect(result).to.eql({ status: 'Cancelled', error: 'Variant Analysis was canceled.' });
+      expect(result).to.eql({ status: 'Canceled' });
     });
 
     describe('when the variant analysis fails', async () => {
       let mockFailedApiResponse: VariantAnalysisApiResponse;
 
       beforeEach(async function() {
-        mockFailedApiResponse = createFailedMockApiResponse('in_progress');
+        mockFailedApiResponse = createFailedMockApiResponse();
         mockGetVariantAnalysis = sandbox.stub(ghApiClient, 'getVariantAnalysis').resolves(mockFailedApiResponse);
       });
 
       it('should mark as failed locally and stop monitoring', async () => {
         const result = await variantAnalysisMonitor.monitorVariantAnalysis(variantAnalysis, cancellationTokenSource.token);
 
         expect(mockGetVariantAnalysis.calledOnce).to.be.true;
-        expect(result.status).to.eql('Failed');
-        expect(result.error).to.eql(`Variant Analysis has failed: ${mockFailedApiResponse.failure_reason}`);
+        expect(result.status).to.eql('Completed');
         expect(result.variantAnalysis?.status).to.equal(VariantAnalysisStatus.Failed);
         expect(result.variantAnalysis?.failureReason).to.equal(processFailureReason(mockFailedApiResponse.failure_reason as VariantAnalysisFailureReason));
       });
@@ -130,7 +129,7 @@ describe('Variant Analysis Monitor', async function() {
         it('should succeed and return a list of scanned repo ids', async () => {
           const result = await variantAnalysisMonitor.monitorVariantAnalysis(variantAnalysis, cancellationTokenSource.token);
 
-          expect(result.status).to.equal('CompletedSuccessfully');
+          expect(result.status).to.equal('Completed');
           expect(result.scannedReposDownloaded).to.eql(succeededRepos.map(r => r.repository.id));
         });
 
@@ -173,7 +172,7 @@ describe('Variant Analysis Monitor', async function() {
         it('should succeed and return an empty list of scanned repo ids', async () => {
           const result = await variantAnalysisMonitor.monitorVariantAnalysis(variantAnalysis, cancellationTokenSource.token);
 
-          expect(result.status).to.equal('CompletedSuccessfully');
+          expect(result.status).to.equal('Completed');
           expect(result.scannedReposDownloaded).to.eql([]);
         });
 
@@ -194,7 +193,7 @@ describe('Variant Analysis Monitor', async function() {
         it('should succeed and return an empty list of scanned repo ids', async () => {
           const result = await variantAnalysisMonitor.monitorVariantAnalysis(variantAnalysis, cancellationTokenSource.token);
 
-          expect(result.status).to.equal('CompletedSuccessfully');
+          expect(result.status).to.equal('Completed');
           expect(result.scannedReposDownloaded).to.eql([]);
         });
 

extensions/ql-vscode/src/vscode-tests/factories/remote-queries/gh-api/variant-analysis-api-response.ts

@@ -39,13 +39,10 @@ export function createMockApiResponse(
 }
 
 export function createFailedMockApiResponse(
-  status: VariantAnalysisStatus = 'in_progress',
   scannedRepos: VariantAnalysisScannedRepository[] = createMockScannedRepos(),
   skippedRepos: VariantAnalysisSkippedRepositories = createMockSkippedRepos(),
 ): VariantAnalysisApiResponse {
-  const variantAnalysis = createMockApiResponse(status, scannedRepos, skippedRepos);
-  variantAnalysis.status = status;
+  const variantAnalysis = createMockApiResponse('completed', scannedRepos, skippedRepos);
   variantAnalysis.failure_reason = 'internal_error';
-
   return variantAnalysis;
 }