From c4b1134903db6886e2bd83a88882f5a9dcf11588 Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 27 Oct 2022 16:58:43 +0100 Subject: [PATCH 01/12] Rename variantAnalysis to be more specific --- .../variant-analysis-manager.test.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) 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 dd1553698..f4ac94988 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 @@ -27,7 +27,7 @@ describe('Variant Analysis Manager', async function() { let cli: CodeQLCliServer; let cancellationTokenSource: CancellationTokenSource; let variantAnalysisManager: VariantAnalysisManager; - let variantAnalysis: VariantAnalysisApiResponse; + let variantAnalysisApiResponse: VariantAnalysisApiResponse; let scannedRepos: ApiVariantAnalysisScannedRepository[]; let getVariantAnalysisRepoStub: sinon.SinonStub; let getVariantAnalysisRepoResultStub: sinon.SinonStub; @@ -43,7 +43,7 @@ describe('Variant Analysis Manager', async function() { cancellationTokenSource = new CancellationTokenSource(); scannedRepos = createMockScannedRepos(); - variantAnalysis = createMockApiResponse('in_progress', scannedRepos); + variantAnalysisApiResponse = createMockApiResponse('in_progress', scannedRepos); try { const extension = await extensions.getExtension>('GitHub.vscode-codeql')!.activate(); @@ -66,7 +66,7 @@ describe('Variant Analysis Manager', async function() { try { await variantAnalysisManager.autoDownloadVariantAnalysisResult( scannedRepos[0], - variantAnalysis, + variantAnalysisApiResponse, cancellationTokenSource.token ); } catch (error: any) { @@ -103,7 +103,7 @@ describe('Variant Analysis Manager', async function() { it('should not try to download the result', async () => { await variantAnalysisManager.autoDownloadVariantAnalysisResult( scannedRepos[0], - variantAnalysis, + variantAnalysisApiResponse, cancellationTokenSource.token ); @@ -126,7 +126,7 @@ describe('Variant Analysis Manager', async function() { await variantAnalysisManager.autoDownloadVariantAnalysisResult( scannedRepos[0], - variantAnalysis, + variantAnalysisApiResponse, cancellationTokenSource.token ); @@ -136,7 +136,7 @@ describe('Variant Analysis Manager', async function() { it('should fetch a repo task', async () => { await variantAnalysisManager.autoDownloadVariantAnalysisResult( scannedRepos[0], - variantAnalysis, + variantAnalysisApiResponse, cancellationTokenSource.token ); @@ -146,7 +146,7 @@ describe('Variant Analysis Manager', async function() { it('should fetch a repo result', async () => { await variantAnalysisManager.autoDownloadVariantAnalysisResult( scannedRepos[0], - variantAnalysis, + variantAnalysisApiResponse, cancellationTokenSource.token ); @@ -156,9 +156,9 @@ describe('Variant Analysis Manager', async function() { it('should pop download tasks off the queue', async () => { const getResultsSpy = sandbox.spy(variantAnalysisManager, 'autoDownloadVariantAnalysisResult'); - await variantAnalysisManager.enqueueDownload(scannedRepos[0], variantAnalysis, cancellationTokenSource.token); - await variantAnalysisManager.enqueueDownload(scannedRepos[1], variantAnalysis, cancellationTokenSource.token); - await variantAnalysisManager.enqueueDownload(scannedRepos[2], variantAnalysis, cancellationTokenSource.token); + await variantAnalysisManager.enqueueDownload(scannedRepos[0], variantAnalysisApiResponse, cancellationTokenSource.token); + await variantAnalysisManager.enqueueDownload(scannedRepos[1], variantAnalysisApiResponse, cancellationTokenSource.token); + await variantAnalysisManager.enqueueDownload(scannedRepos[2], variantAnalysisApiResponse, cancellationTokenSource.token); expect(variantAnalysisManager.downloadsQueueSize()).to.equal(0); expect(getResultsSpy).to.have.been.calledThrice; From 51589e953eb5a493f9bf40835532dc1dee200034 Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 27 Oct 2022 17:01:50 +0100 Subject: [PATCH 02/12] Move test fixtures earlier in file --- .../variant-analysis-results-manager.test.ts | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts index 9f7c840d6..7b7992dea 100644 --- a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts @@ -13,7 +13,6 @@ import { CodeQLCliServer } from '../../../cli'; import { storagePath } from '../global.helper'; import { faker } from '@faker-js/faker'; import * as ghApiClient from '../../../remote-queries/gh-api/gh-api-client'; -import { VariantAnalysisRepoTask } from '../../../remote-queries/gh-api/variant-analysis'; describe(VariantAnalysisResultsManager.name, function() { this.timeout(10000); @@ -47,15 +46,24 @@ describe(VariantAnalysisResultsManager.name, function() { describe('download', () => { let getOctokitStub: sinon.SinonStub; - let variantAnalysisStoragePath: string; const mockCredentials = { getOctokit: () => Promise.resolve({ request: getOctokitStub }) } as unknown as Credentials; + let dummyRepoTask = createMockVariantAnalysisRepoTask(); + let variantAnalysisStoragePath: string; + let repoTaskStorageDirectory: string; beforeEach(async () => { + dummyRepoTask = createMockVariantAnalysisRepoTask(); + variantAnalysisStoragePath = path.join(storagePath, variantAnalysisId.toString()); + repoTaskStorageDirectory = variantAnalysisResultsManager.getRepoStorageDirectory(variantAnalysisStoragePath, dummyRepoTask.repository.full_name); + }); + + afterEach(async () => { + fs.rmSync(variantAnalysisStoragePath, { recursive: true }); }); describe('when the artifact_url is missing', async () => { @@ -79,14 +87,9 @@ describe(VariantAnalysisResultsManager.name, function() { }); describe('when the artifact_url is present', async () => { - let dummyRepoTask: VariantAnalysisRepoTask; - let storageDirectory: string; let arrayBuffer: ArrayBuffer; beforeEach(async () => { - dummyRepoTask = createMockVariantAnalysisRepoTask(); - - storageDirectory = variantAnalysisResultsManager.getRepoStorageDirectory(variantAnalysisStoragePath, dummyRepoTask.repository.full_name); const sourceFilePath = path.join(__dirname, '../../../../src/vscode-tests/cli-integration/data/variant-analysis-results.zip'); arrayBuffer = fs.readFileSync(sourceFilePath).buffer; @@ -96,11 +99,6 @@ describe(VariantAnalysisResultsManager.name, function() { .resolves(arrayBuffer); }); - afterEach(async () => { - fs.removeSync(`${storageDirectory}/results.zip`); - fs.removeSync(`${storageDirectory}/results`); - }); - it('should call the API to download the results', async () => { await variantAnalysisResultsManager.download( mockCredentials, @@ -120,7 +118,7 @@ describe(VariantAnalysisResultsManager.name, function() { variantAnalysisStoragePath ); - expect(fs.existsSync(`${storageDirectory}/results.zip`)).to.be.true; + expect(fs.existsSync(`${repoTaskStorageDirectory}/results.zip`)).to.be.true; }); it('should unzip the results in a `results/` folder', async () => { @@ -131,7 +129,7 @@ describe(VariantAnalysisResultsManager.name, function() { variantAnalysisStoragePath ); - expect(fs.existsSync(`${storageDirectory}/results/results.sarif`)).to.be.true; + expect(fs.existsSync(`${repoTaskStorageDirectory}/results/results.sarif`)).to.be.true; }); }); }); From caeaba2f2fc88f013d78ba093d78990c80f5a328 Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 27 Oct 2022 17:04:26 +0100 Subject: [PATCH 03/12] Make isVariantAnalysisRepoDownloaded public --- .../variant-analysis-results-manager.ts | 2 +- .../variant-analysis-results-manager.test.ts | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/remote-queries/variant-analysis-results-manager.ts b/extensions/ql-vscode/src/remote-queries/variant-analysis-results-manager.ts index e61fdadd4..b9af5ab1f 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-results-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-results-manager.ts @@ -146,7 +146,7 @@ export class VariantAnalysisResultsManager extends DisposableObject { throw new Error('Missing results file'); } - private async isVariantAnalysisRepoDownloaded( + public async isVariantAnalysisRepoDownloaded( variantAnalysisStoragePath: string, repositoryFullName: string, ): Promise { diff --git a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts index 7b7992dea..4fed66214 100644 --- a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts @@ -66,6 +66,12 @@ describe(VariantAnalysisResultsManager.name, function() { fs.rmSync(variantAnalysisStoragePath, { recursive: true }); }); + describe('isVariantAnalysisRepoDownloaded', () => { + it('should return false when no results are downloaded', async () => { + expect(await variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(variantAnalysisStoragePath, dummyRepoTask.repository.full_name)).to.equal(false); + }); + }); + describe('when the artifact_url is missing', async () => { it('should not try to download the result', async () => { const dummyRepoTask = createMockVariantAnalysisRepoTask(); @@ -131,6 +137,19 @@ describe(VariantAnalysisResultsManager.name, function() { expect(fs.existsSync(`${repoTaskStorageDirectory}/results/results.sarif`)).to.be.true; }); + + describe('isVariantAnalysisRepoDownloaded', () => { + it('should return true once results are downloaded', async () => { + await variantAnalysisResultsManager.download( + mockCredentials, + variantAnalysisId, + dummyRepoTask, + variantAnalysisStoragePath + ); + + expect(await variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(variantAnalysisStoragePath, dummyRepoTask.repository.full_name)).to.equal(true); + }); + }); }); }); }); From 599a9ed5d9706d7d68f25477c3a6206b767c4faf Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 27 Oct 2022 17:05:32 +0100 Subject: [PATCH 04/12] When rehydrating, always trigger a monitoring command if variant analysis is not complete --- extensions/ql-vscode/src/query-history.ts | 2 +- .../remote-queries/shared/variant-analysis.ts | 8 + .../variant-analysis-manager.ts | 37 +++- .../variant-analysis-manager.test.ts | 168 +++++++++++++++++- 4 files changed, 206 insertions(+), 9 deletions(-) diff --git a/extensions/ql-vscode/src/query-history.ts b/extensions/ql-vscode/src/query-history.ts index 36f523bfa..b38e9d3c7 100644 --- a/extensions/ql-vscode/src/query-history.ts +++ b/extensions/ql-vscode/src/query-history.ts @@ -694,7 +694,7 @@ export class QueryHistoryManager extends DisposableObject { await this.remoteQueriesManager.rehydrateRemoteQuery(item.queryId, item.remoteQuery, item.status); } if (item.t === 'variant-analysis') { - await this.variantAnalysisManager.rehydrateVariantAnalysis(item.variantAnalysis, item.status); + await this.variantAnalysisManager.rehydrateVariantAnalysis(item.variantAnalysis); } })); } diff --git a/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts b/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts index 9bd4b4557..900ce5f9a 100644 --- a/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts +++ b/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts @@ -151,6 +151,14 @@ export function hasRepoScanCompleted(repo: VariantAnalysisScannedRepository): bo return isCompletedAnalysisRepoStatus(repo.analysisStatus); } +/** + * @param repo + * @returns whether the repo scan is complete and has results that can be downloaded + */ +export function repoScanHasResults(repo: VariantAnalysisScannedRepository): boolean { + return repo.analysisStatus === VariantAnalysisRepoStatus.Succeeded && (repo.resultCount || 0) > 0; +} + /** * @param repos * @returns the total number of results. Will be `undefined` when there are no repos with results. 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 a035e9e08..38872e2e6 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts @@ -11,10 +11,14 @@ import { VariantAnalysisScannedRepository as ApiVariantAnalysisScannedRepository } from './gh-api/variant-analysis'; import { + hasRepoScanCompleted, + repoScanHasResults, VariantAnalysis, VariantAnalysisQueryLanguage, + VariantAnalysisScannedRepository, VariantAnalysisScannedRepositoryDownloadStatus, VariantAnalysisScannedRepositoryResult, - VariantAnalysisScannedRepositoryState + VariantAnalysisScannedRepositoryState, + VariantAnalysisStatus } from './shared/variant-analysis'; import { getErrorMessage } from '../pure/helpers-pure'; import { VariantAnalysisView } from './variant-analysis-view'; @@ -24,7 +28,6 @@ import { getControllerRepo } from './run-remote-query'; import { processUpdatedVariantAnalysis } from './variant-analysis-processor'; import PQueue from 'p-queue'; import { createTimestampFile, showAndLogErrorMessage } from '../helpers'; -import { QueryStatus } from '../query-status'; import * as fs from 'fs-extra'; @@ -56,7 +59,7 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA this.variantAnalysisResultsManager.onResultLoaded(this.onRepoResultLoaded.bind(this)); } - public async rehydrateVariantAnalysis(variantAnalysis: VariantAnalysis, status: QueryStatus) { + public async rehydrateVariantAnalysis(variantAnalysis: VariantAnalysis) { if (!(await this.variantAnalysisRecordExists(variantAnalysis.id))) { // In this case, the variant analysis was deleted from disk, most likely because // it was purged by another workspace. @@ -64,14 +67,34 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA } else { this.variantAnalyses.set(variantAnalysis.id, variantAnalysis); await this.getView(variantAnalysis.id)?.updateView(variantAnalysis); - if (status === QueryStatus.InProgress) { - // In this case, last time we checked, the query was still in progress. - // We need to setup the monitor to check for completion. + + if (!await this.isVariantAnalysisComplete(variantAnalysis)) { await commands.executeCommand('codeQL.monitorVariantAnalysis', variantAnalysis); } } } + public async isVariantAnalysisComplete(variantAnalysis: VariantAnalysis): Promise { + // 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 { + if (!hasRepoScanCompleted(repo)) { + return false; + } else if (!repoScanHasResults(repo)) { + return true; + } else { + const storageLocation = this.getVariantAnalysisStorageLocation(variantAnalysisId); + return await this.variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(storageLocation, repo.repository.fullName); + } + } + public async removeVariantAnalysis(variantAnalysis: VariantAnalysis) { this.variantAnalysisResultsManager.removeAnalysisResults(variantAnalysis); await this.removeStorageDirectory(variantAnalysis.id); @@ -234,7 +257,7 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA * used by the query history manager to determine when the directory * should be deleted. */ - private async prepareStorageDirectory(variantAnalysisId: number): Promise { + public async prepareStorageDirectory(variantAnalysisId: number): Promise { await createTimestampFile(this.getVariantAnalysisStorageLocation(variantAnalysisId)); } 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 f4ac94988..89b596fcf 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 @@ -1,6 +1,6 @@ import * as sinon from 'sinon'; import { expect } from 'chai'; -import { CancellationTokenSource, extensions } from 'vscode'; +import { CancellationTokenSource, commands, Disposable, extensions } from 'vscode'; import { CodeQLExtensionInterface } from '../../../extension'; import { logger } from '../../../logging'; import * as config from '../../../config'; @@ -21,6 +21,8 @@ import { createMockVariantAnalysisRepoTask } from '../../factories/remote-querie 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'; describe('Variant Analysis Manager', async function() { let sandbox: sinon.SinonSandbox; @@ -165,4 +167,168 @@ describe('Variant Analysis Manager', async function() { }); }); }); + + describe('when rehydrating a query', async () => { + let variantAnalysis: VariantAnalysis; + let variantAnalysisRemovedFired = false; + let onVariantAnalysisRemovedListener: Disposable; + let monitorVariantAnalysisCommandCalled = false; + + beforeEach(() => { + variantAnalysis = createMockVariantAnalysis(); + + variantAnalysisRemovedFired = false; + onVariantAnalysisRemovedListener = variantAnalysisManager.onVariantAnalysisRemoved(() => { + variantAnalysisRemovedFired = true; + }); + + monitorVariantAnalysisCommandCalled = false; + sandbox.stub(commands, 'executeCommand').callsFake((command: string) => { + if (command === 'codeQL.monitorVariantAnalysis') { + monitorVariantAnalysisCommandCalled = true; + } + return Promise.resolve(); + }); + }); + + afterEach(() => { + onVariantAnalysisRemovedListener.dispose(); + }); + + describe('when variant analysis record doesn\'t exist', async () => { + it('should remove the variant analysis', async () => { + await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); + expect(variantAnalysisRemovedFired).to.equal(true); + }); + + it('should not trigger a monitoring command', async () => { + await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); + expect(monitorVariantAnalysisCommandCalled).to.equal(false); + }); + }); + + describe('when variant analysis record does exist', async () => { + let variantAnalysisStorageLocation: string; + + beforeEach(async () => { + variantAnalysisStorageLocation = variantAnalysisManager.getVariantAnalysisStorageLocation(variantAnalysis.id); + await variantAnalysisManager.prepareStorageDirectory(variantAnalysis.id); + }); + + afterEach(() => { + fs.rmSync(variantAnalysisStorageLocation, { recursive: true }); + }); + + describe('when the variant analysis is not complete', async () => { + beforeEach(() => { + sinon.stub(variantAnalysisManager, 'isVariantAnalysisComplete').resolves(false); + }); + + it('should not remove the variant analysis', async () => { + await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); + expect(variantAnalysisRemovedFired).to.equal(false); + }); + + it('should trigger a monitoring command', async () => { + await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); + expect(monitorVariantAnalysisCommandCalled).to.equal(true); + }); + }); + + describe('when the variant analysis is complete', async () => { + beforeEach(() => { + sinon.stub(variantAnalysisManager, 'isVariantAnalysisComplete').resolves(true); + }); + + it('should not remove the variant analysis', async () => { + await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); + expect(variantAnalysisRemovedFired).to.equal(false); + }); + + it('should not trigger a monitoring command', async () => { + await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); + expect(monitorVariantAnalysisCommandCalled).to.equal(false); + }); + }); + }); + }); + + 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); + }); + }); + }); + }); + } + }); }); From 7748f82c9688965686ce5ee803ce52cc1c5a6427 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 31 Oct 2022 10:24:34 +0000 Subject: [PATCH 05/12] Stop checking result count and rename repoScanHasResults --- .../ql-vscode/src/remote-queries/shared/variant-analysis.ts | 6 +++--- .../src/remote-queries/variant-analysis-manager.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts b/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts index 900ce5f9a..9505ad6ab 100644 --- a/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts +++ b/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts @@ -153,10 +153,10 @@ export function hasRepoScanCompleted(repo: VariantAnalysisScannedRepository): bo /** * @param repo - * @returns whether the repo scan is complete and has results that can be downloaded + * @returns whether the repo scan has an artifact that can be downloaded */ -export function repoScanHasResults(repo: VariantAnalysisScannedRepository): boolean { - return repo.analysisStatus === VariantAnalysisRepoStatus.Succeeded && (repo.resultCount || 0) > 0; +export function repoHasDownloadableArtifact(repo: VariantAnalysisScannedRepository): boolean { + return repo.analysisStatus === VariantAnalysisRepoStatus.Succeeded; } /** 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 38872e2e6..c9e8b6e52 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts @@ -12,7 +12,7 @@ import { } from './gh-api/variant-analysis'; import { hasRepoScanCompleted, - repoScanHasResults, + repoHasDownloadableArtifact, VariantAnalysis, VariantAnalysisQueryLanguage, VariantAnalysisScannedRepository, VariantAnalysisScannedRepositoryDownloadStatus, @@ -87,7 +87,7 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA private async isVariantAnalysisRepoComplete(variantAnalysisId: number, repo: VariantAnalysisScannedRepository): Promise { if (!hasRepoScanCompleted(repo)) { return false; - } else if (!repoScanHasResults(repo)) { + } else if (!repoHasDownloadableArtifact(repo)) { return true; } else { const storageLocation = this.getVariantAnalysisStorageLocation(variantAnalysisId); From 7e59d4c736372ac6405d774fd9cd1e9501c96072 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 31 Oct 2022 10:38:44 +0000 Subject: [PATCH 06/12] Convert to using sinon spies --- .../variant-analysis-manager.test.ts | 38 +++++++------------ 1 file changed, 13 insertions(+), 25 deletions(-) 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 89b596fcf..2212cdd4b 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 @@ -1,6 +1,6 @@ import * as sinon from 'sinon'; import { expect } from 'chai'; -import { CancellationTokenSource, commands, Disposable, extensions } from 'vscode'; +import { CancellationTokenSource, commands, extensions } from 'vscode'; import { CodeQLExtensionInterface } from '../../../extension'; import { logger } from '../../../logging'; import * as config from '../../../config'; @@ -170,40 +170,28 @@ describe('Variant Analysis Manager', async function() { describe('when rehydrating a query', async () => { let variantAnalysis: VariantAnalysis; - let variantAnalysisRemovedFired = false; - let onVariantAnalysisRemovedListener: Disposable; - let monitorVariantAnalysisCommandCalled = false; + let variantAnalysisRemovedSpy: sinon.SinonSpy; + let monitorVariantAnalysisCommandSpy: sinon.SinonSpy; beforeEach(() => { variantAnalysis = createMockVariantAnalysis(); - variantAnalysisRemovedFired = false; - onVariantAnalysisRemovedListener = variantAnalysisManager.onVariantAnalysisRemoved(() => { - variantAnalysisRemovedFired = true; - }); + variantAnalysisRemovedSpy = sinon.spy(); + variantAnalysisManager.onVariantAnalysisRemoved(variantAnalysisRemovedSpy); - monitorVariantAnalysisCommandCalled = false; - sandbox.stub(commands, 'executeCommand').callsFake((command: string) => { - if (command === 'codeQL.monitorVariantAnalysis') { - monitorVariantAnalysisCommandCalled = true; - } - return Promise.resolve(); - }); - }); - - afterEach(() => { - onVariantAnalysisRemovedListener.dispose(); + monitorVariantAnalysisCommandSpy = sinon.spy(); + sandbox.stub(commands, 'executeCommand').callsFake(monitorVariantAnalysisCommandSpy); }); describe('when variant analysis record doesn\'t exist', async () => { it('should remove the variant analysis', async () => { await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); - expect(variantAnalysisRemovedFired).to.equal(true); + sinon.assert.calledOnce(variantAnalysisRemovedSpy); }); it('should not trigger a monitoring command', async () => { await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); - expect(monitorVariantAnalysisCommandCalled).to.equal(false); + sinon.assert.notCalled(monitorVariantAnalysisCommandSpy); }); }); @@ -226,12 +214,12 @@ describe('Variant Analysis Manager', async function() { it('should not remove the variant analysis', async () => { await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); - expect(variantAnalysisRemovedFired).to.equal(false); + sinon.assert.notCalled(variantAnalysisRemovedSpy); }); it('should trigger a monitoring command', async () => { await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); - expect(monitorVariantAnalysisCommandCalled).to.equal(true); + sinon.assert.calledWith(monitorVariantAnalysisCommandSpy, 'codeQL.monitorVariantAnalysis'); }); }); @@ -242,12 +230,12 @@ describe('Variant Analysis Manager', async function() { it('should not remove the variant analysis', async () => { await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); - expect(variantAnalysisRemovedFired).to.equal(false); + sinon.assert.notCalled(variantAnalysisRemovedSpy); }); it('should not trigger a monitoring command', async () => { await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); - expect(monitorVariantAnalysisCommandCalled).to.equal(false); + sinon.assert.notCalled(monitorVariantAnalysisCommandSpy); }); }); }); From b53366f27772067a14e12321f0cbc6eddf41c43d Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 31 Oct 2022 11:19:33 +0000 Subject: [PATCH 07/12] Move isVariantAnalysisComplete implementation out of variant analysis manager --- .../remote-queries/shared/variant-analysis.ts | 20 +++++ .../variant-analysis-manager.ts | 30 ++----- .../variant-analysis-manager.test.ts | 86 +------------------ .../test/pure-tests/variant-analysis.test.ts | 79 ++++++++++++++++- 4 files changed, 108 insertions(+), 107 deletions(-) diff --git a/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts b/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts index 9505ad6ab..daaba9df5 100644 --- a/extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts +++ b/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 +): Promise { + // 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 +): Promise { + 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 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 c9e8b6e52..70af514a0 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts +++ b/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 { - // 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 { - 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 { + const storageLocation = this.getVariantAnalysisStorageLocation(variantAnalysis.id); + return (repo) => this.variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(storageLocation, repo.repository.fullName); } public async removeVariantAnalysis(variantAnalysis: VariantAnalysis) { 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 2212cdd4b..a9e5d03e9 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 @@ -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); - }); - }); - }); - }); - } - }); }); diff --git a/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts b/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts index d66ea4975..40e33b682 100644 --- a/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts +++ b/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); + }); + }); + }); + }); + } +}); From b497c4fa0080ccaf8da16148e7240c61300c289d Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 31 Oct 2022 11:19:52 +0000 Subject: [PATCH 08/12] make public prepareStorageDirectory private --- .../ql-vscode/src/remote-queries/variant-analysis-manager.ts | 2 +- .../remote-queries/variant-analysis-manager.test.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) 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 70af514a0..5f86a45ce 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts @@ -239,7 +239,7 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA * used by the query history manager to determine when the directory * should be deleted. */ - public async prepareStorageDirectory(variantAnalysisId: number): Promise { + private async prepareStorageDirectory(variantAnalysisId: number): Promise { await createTimestampFile(this.getVariantAnalysisStorageLocation(variantAnalysisId)); } 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 a9e5d03e9..b13572f50 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 @@ -24,6 +24,7 @@ import { VariantAnalysisResultsManager } from '../../../remote-queries/variant-a import { createMockVariantAnalysis } from '../../factories/remote-queries/shared/variant-analysis'; import { VariantAnalysis } from '../../../remote-queries/shared/variant-analysis'; import * as VariantAnalysisModule from '../../../remote-queries/shared/variant-analysis'; +import { createTimestampFile } from '../../../helpers'; describe('Variant Analysis Manager', async function() { let sandbox: sinon.SinonSandbox; @@ -201,7 +202,7 @@ describe('Variant Analysis Manager', async function() { beforeEach(async () => { variantAnalysisStorageLocation = variantAnalysisManager.getVariantAnalysisStorageLocation(variantAnalysis.id); - await variantAnalysisManager.prepareStorageDirectory(variantAnalysis.id); + await createTimestampFile(variantAnalysisStorageLocation); }); afterEach(() => { From b751cee618985443c0f4c41c4742af0cb87bc2a7 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 31 Oct 2022 11:39:05 +0000 Subject: [PATCH 09/12] Check directory exists before deleting --- .../remote-queries/variant-analysis-results-manager.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts index 4fed66214..fc928d9b1 100644 --- a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts @@ -63,7 +63,9 @@ describe(VariantAnalysisResultsManager.name, function() { }); afterEach(async () => { - fs.rmSync(variantAnalysisStoragePath, { recursive: true }); + if (fs.existsSync(variantAnalysisStoragePath)) { + fs.rmSync(variantAnalysisStoragePath, { recursive: true }); + } }); describe('isVariantAnalysisRepoDownloaded', () => { From c9038f53348dbd1dea853dba12690118898d88c0 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 31 Oct 2022 14:29:26 +0000 Subject: [PATCH 10/12] Add awaits --- .../test/pure-tests/variant-analysis.test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts b/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts index 40e33b682..da238ef15 100644 --- a/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts +++ b/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts @@ -28,20 +28,20 @@ describe('isVariantAnalysisComplete', async () => { 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); + expect(await 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); + expect(await 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); + expect(await isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(true); }); }); }); @@ -60,27 +60,27 @@ describe('isVariantAnalysisComplete', async () => { 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); + expect(await 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); + expect(await 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); + expect(await 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); + expect(await isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(true); }); }); }); From 6dc684f2b60971d0a8945c0379b34dc6a98e675e Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 31 Oct 2022 15:14:52 +0000 Subject: [PATCH 11/12] Fix unit test expectated results :facepalm: --- .../test/pure-tests/variant-analysis.test.ts | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts b/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts index da238ef15..0a97e03df 100644 --- a/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts +++ b/extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts @@ -1,5 +1,6 @@ import { expect } from 'chai'; -import { VariantAnalysis, parseVariantAnalysisQueryLanguage, VariantAnalysisQueryLanguage, VariantAnalysisStatus, isVariantAnalysisComplete } from '../../src/remote-queries/shared/variant-analysis'; +import { VariantAnalysis, parseVariantAnalysisQueryLanguage, VariantAnalysisQueryLanguage, VariantAnalysisStatus, isVariantAnalysisComplete, VariantAnalysisRepoStatus } from '../../src/remote-queries/shared/variant-analysis'; +import { createMockScannedRepo } from '../../src/vscode-tests/factories/remote-queries/shared/scanned-repositories'; import { createMockVariantAnalysis } from '../../src/vscode-tests/factories/remote-queries/shared/variant-analysis'; describe('parseVariantAnalysisQueryLanguage', () => { @@ -41,7 +42,7 @@ describe('isVariantAnalysisComplete', async () => { describe('when all results are downloaded', async () => { it('should say the variant analysis is complete', async () => { - expect(await isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(true); + expect(await isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(false); }); }); }); @@ -71,17 +72,30 @@ describe('isVariantAnalysisComplete', async () => { }); }); - 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(await isVariantAnalysisComplete(variantAnalysis, async () => false)).to.equal(false); - }); + describe('when a repo scan is still in progress', async () => { + it('should say the variant analysis is not complete', async () => { + variantAnalysis.scannedRepos = [ + createMockScannedRepo('in-progress-repo', false, VariantAnalysisRepoStatus.InProgress), + ]; + expect(await isVariantAnalysisComplete(variantAnalysis, async () => false)).to.equal(false); }); + }); - describe('when all results are downloaded', async () => { - it('should say the variant analysis is complete', async () => { - expect(await isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(true); - }); + describe('when not all results are downloaded', async () => { + it('should say the variant analysis is not complete', async () => { + variantAnalysis.scannedRepos = [ + createMockScannedRepo('in-progress-repo', false, VariantAnalysisRepoStatus.Succeeded), + ]; + expect(await isVariantAnalysisComplete(variantAnalysis, async () => false)).to.equal(false); + }); + }); + + describe('when all results are downloaded', async () => { + it('should say the variant analysis is complete', async () => { + variantAnalysis.scannedRepos = [ + createMockScannedRepo('in-progress-repo', false, VariantAnalysisRepoStatus.Succeeded), + ]; + expect(await isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(true); }); }); }); From 12d52550731aca5de98a8f9e8e4662a4cb8ecf70 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 31 Oct 2022 15:41:28 +0000 Subject: [PATCH 12/12] Fix rehydrateVariantAnalysis integration tests The method no longer accepts a second argument --- .../remote-queries/variant-analysis-history.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-history.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-history.test.ts index 5d6292fa4..0e9406e40 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-history.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-history.test.ts @@ -121,9 +121,7 @@ describe('Variant Analyses and QueryHistoryManager', function() { expect(rehydrateVariantAnalysisStub).to.have.callCount(2); expect(rehydrateVariantAnalysisStub.getCall(0).args[0]).to.deep.eq(rawQueryHistory[0].variantAnalysis); - expect(rehydrateVariantAnalysisStub.getCall(0).args[1]).to.deep.eq(rawQueryHistory[0].status); expect(rehydrateVariantAnalysisStub.getCall(1).args[0]).to.deep.eq(rawQueryHistory[1].variantAnalysis); - expect(rehydrateVariantAnalysisStub.getCall(1).args[1]).to.deep.eq(rawQueryHistory[1].status); expect(qhm.treeDataProvider.allHistory[0]).to.deep.eq(rawQueryHistory[0]); expect(qhm.treeDataProvider.allHistory[1]).to.deep.eq(rawQueryHistory[1]);