Move isVariantAnalysisComplete implementation out of variant analysis manager

Author: robertbrignull

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

@@ -130,6 +130,26 @@ export interface VariantAnalysisSubmission {
   }
 }
 
+export async function isVariantAnalysisComplete(
+  variantAnalysis: VariantAnalysis,
+  artifactDownloaded: (repo: VariantAnalysisScannedRepository) => Promise<boolean>
+): Promise<boolean> {
+  // It's only acceptable to have no scanned repos if the variant analysis is not in a final state.
+  // Otherwise it means the analysis hit some kind of internal error or there were no repos to scan.
+  if (variantAnalysis.scannedRepos === undefined || variantAnalysis.scannedRepos.length === 0) {
+    return variantAnalysis.status !== VariantAnalysisStatus.InProgress;
+  }
+
+  return (await Promise.all(variantAnalysis.scannedRepos.map(repo => isVariantAnalysisRepoComplete(repo, artifactDownloaded)))).every(x => x);
+}
+
+async function isVariantAnalysisRepoComplete(
+  repo: VariantAnalysisScannedRepository,
+  artifactDownloaded: (repo: VariantAnalysisScannedRepository) => Promise<boolean>
+): Promise<boolean> {
+  return hasRepoScanCompleted(repo) && (!repoHasDownloadableArtifact(repo) || await artifactDownloaded(repo));
+}
+
 /**
  * @param status
  * @returns whether the status is in a completed state, i.e. it cannot normally change state anymore

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

@@ -11,14 +11,12 @@ import {
   VariantAnalysisScannedRepository as ApiVariantAnalysisScannedRepository
 } from './gh-api/variant-analysis';
 import {
-  hasRepoScanCompleted,
-  repoHasDownloadableArtifact,
+  isVariantAnalysisComplete,
   VariantAnalysis, VariantAnalysisQueryLanguage,
   VariantAnalysisScannedRepository,
   VariantAnalysisScannedRepositoryDownloadStatus,
   VariantAnalysisScannedRepositoryResult,
-  VariantAnalysisScannedRepositoryState,
-  VariantAnalysisStatus
+  VariantAnalysisScannedRepositoryState
 } from './shared/variant-analysis';
 import { getErrorMessage } from '../pure/helpers-pure';
 import { VariantAnalysisView } from './variant-analysis-view';
@@ -68,31 +66,15 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA
       this.variantAnalyses.set(variantAnalysis.id, variantAnalysis);
       await this.getView(variantAnalysis.id)?.updateView(variantAnalysis);
 
-      if (!await this.isVariantAnalysisComplete(variantAnalysis)) {
+      if (!await isVariantAnalysisComplete(variantAnalysis, this.makeResultDownloadChecker(variantAnalysis))) {
         await commands.executeCommand('codeQL.monitorVariantAnalysis', variantAnalysis);
       }
     }
   }
 
-  public async isVariantAnalysisComplete(variantAnalysis: VariantAnalysis): Promise<boolean> {
-    // It's only acceptable to have no scanned repos if the variant analysis is not in a final state.
-    // Otherwise it means the analysis hit some kind of internal error or there were no repos to scan.
-    if (variantAnalysis.scannedRepos === undefined || variantAnalysis.scannedRepos.length === 0) {
-      return variantAnalysis.status !== VariantAnalysisStatus.InProgress;
-    }
-
-    return (await Promise.all(variantAnalysis.scannedRepos.map(repo => this.isVariantAnalysisRepoComplete(variantAnalysis.id, repo)))).every(x => x);
-  }
-
-  private async isVariantAnalysisRepoComplete(variantAnalysisId: number, repo: VariantAnalysisScannedRepository): Promise<boolean> {
-    if (!hasRepoScanCompleted(repo)) {
-      return false;
-    } else if (!repoHasDownloadableArtifact(repo)) {
-      return true;
-    } else {
-      const storageLocation = this.getVariantAnalysisStorageLocation(variantAnalysisId);
-      return await this.variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(storageLocation, repo.repository.fullName);
-    }
+  private makeResultDownloadChecker(variantAnalysis: VariantAnalysis): (repo: VariantAnalysisScannedRepository) => Promise<boolean> {
+    const storageLocation = this.getVariantAnalysisStorageLocation(variantAnalysis.id);
+    return (repo) => this.variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(storageLocation, repo.repository.fullName);
   }
 
   public async removeVariantAnalysis(variantAnalysis: VariantAnalysis) {

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

@@ -22,7 +22,8 @@ import { CodeQLCliServer } from '../../../cli';
 import { storagePath } from '../global.helper';
 import { VariantAnalysisResultsManager } from '../../../remote-queries/variant-analysis-results-manager';
 import { createMockVariantAnalysis } from '../../factories/remote-queries/shared/variant-analysis';
-import { VariantAnalysis, VariantAnalysisStatus } from '../../../remote-queries/shared/variant-analysis';
+import { VariantAnalysis } from '../../../remote-queries/shared/variant-analysis';
+import * as VariantAnalysisModule from '../../../remote-queries/shared/variant-analysis';
 
 describe('Variant Analysis Manager', async function() {
   let sandbox: sinon.SinonSandbox;
@@ -209,7 +210,7 @@ describe('Variant Analysis Manager', async function() {
 
       describe('when the variant analysis is not complete', async () => {
         beforeEach(() => {
-          sinon.stub(variantAnalysisManager, 'isVariantAnalysisComplete').resolves(false);
+          sandbox.stub(VariantAnalysisModule, 'isVariantAnalysisComplete').resolves(false);
         });
 
         it('should not remove the variant analysis', async () => {
@@ -225,7 +226,7 @@ describe('Variant Analysis Manager', async function() {
 
       describe('when the variant analysis is complete', async () => {
         beforeEach(() => {
-          sinon.stub(variantAnalysisManager, 'isVariantAnalysisComplete').resolves(true);
+          sandbox.stub(VariantAnalysisModule, 'isVariantAnalysisComplete').resolves(true);
         });
 
         it('should not remove the variant analysis', async () => {
@@ -240,83 +241,4 @@ describe('Variant Analysis Manager', async function() {
       });
     });
   });
-
-  describe('isVariantAnalysisComplete', async () => {
-    let variantAnalysis: VariantAnalysis;
-
-    beforeEach(() => {
-      variantAnalysis = createMockVariantAnalysis();
-    });
-
-    describe('when variant analysis status is InProgress', async () => {
-      beforeEach(() => {
-        variantAnalysis.status = VariantAnalysisStatus.InProgress;
-      });
-
-      describe('when scanned repos is undefined', async () => {
-        it('should say the variant analysis is not complete', async () => {
-          variantAnalysis.scannedRepos = undefined;
-          expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(false);
-        });
-      });
-
-      describe('when scanned repos is non-empty', async () => {
-        describe('when not all results are downloaded', async () => {
-          it('should say the variant analysis is not complete', async () => {
-            sinon.stub(variantAnalysisResultsManager, 'isVariantAnalysisRepoDownloaded').resolves(false);
-            expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(false);
-          });
-        });
-
-        describe('when all results are downloaded', async () => {
-          it('should say the variant analysis is complete', async () => {
-            sinon.stub(variantAnalysisResultsManager, 'isVariantAnalysisRepoDownloaded').resolves(true);
-            expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(true);
-          });
-        });
-      });
-    });
-
-    for (const variantAnalysisStatus of [
-      VariantAnalysisStatus.Succeeded,
-      VariantAnalysisStatus.Failed,
-      VariantAnalysisStatus.Canceled
-    ]) {
-      describe(`when variant analysis status is ${variantAnalysisStatus}`, async () => {
-        beforeEach(() => {
-          variantAnalysis.status = variantAnalysisStatus;
-        });
-
-        describe('when scanned repos is undefined', async () => {
-          it('should say the variant analysis is complete', async () => {
-            variantAnalysis.scannedRepos = undefined;
-            expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(true);
-          });
-        });
-
-        describe('when scanned repos is empty', async () => {
-          it('should say the variant analysis is complete', async () => {
-            variantAnalysis.scannedRepos = [];
-            expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(true);
-          });
-        });
-
-        describe('when scanned repos is non-empty', async () => {
-          describe('when not all results are downloaded', async () => {
-            it('should say the variant analysis is not complete', async () => {
-              sinon.stub(variantAnalysisResultsManager, 'isVariantAnalysisRepoDownloaded').resolves(false);
-              expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(false);
-            });
-          });
-
-          describe('when all results are downloaded', async () => {
-            it('should say the variant analysis is complete', async () => {
-              sinon.stub(variantAnalysisResultsManager, 'isVariantAnalysisRepoDownloaded').resolves(true);
-              expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(true);
-            });
-          });
-        });
-      });
-    }
-  });
 });

extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts

@@ -1,5 +1,6 @@
 import { expect } from 'chai';
-import { parseVariantAnalysisQueryLanguage, VariantAnalysisQueryLanguage } from '../../src/remote-queries/shared/variant-analysis';
+import { VariantAnalysis, parseVariantAnalysisQueryLanguage, VariantAnalysisQueryLanguage, VariantAnalysisStatus, isVariantAnalysisComplete } from '../../src/remote-queries/shared/variant-analysis';
+import { createMockVariantAnalysis } from '../../src/vscode-tests/factories/remote-queries/shared/variant-analysis';
 
 describe('parseVariantAnalysisQueryLanguage', () => {
   it('parses a valid language', () => {
@@ -10,3 +11,79 @@ describe('parseVariantAnalysisQueryLanguage', () => {
     expect(parseVariantAnalysisQueryLanguage('rubbish')).to.not.exist;
   });
 });
+
+describe('isVariantAnalysisComplete', async () => {
+  let variantAnalysis: VariantAnalysis;
+  const uncallableArtifactDownloadChecker = () => { throw new Error('Should not be called'); };
+
+  beforeEach(() => {
+    variantAnalysis = createMockVariantAnalysis();
+  });
+
+  describe('when variant analysis status is InProgress', async () => {
+    beforeEach(() => {
+      variantAnalysis.status = VariantAnalysisStatus.InProgress;
+    });
+
+    describe('when scanned repos is undefined', async () => {
+      it('should say the variant analysis is not complete', async () => {
+        variantAnalysis.scannedRepos = undefined;
+        expect(isVariantAnalysisComplete(variantAnalysis, uncallableArtifactDownloadChecker)).to.equal(false);
+      });
+    });
+
+    describe('when scanned repos is non-empty', async () => {
+      describe('when not all results are downloaded', async () => {
+        it('should say the variant analysis is not complete', async () => {
+          expect(isVariantAnalysisComplete(variantAnalysis, async () => false)).to.equal(false);
+        });
+      });
+
+      describe('when all results are downloaded', async () => {
+        it('should say the variant analysis is complete', async () => {
+          expect(isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(true);
+        });
+      });
+    });
+  });
+
+  for (const variantAnalysisStatus of [
+    VariantAnalysisStatus.Succeeded,
+    VariantAnalysisStatus.Failed,
+    VariantAnalysisStatus.Canceled
+  ]) {
+    describe(`when variant analysis status is ${variantAnalysisStatus}`, async () => {
+      beforeEach(() => {
+        variantAnalysis.status = variantAnalysisStatus;
+      });
+
+      describe('when scanned repos is undefined', async () => {
+        it('should say the variant analysis is complete', async () => {
+          variantAnalysis.scannedRepos = undefined;
+          expect(isVariantAnalysisComplete(variantAnalysis, uncallableArtifactDownloadChecker)).to.equal(true);
+        });
+      });
+
+      describe('when scanned repos is empty', async () => {
+        it('should say the variant analysis is complete', async () => {
+          variantAnalysis.scannedRepos = [];
+          expect(isVariantAnalysisComplete(variantAnalysis, uncallableArtifactDownloadChecker)).to.equal(true);
+        });
+      });
+
+      describe('when scanned repos is non-empty', async () => {
+        describe('when not all results are downloaded', async () => {
+          it('should say the variant analysis is not complete', async () => {
+            expect(isVariantAnalysisComplete(variantAnalysis, async () => false)).to.equal(false);
+          });
+        });
+
+        describe('when all results are downloaded', async () => {
+          it('should say the variant analysis is complete', async () => {
+            expect(isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(true);
+          });
+        });
+      });
+    });
+  }
+});