From 0c483d1e2966bdd60ecf8b852bbf544a9f267e71 Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 20 Dec 2022 15:26:19 +0000 Subject: [PATCH 1/8] Remove places where we are checking if credentials are undefined This cannot happen already, even before the other changes in this PR. The Credentials.initialize method can never return undefined, so these checks would never return true. The real place that checks that we are authenticated is in the vscode.authentication.getSession method, and it will reject the promise if the user declines to authenticate or anything else means we can't get an authenticated session. I feel justified in removing the tests for these cases because the code was never actually called in production, and we are covered by the vscode authentication library rejecting the promise. Any exception thrown from Credentials.initialize would behave the same as the one I'm deleting. --- .../remote-queries/remote-queries-monitor.ts | 4 - .../variant-analysis-manager.ts | 6 - .../variant-analysis-monitor.ts | 3 - .../variant-analysis-manager.test.ts | 624 ++++++++---------- .../variant-analysis-monitor.test.ts | 407 ++++++------ 5 files changed, 472 insertions(+), 572 deletions(-) diff --git a/extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts b/extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts index 475404414..7017c46ad 100644 --- a/extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts +++ b/extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts @@ -27,10 +27,6 @@ export class RemoteQueriesMonitor { ): Promise { const credentials = await Credentials.initialize(this.extensionContext); - if (!credentials) { - throw Error("Error authenticating with GitHub"); - } - let attemptCount = 0; while (attemptCount <= RemoteQueriesMonitor.maxAttemptCount) { 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 e045e4575..392ac7b57 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts @@ -480,9 +480,6 @@ export class VariantAnalysisManager await this.onRepoStateUpdated(variantAnalysis.id, repoState); const credentials = await Credentials.initialize(this.ctx); - if (!credentials) { - throw Error("Error authenticating with GitHub"); - } if (cancellationToken && cancellationToken.isCancellationRequested) { repoState.downloadStatus = @@ -581,9 +578,6 @@ export class VariantAnalysisManager } const credentials = await Credentials.initialize(this.ctx); - if (!credentials) { - throw Error("Error authenticating with GitHub"); - } void showAndLogInformationMessage( "Cancelling variant analysis. This may take a while.", diff --git a/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts b/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts index ba528de15..267d2ec5b 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts @@ -45,9 +45,6 @@ export class VariantAnalysisMonitor extends DisposableObject { cancellationToken: CancellationToken, ): Promise { const credentials = await Credentials.initialize(this.extensionContext); - if (!credentials) { - throw Error("Error authenticating with GitHub"); - } let attemptCount = 0; const scannedReposDownloaded: number[] = []; 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 3a2a3863e..052b2c0c9 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 @@ -142,6 +142,14 @@ describe("Variant Analysis Manager", () => { beforeEach(async () => { writeFileStub.mockRestore(); + const mockCredentials = { + getOctokit: () => + Promise.resolve({ + request: jest.fn(), + }), + } as unknown as Credentials; + jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials); + // Should not have asked for a language showQuickPickSpy = jest .spyOn(window, "showQuickPick") @@ -367,269 +375,267 @@ describe("Variant Analysis Manager", () => { }); describe("autoDownloadVariantAnalysisResult", () => { - describe("when credentials are invalid", () => { + let arrayBuffer: ArrayBuffer; + + let getVariantAnalysisRepoStub: jest.SpiedFunction< + typeof ghApiClient.getVariantAnalysisRepo + >; + let getVariantAnalysisRepoResultStub: jest.SpiedFunction< + typeof ghApiClient.getVariantAnalysisRepoResult + >; + + beforeEach(async () => { + const mockCredentials = { + getOctokit: () => + Promise.resolve({ + request: jest.fn(), + }), + } as unknown as Credentials; + jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials); + + const sourceFilePath = join( + __dirname, + "../../../../src/vscode-tests/cli-integration/data/variant-analysis-results.zip", + ); + arrayBuffer = fs.readFileSync(sourceFilePath).buffer; + + getVariantAnalysisRepoStub = jest.spyOn( + ghApiClient, + "getVariantAnalysisRepo", + ); + getVariantAnalysisRepoResultStub = jest.spyOn( + ghApiClient, + "getVariantAnalysisRepoResult", + ); + }); + + describe("when the artifact_url is missing", () => { beforeEach(async () => { - jest - .spyOn(Credentials, "initialize") - .mockResolvedValue(undefined as unknown as Credentials); + const dummyRepoTask = createMockVariantAnalysisRepoTask(); + delete dummyRepoTask.artifact_url; + + getVariantAnalysisRepoStub.mockResolvedValue(dummyRepoTask); + getVariantAnalysisRepoResultStub.mockResolvedValue(arrayBuffer); }); - it("should return early if credentials are wrong", async () => { - try { - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ); - } catch (error: any) { - expect(error.message).toBe("Error authenticating with GitHub"); - } + it("should not try to download the result", async () => { + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(getVariantAnalysisRepoResultStub).not.toHaveBeenCalled(); }); }); - describe("when credentials are valid", () => { - let arrayBuffer: ArrayBuffer; - - let getVariantAnalysisRepoStub: jest.SpiedFunction< - typeof ghApiClient.getVariantAnalysisRepo - >; - let getVariantAnalysisRepoResultStub: jest.SpiedFunction< - typeof ghApiClient.getVariantAnalysisRepoResult - >; + describe("when the artifact_url is present", () => { + let dummyRepoTask: VariantAnalysisRepoTask; beforeEach(async () => { - const mockCredentials = { - getOctokit: () => - Promise.resolve({ - request: jest.fn(), - }), - } as unknown as Credentials; - jest - .spyOn(Credentials, "initialize") - .mockResolvedValue(mockCredentials); + dummyRepoTask = createMockVariantAnalysisRepoTask(); - const sourceFilePath = join( - __dirname, - "../../../../src/vscode-tests/cli-integration/data/variant-analysis-results.zip", - ); - arrayBuffer = fs.readFileSync(sourceFilePath).buffer; + getVariantAnalysisRepoStub.mockResolvedValue(dummyRepoTask); + getVariantAnalysisRepoResultStub.mockResolvedValue(arrayBuffer); + }); - getVariantAnalysisRepoStub = jest.spyOn( - ghApiClient, - "getVariantAnalysisRepo", + it("should return early if variant analysis is cancelled", async () => { + cancellationTokenSource.cancel(); + + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], + variantAnalysis, + cancellationTokenSource.token, ); - getVariantAnalysisRepoResultStub = jest.spyOn( - ghApiClient, - "getVariantAnalysisRepoResult", + + expect(getVariantAnalysisRepoStub).not.toHaveBeenCalled(); + }); + + it("should fetch a repo task", async () => { + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(getVariantAnalysisRepoStub).toHaveBeenCalled(); + }); + + it("should fetch a repo result", async () => { + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(getVariantAnalysisRepoResultStub).toHaveBeenCalled(); + }); + + it("should skip the download if the repository has already been downloaded", async () => { + // First, do a download so it is downloaded. This avoids having to mock the repo states. + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], + variantAnalysis, + cancellationTokenSource.token, + ); + + getVariantAnalysisRepoStub.mockClear(); + + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(getVariantAnalysisRepoStub).not.toHaveBeenCalled(); + }); + + it("should write the repo state when the download is successful", async () => { + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(outputJsonStub).toHaveBeenCalledWith( + join(storagePath, variantAnalysis.id.toString(), "repo_states.json"), + { + [scannedRepos[0].repository.id]: { + repositoryId: scannedRepos[0].repository.id, + downloadStatus: + VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, + }, + }, ); }); - describe("when the artifact_url is missing", () => { - beforeEach(async () => { - const dummyRepoTask = createMockVariantAnalysisRepoTask(); - delete dummyRepoTask.artifact_url; + it("should not write the repo state when the download fails", async () => { + getVariantAnalysisRepoResultStub.mockRejectedValue( + new Error("Failed to download"), + ); - getVariantAnalysisRepoStub.mockResolvedValue(dummyRepoTask); - getVariantAnalysisRepoResultStub.mockResolvedValue(arrayBuffer); - }); - - it("should not try to download the result", async () => { - await variantAnalysisManager.autoDownloadVariantAnalysisResult( + await expect( + variantAnalysisManager.autoDownloadVariantAnalysisResult( scannedRepos[0], variantAnalysis, cancellationTokenSource.token, - ); + ), + ).rejects.toThrow(); - expect(getVariantAnalysisRepoResultStub).not.toHaveBeenCalled(); - }); + expect(outputJsonStub).not.toHaveBeenCalled(); }); - describe("when the artifact_url is present", () => { - let dummyRepoTask: VariantAnalysisRepoTask; + it("should have a failed repo state when the repo task API fails", async () => { + getVariantAnalysisRepoStub.mockRejectedValueOnce( + new Error("Failed to download"), + ); - beforeEach(async () => { - dummyRepoTask = createMockVariantAnalysisRepoTask(); - - getVariantAnalysisRepoStub.mockResolvedValue(dummyRepoTask); - getVariantAnalysisRepoResultStub.mockResolvedValue(arrayBuffer); - }); - - it("should return early if variant analysis is cancelled", async () => { - cancellationTokenSource.cancel(); - - await variantAnalysisManager.autoDownloadVariantAnalysisResult( + await expect( + variantAnalysisManager.autoDownloadVariantAnalysisResult( scannedRepos[0], variantAnalysis, cancellationTokenSource.token, - ); + ), + ).rejects.toThrow(); - expect(getVariantAnalysisRepoStub).not.toHaveBeenCalled(); - }); + expect(outputJsonStub).not.toHaveBeenCalled(); - it("should fetch a repo task", async () => { - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ); + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[1], + variantAnalysis, + cancellationTokenSource.token, + ); - expect(getVariantAnalysisRepoStub).toHaveBeenCalled(); - }); - - it("should fetch a repo result", async () => { - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(getVariantAnalysisRepoResultStub).toHaveBeenCalled(); - }); - - it("should skip the download if the repository has already been downloaded", async () => { - // First, do a download so it is downloaded. This avoids having to mock the repo states. - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ); - - getVariantAnalysisRepoStub.mockClear(); - - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(getVariantAnalysisRepoStub).not.toHaveBeenCalled(); - }); - - it("should write the repo state when the download is successful", async () => { - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(outputJsonStub).toHaveBeenCalledWith( - join( - storagePath, - variantAnalysis.id.toString(), - "repo_states.json", - ), - { - [scannedRepos[0].repository.id]: { - repositoryId: scannedRepos[0].repository.id, - downloadStatus: - VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, - }, + expect(outputJsonStub).toHaveBeenCalledWith( + join(storagePath, variantAnalysis.id.toString(), "repo_states.json"), + { + [scannedRepos[0].repository.id]: { + repositoryId: scannedRepos[0].repository.id, + downloadStatus: + VariantAnalysisScannedRepositoryDownloadStatus.Failed, }, - ); - }); + [scannedRepos[1].repository.id]: { + repositoryId: scannedRepos[1].repository.id, + downloadStatus: + VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, + }, + }, + ); + }); - it("should not write the repo state when the download fails", async () => { - getVariantAnalysisRepoResultStub.mockRejectedValue( - new Error("Failed to download"), - ); + it("should have a failed repo state when the download fails", async () => { + getVariantAnalysisRepoResultStub.mockRejectedValueOnce( + new Error("Failed to download"), + ); - await expect( - variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ), - ).rejects.toThrow(); - - expect(outputJsonStub).not.toHaveBeenCalled(); - }); - - it("should have a failed repo state when the repo task API fails", async () => { - getVariantAnalysisRepoStub.mockRejectedValueOnce( - new Error("Failed to download"), - ); - - await expect( - variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ), - ).rejects.toThrow(); - - expect(outputJsonStub).not.toHaveBeenCalled(); - - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[1], + await expect( + variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], variantAnalysis, cancellationTokenSource.token, - ); + ), + ).rejects.toThrow(); - expect(outputJsonStub).toHaveBeenCalledWith( - join( - storagePath, - variantAnalysis.id.toString(), - "repo_states.json", - ), - { - [scannedRepos[0].repository.id]: { - repositoryId: scannedRepos[0].repository.id, - downloadStatus: - VariantAnalysisScannedRepositoryDownloadStatus.Failed, - }, - [scannedRepos[1].repository.id]: { - repositoryId: scannedRepos[1].repository.id, - downloadStatus: - VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, - }, + expect(outputJsonStub).not.toHaveBeenCalled(); + + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[1], + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(outputJsonStub).toHaveBeenCalledWith( + join(storagePath, variantAnalysis.id.toString(), "repo_states.json"), + { + [scannedRepos[0].repository.id]: { + repositoryId: scannedRepos[0].repository.id, + downloadStatus: + VariantAnalysisScannedRepositoryDownloadStatus.Failed, }, - ); + [scannedRepos[1].repository.id]: { + repositoryId: scannedRepos[1].repository.id, + downloadStatus: + VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, + }, + }, + ); + }); + + it("should update the repo state correctly", async () => { + mockRepoStates({ + [scannedRepos[1].repository.id]: { + repositoryId: scannedRepos[1].repository.id, + downloadStatus: + VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, + }, + [scannedRepos[2].repository.id]: { + repositoryId: scannedRepos[2].repository.id, + downloadStatus: + VariantAnalysisScannedRepositoryDownloadStatus.InProgress, + }, }); - it("should have a failed repo state when the download fails", async () => { - getVariantAnalysisRepoResultStub.mockRejectedValueOnce( - new Error("Failed to download"), - ); + await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); - await expect( - variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ), - ).rejects.toThrow(); + expect(pathExistsStub).toBeCalledWith( + join(storagePath, variantAnalysis.id.toString()), + ); + expect(readJsonStub).toHaveBeenCalledTimes(1); + expect(readJsonStub).toHaveBeenCalledWith( + join(storagePath, variantAnalysis.id.toString(), "repo_states.json"), + ); - expect(outputJsonStub).not.toHaveBeenCalled(); + pathExistsStub.mockRestore(); - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[1], - variantAnalysis, - cancellationTokenSource.token, - ); + await variantAnalysisManager.autoDownloadVariantAnalysisResult( + scannedRepos[0], + variantAnalysis, + cancellationTokenSource.token, + ); - expect(outputJsonStub).toHaveBeenCalledWith( - join( - storagePath, - variantAnalysis.id.toString(), - "repo_states.json", - ), - { - [scannedRepos[0].repository.id]: { - repositoryId: scannedRepos[0].repository.id, - downloadStatus: - VariantAnalysisScannedRepositoryDownloadStatus.Failed, - }, - [scannedRepos[1].repository.id]: { - repositoryId: scannedRepos[1].repository.id, - downloadStatus: - VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, - }, - }, - ); - }); - - it("should update the repo state correctly", async () => { - mockRepoStates({ + expect(outputJsonStub).toHaveBeenCalledWith( + join(storagePath, variantAnalysis.id.toString(), "repo_states.json"), + { [scannedRepos[1].repository.id]: { repositoryId: scannedRepos[1].repository.id, downloadStatus: @@ -640,66 +646,22 @@ describe("Variant Analysis Manager", () => { downloadStatus: VariantAnalysisScannedRepositoryDownloadStatus.InProgress, }, - }); - - await variantAnalysisManager.rehydrateVariantAnalysis( - variantAnalysis, - ); - - expect(pathExistsStub).toBeCalledWith( - join(storagePath, variantAnalysis.id.toString()), - ); - expect(readJsonStub).toHaveBeenCalledTimes(1); - expect(readJsonStub).toHaveBeenCalledWith( - join( - storagePath, - variantAnalysis.id.toString(), - "repo_states.json", - ), - ); - - pathExistsStub.mockRestore(); - - await variantAnalysisManager.autoDownloadVariantAnalysisResult( - scannedRepos[0], - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(outputJsonStub).toHaveBeenCalledWith( - join( - storagePath, - variantAnalysis.id.toString(), - "repo_states.json", - ), - { - [scannedRepos[1].repository.id]: { - repositoryId: scannedRepos[1].repository.id, - downloadStatus: - VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, - }, - [scannedRepos[2].repository.id]: { - repositoryId: scannedRepos[2].repository.id, - downloadStatus: - VariantAnalysisScannedRepositoryDownloadStatus.InProgress, - }, - [scannedRepos[0].repository.id]: { - repositoryId: scannedRepos[0].repository.id, - downloadStatus: - VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, - }, + [scannedRepos[0].repository.id]: { + repositoryId: scannedRepos[0].repository.id, + downloadStatus: + VariantAnalysisScannedRepositoryDownloadStatus.Succeeded, }, - ); - }); - - function mockRepoStates( - repoStates: Record, - ) { - pathExistsStub.mockImplementation(() => true); - // This will read in the correct repo states - readJsonStub.mockImplementation(() => Promise.resolve(repoStates)); - } + }, + ); }); + + function mockRepoStates( + repoStates: Record, + ) { + pathExistsStub.mockImplementation(() => true); + // This will read in the correct repo states + readJsonStub.mockImplementation(() => Promise.resolve(repoStates)); + } }); }); @@ -876,6 +838,8 @@ describe("Variant Analysis Manager", () => { let variantAnalysisStorageLocation: string; + let mockCredentials: Credentials; + beforeEach(async () => { variantAnalysis = createMockVariantAnalysis({}); @@ -889,82 +853,54 @@ describe("Variant Analysis Manager", () => { ); await createTimestampFile(variantAnalysisStorageLocation); await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis); + + mockCredentials = { + getOctokit: () => + Promise.resolve({ + request: jest.fn(), + }), + } as unknown as Credentials; + jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials); }); afterEach(() => { fs.rmSync(variantAnalysisStorageLocation, { recursive: true }); }); - describe("when the credentials are invalid", () => { - beforeEach(async () => { - jest - .spyOn(Credentials, "initialize") - .mockResolvedValue(undefined as unknown as Credentials); - }); - - it("should return early", async () => { - try { - await variantAnalysisManager.cancelVariantAnalysis( - variantAnalysis.id, - ); - } catch (error: any) { - expect(error.message).toBe("Error authenticating with GitHub"); - } - }); + it("should return early if the variant analysis is not found", async () => { + try { + await variantAnalysisManager.cancelVariantAnalysis( + variantAnalysis.id + 100, + ); + } catch (error: any) { + expect(error.message).toBe( + `No variant analysis with id: ${variantAnalysis.id + 100}`, + ); + } }); - describe("when the credentials are valid", () => { - let mockCredentials: Credentials; - - beforeEach(async () => { - mockCredentials = { - getOctokit: () => - Promise.resolve({ - request: jest.fn(), - }), - } as unknown as Credentials; - jest - .spyOn(Credentials, "initialize") - .mockResolvedValue(mockCredentials); + it("should return early if the variant analysis does not have an actions workflow run id", async () => { + await variantAnalysisManager.onVariantAnalysisUpdated({ + ...variantAnalysis, + actionsWorkflowRunId: undefined, }); - it("should return early if the variant analysis is not found", async () => { - try { - await variantAnalysisManager.cancelVariantAnalysis( - variantAnalysis.id + 100, - ); - } catch (error: any) { - expect(error.message).toBe( - `No variant analysis with id: ${variantAnalysis.id + 100}`, - ); - } - }); - - it("should return early if the variant analysis does not have an actions workflow run id", async () => { - await variantAnalysisManager.onVariantAnalysisUpdated({ - ...variantAnalysis, - actionsWorkflowRunId: undefined, - }); - - try { - await variantAnalysisManager.cancelVariantAnalysis( - variantAnalysis.id, - ); - } catch (error: any) { - expect(error.message).toBe( - `No workflow run id for variant analysis with id: ${variantAnalysis.id}`, - ); - } - }); - - it("should return cancel if valid", async () => { + try { await variantAnalysisManager.cancelVariantAnalysis(variantAnalysis.id); - - expect(mockCancelVariantAnalysis).toBeCalledWith( - mockCredentials, - variantAnalysis, + } catch (error: any) { + expect(error.message).toBe( + `No workflow run id for variant analysis with id: ${variantAnalysis.id}`, ); - }); + } + }); + + it("should return cancel if valid", async () => { + await variantAnalysisManager.cancelVariantAnalysis(variantAnalysis.id); + + expect(mockCancelVariantAnalysis).toBeCalledWith( + mockCredentials, + variantAnalysis, + ); }); }); diff --git a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts index eb1700ef9..ca03e00b4 100644 --- a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts @@ -77,260 +77,237 @@ describe("Variant Analysis Monitor", () => { .mockRejectedValue(new Error("Not mocked")); limitNumberOfAttemptsToMonitor(); + + const mockCredentials = { + getOctokit: () => + Promise.resolve({ + request: jest.fn(), + }), + } as unknown as Credentials; + jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials); }); - describe("when credentials are invalid", () => { + it("should return early if variant analysis is cancelled", async () => { + cancellationTokenSource.cancel(); + + await variantAnalysisMonitor.monitorVariantAnalysis( + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(onVariantAnalysisChangeSpy).not.toHaveBeenCalled(); + }); + + it("should return early if variant analysis should be cancelled", async () => { + shouldCancelMonitor.mockResolvedValue(true); + + await variantAnalysisMonitor.monitorVariantAnalysis( + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(onVariantAnalysisChangeSpy).not.toHaveBeenCalled(); + }); + + describe("when the variant analysis fails", () => { + let mockFailedApiResponse: VariantAnalysisApiResponse; + beforeEach(async () => { - jest - .spyOn(Credentials, "initialize") - .mockResolvedValue(undefined as unknown as Credentials); + mockFailedApiResponse = createFailedMockApiResponse(); + mockGetVariantAnalysis.mockResolvedValue(mockFailedApiResponse); }); - it("should return early if credentials are wrong", async () => { - try { + it("should mark as failed and stop monitoring", async () => { + await variantAnalysisMonitor.monitorVariantAnalysis( + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(mockGetVariantAnalysis).toHaveBeenCalledTimes(1); + + expect(onVariantAnalysisChangeSpy).toHaveBeenCalledWith( + expect.objectContaining({ + status: VariantAnalysisStatus.Failed, + failureReason: processFailureReason( + mockFailedApiResponse.failure_reason as VariantAnalysisFailureReason, + ), + }), + ); + }); + }); + + describe("when the variant analysis is in progress", () => { + let mockApiResponse: VariantAnalysisApiResponse; + let scannedRepos: ApiVariantAnalysisScannedRepository[]; + let succeededRepos: ApiVariantAnalysisScannedRepository[]; + + describe("when there are successfully scanned repos", () => { + beforeEach(async () => { + scannedRepos = createMockScannedRepos([ + "pending", + "pending", + "in_progress", + "in_progress", + "succeeded", + "succeeded", + "succeeded", + ]); + mockApiResponse = createMockApiResponse("succeeded", scannedRepos); + mockGetVariantAnalysis.mockResolvedValue(mockApiResponse); + succeededRepos = scannedRepos.filter( + (r) => r.analysis_status === "succeeded", + ); + }); + + it("should trigger a download extension command for each repo", async () => { + const succeededRepos = scannedRepos.filter( + (r) => r.analysis_status === "succeeded", + ); + const commandSpy = jest + .spyOn(commands, "executeCommand") + .mockResolvedValue(undefined); + await variantAnalysisMonitor.monitorVariantAnalysis( variantAnalysis, cancellationTokenSource.token, ); - } catch (error: any) { - expect(error.message).toBe("Error authenticating with GitHub"); - } - }); - }); - describe("when credentials are valid", () => { - beforeEach(async () => { - const mockCredentials = { - getOctokit: () => - Promise.resolve({ - request: jest.fn(), - }), - } as unknown as Credentials; - jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials); + expect(commandSpy).toBeCalledTimes(succeededRepos.length); + + succeededRepos.forEach((succeededRepo, index) => { + expect(commandSpy).toHaveBeenNthCalledWith( + index + 1, + "codeQL.autoDownloadVariantAnalysisResult", + processScannedRepository(succeededRepo), + processUpdatedVariantAnalysis(variantAnalysis, mockApiResponse), + ); + }); + }); + + it("should download all available results", async () => { + await variantAnalysisMonitor.monitorVariantAnalysis( + variantAnalysis, + cancellationTokenSource.token, + ); + + expect(mockGetDownloadResult).toBeCalledTimes(succeededRepos.length); + + succeededRepos.forEach((succeededRepo, index) => { + expect(mockGetDownloadResult).toHaveBeenNthCalledWith( + index + 1, + processScannedRepository(succeededRepo), + processUpdatedVariantAnalysis(variantAnalysis, mockApiResponse), + undefined, + ); + }); + }); }); - it("should return early if variant analysis is cancelled", async () => { - cancellationTokenSource.cancel(); - - await variantAnalysisMonitor.monitorVariantAnalysis( - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(onVariantAnalysisChangeSpy).not.toHaveBeenCalled(); - }); - - it("should return early if variant analysis should be cancelled", async () => { - shouldCancelMonitor.mockResolvedValue(true); - - await variantAnalysisMonitor.monitorVariantAnalysis( - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(onVariantAnalysisChangeSpy).not.toHaveBeenCalled(); - }); - - describe("when the variant analysis fails", () => { - let mockFailedApiResponse: VariantAnalysisApiResponse; + describe("when there are only in progress repos", () => { + let scannedRepos: ApiVariantAnalysisScannedRepository[]; beforeEach(async () => { - mockFailedApiResponse = createFailedMockApiResponse(); - mockGetVariantAnalysis.mockResolvedValue(mockFailedApiResponse); + scannedRepos = createMockScannedRepos(["pending", "in_progress"]); + mockApiResponse = createMockApiResponse("in_progress", scannedRepos); + mockGetVariantAnalysis.mockResolvedValue(mockApiResponse); }); - it("should mark as failed and stop monitoring", async () => { + it("should succeed and not download any repos via a command", async () => { + const commandSpy = jest + .spyOn(commands, "executeCommand") + .mockResolvedValue(undefined); + await variantAnalysisMonitor.monitorVariantAnalysis( variantAnalysis, cancellationTokenSource.token, ); - expect(mockGetVariantAnalysis).toHaveBeenCalledTimes(1); + expect(commandSpy).not.toHaveBeenCalled(); + }); - expect(onVariantAnalysisChangeSpy).toHaveBeenCalledWith( - expect.objectContaining({ - status: VariantAnalysisStatus.Failed, - failureReason: processFailureReason( - mockFailedApiResponse.failure_reason as VariantAnalysisFailureReason, - ), - }), + it("should not try to download any repos", async () => { + await variantAnalysisMonitor.monitorVariantAnalysis( + variantAnalysis, + cancellationTokenSource.token, ); + + expect(mockGetDownloadResult).not.toBeCalled(); }); }); - describe("when the variant analysis is in progress", () => { - let mockApiResponse: VariantAnalysisApiResponse; + describe("when the responses change", () => { let scannedRepos: ApiVariantAnalysisScannedRepository[]; - let succeededRepos: ApiVariantAnalysisScannedRepository[]; - describe("when there are successfully scanned repos", () => { - beforeEach(async () => { - scannedRepos = createMockScannedRepos([ - "pending", - "pending", - "in_progress", - "in_progress", - "succeeded", - "succeeded", - "succeeded", - ]); - mockApiResponse = createMockApiResponse("succeeded", scannedRepos); - mockGetVariantAnalysis.mockResolvedValue(mockApiResponse); - succeededRepos = scannedRepos.filter( - (r) => r.analysis_status === "succeeded", - ); - }); + beforeEach(async () => { + scannedRepos = createMockScannedRepos([ + "pending", + "in_progress", + "in_progress", + "in_progress", + "pending", + "pending", + ]); + mockApiResponse = createMockApiResponse("in_progress", scannedRepos); + mockGetVariantAnalysis.mockResolvedValueOnce(mockApiResponse); - it("should trigger a download extension command for each repo", async () => { - const succeededRepos = scannedRepos.filter( - (r) => r.analysis_status === "succeeded", - ); - const commandSpy = jest - .spyOn(commands, "executeCommand") - .mockResolvedValue(undefined); + let nextApiResponse = { + ...mockApiResponse, + scanned_repositories: [...scannedRepos.map((r) => ({ ...r }))], + }; + nextApiResponse.scanned_repositories[0].analysis_status = "succeeded"; + nextApiResponse.scanned_repositories[1].analysis_status = "succeeded"; + mockGetVariantAnalysis.mockResolvedValueOnce(nextApiResponse); - await variantAnalysisMonitor.monitorVariantAnalysis( - variantAnalysis, - cancellationTokenSource.token, - ); + nextApiResponse = { + ...mockApiResponse, + scanned_repositories: [ + ...nextApiResponse.scanned_repositories.map((r) => ({ ...r })), + ], + }; + nextApiResponse.scanned_repositories[2].analysis_status = "succeeded"; + nextApiResponse.scanned_repositories[5].analysis_status = "succeeded"; + mockGetVariantAnalysis.mockResolvedValueOnce(nextApiResponse); - expect(commandSpy).toBeCalledTimes(succeededRepos.length); - - succeededRepos.forEach((succeededRepo, index) => { - expect(commandSpy).toHaveBeenNthCalledWith( - index + 1, - "codeQL.autoDownloadVariantAnalysisResult", - processScannedRepository(succeededRepo), - processUpdatedVariantAnalysis(variantAnalysis, mockApiResponse), - ); - }); - }); - - it("should download all available results", async () => { - await variantAnalysisMonitor.monitorVariantAnalysis( - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(mockGetDownloadResult).toBeCalledTimes(succeededRepos.length); - - succeededRepos.forEach((succeededRepo, index) => { - expect(mockGetDownloadResult).toHaveBeenNthCalledWith( - index + 1, - processScannedRepository(succeededRepo), - processUpdatedVariantAnalysis(variantAnalysis, mockApiResponse), - undefined, - ); - }); - }); + nextApiResponse = { + ...mockApiResponse, + scanned_repositories: [ + ...nextApiResponse.scanned_repositories.map((r) => ({ ...r })), + ], + }; + nextApiResponse.scanned_repositories[3].analysis_status = "succeeded"; + nextApiResponse.scanned_repositories[4].analysis_status = "failed"; + mockGetVariantAnalysis.mockResolvedValueOnce(nextApiResponse); }); - describe("when there are only in progress repos", () => { - let scannedRepos: ApiVariantAnalysisScannedRepository[]; + it("should trigger a download extension command for each repo", async () => { + const commandSpy = jest + .spyOn(commands, "executeCommand") + .mockResolvedValue(undefined); - beforeEach(async () => { - scannedRepos = createMockScannedRepos(["pending", "in_progress"]); - mockApiResponse = createMockApiResponse("in_progress", scannedRepos); - mockGetVariantAnalysis.mockResolvedValue(mockApiResponse); - }); + await variantAnalysisMonitor.monitorVariantAnalysis( + variantAnalysis, + cancellationTokenSource.token, + ); - it("should succeed and not download any repos via a command", async () => { - const commandSpy = jest - .spyOn(commands, "executeCommand") - .mockResolvedValue(undefined); + expect(mockGetVariantAnalysis).toBeCalledTimes(4); + expect(commandSpy).toBeCalledTimes(5); + }); + }); - await variantAnalysisMonitor.monitorVariantAnalysis( - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(commandSpy).not.toHaveBeenCalled(); - }); - - it("should not try to download any repos", async () => { - await variantAnalysisMonitor.monitorVariantAnalysis( - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(mockGetDownloadResult).not.toBeCalled(); - }); + describe("when there are no repos to scan", () => { + beforeEach(async () => { + scannedRepos = []; + mockApiResponse = createMockApiResponse("succeeded", scannedRepos); + mockGetVariantAnalysis.mockResolvedValue(mockApiResponse); }); - describe("when the responses change", () => { - let scannedRepos: ApiVariantAnalysisScannedRepository[]; + it("should not try to download any repos", async () => { + await variantAnalysisMonitor.monitorVariantAnalysis( + variantAnalysis, + cancellationTokenSource.token, + ); - beforeEach(async () => { - scannedRepos = createMockScannedRepos([ - "pending", - "in_progress", - "in_progress", - "in_progress", - "pending", - "pending", - ]); - mockApiResponse = createMockApiResponse("in_progress", scannedRepos); - mockGetVariantAnalysis.mockResolvedValueOnce(mockApiResponse); - - let nextApiResponse = { - ...mockApiResponse, - scanned_repositories: [...scannedRepos.map((r) => ({ ...r }))], - }; - nextApiResponse.scanned_repositories[0].analysis_status = "succeeded"; - nextApiResponse.scanned_repositories[1].analysis_status = "succeeded"; - mockGetVariantAnalysis.mockResolvedValueOnce(nextApiResponse); - - nextApiResponse = { - ...mockApiResponse, - scanned_repositories: [ - ...nextApiResponse.scanned_repositories.map((r) => ({ ...r })), - ], - }; - nextApiResponse.scanned_repositories[2].analysis_status = "succeeded"; - nextApiResponse.scanned_repositories[5].analysis_status = "succeeded"; - mockGetVariantAnalysis.mockResolvedValueOnce(nextApiResponse); - - nextApiResponse = { - ...mockApiResponse, - scanned_repositories: [ - ...nextApiResponse.scanned_repositories.map((r) => ({ ...r })), - ], - }; - nextApiResponse.scanned_repositories[3].analysis_status = "succeeded"; - nextApiResponse.scanned_repositories[4].analysis_status = "failed"; - mockGetVariantAnalysis.mockResolvedValueOnce(nextApiResponse); - }); - - it("should trigger a download extension command for each repo", async () => { - const commandSpy = jest - .spyOn(commands, "executeCommand") - .mockResolvedValue(undefined); - - await variantAnalysisMonitor.monitorVariantAnalysis( - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(mockGetVariantAnalysis).toBeCalledTimes(4); - expect(commandSpy).toBeCalledTimes(5); - }); - }); - - describe("when there are no repos to scan", () => { - beforeEach(async () => { - scannedRepos = []; - mockApiResponse = createMockApiResponse("succeeded", scannedRepos); - mockGetVariantAnalysis.mockResolvedValue(mockApiResponse); - }); - - it("should not try to download any repos", async () => { - await variantAnalysisMonitor.monitorVariantAnalysis( - variantAnalysis, - cancellationTokenSource.token, - ); - - expect(mockGetDownloadResult).not.toBeCalled(); - }); + expect(mockGetDownloadResult).not.toBeCalled(); }); }); }); From 7e8ce35485c5bf7f2bfed66d51eadb1e7ba48a52 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 21 Dec 2022 14:24:13 +0000 Subject: [PATCH 2/8] Remove the requiresAuthentication parameter It is true by default and no place in the codebase sets it to false. We can simplify the code by removing this case we aren't using. If we want this behaviour in the future we can always implement it again, but I think it's likely to be unnecessary and if you don't want authenticated requests then you likely won't be initializing a Credentials object. --- extensions/ql-vscode/src/authentication.ts | 13 +++---------- extensions/ql-vscode/src/databaseFetcher.ts | 2 +- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index 597c5a078..9386bd50b 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -90,24 +90,17 @@ export class Credentials { /** * Creates or returns an instance of Octokit. * - * @param requireAuthentication Whether the Octokit instance needs to be authenticated as user. * @returns An instance of Octokit. */ - async getOctokit(requireAuthentication = true): Promise { + async getOctokit(): Promise { if (this.octokit) { return this.octokit; } - this.octokit = await this.createOctokit(requireAuthentication); + this.octokit = await this.createOctokit(true); if (!this.octokit) { - if (requireAuthentication) { - throw new Error("Did not initialize Octokit."); - } - - // We don't want to set this in this.octokit because that would prevent - // authenticating when requireCredentials is true. - return new Octokit.Octokit({ retry }); + throw new Error("Did not initialize Octokit."); } return this.octokit; } diff --git a/extensions/ql-vscode/src/databaseFetcher.ts b/extensions/ql-vscode/src/databaseFetcher.ts index 3380201ec..d3280bea1 100644 --- a/extensions/ql-vscode/src/databaseFetcher.ts +++ b/extensions/ql-vscode/src/databaseFetcher.ts @@ -106,7 +106,7 @@ export async function promptImportGithubDatabase( } const octokit = credentials - ? await credentials.getOctokit(true) + ? await credentials.getOctokit() : new Octokit.Octokit({ retry }); const result = await convertGithubNwoToDatabaseUrl(nwo, octokit, progress); From 74f10a306e2084884be0e8dbe16aace463ba7212 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 21 Dec 2022 14:27:50 +0000 Subject: [PATCH 3/8] Remove the overrideToken parameter from createOctokit This was only used from initializeWithToken and only added a completely separate case to the start of the method, effectively turning it into two separate implementations. Therefore we can make things simpler by inlining this case in the one place it is used. --- extensions/ql-vscode/src/authentication.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index 9386bd50b..ee60f030f 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -48,18 +48,13 @@ export class Credentials { */ static async initializeWithToken(overrideToken: string) { const c = new Credentials(); - c.octokit = await c.createOctokit(false, overrideToken); + c.octokit = new Octokit.Octokit({ auth: overrideToken, retry }); return c; } private async createOctokit( createIfNone: boolean, - overrideToken?: string, ): Promise { - if (overrideToken) { - return new Octokit.Octokit({ auth: overrideToken, retry }); - } - const session = await vscode.authentication.getSession( GITHUB_AUTH_PROVIDER_ID, SCOPES, From 8c05b3a508666a092d1d1e8f040a7c7918dae6f3 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 21 Dec 2022 14:36:59 +0000 Subject: [PATCH 4/8] Don't try to pre-populate an octokit I argue that calling createOctokit(false) adds no benefit. If an authenticated session already exists then this silently create an octokit, which makes getOctokit() a no-op just returning the field. However if there is already an authenticated session then getOctokit() would already be able to create an octokit without prompting the user. On the other hand if there isn't an authenticated session then we won't be able to pre-populate an octokit, so getOctokit() will have to prompt the user anyway. Not calling createOctokit(false) in registerListeners also doesn't change behaviour. If the user is authenticated in the new session then we would be able to create an octokit instance wihtout prompting in getOctokit anyway. If the user is not authenticated in the new session then we won't be able to create an instance without prompting either way. The only benefit I can think of is that it moves a tiny amount of computation earlier in the pipeline, but the amount of computation is tiny and it isn't any more async than it would be if it happened in getOctokit(). I don't think this is worth making the code more complex. --- extensions/ql-vscode/src/authentication.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index ee60f030f..830436f94 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -34,7 +34,6 @@ export class Credentials { ): Promise { const c = new Credentials(); c.registerListeners(context); - c.octokit = await c.createOctokit(false); return c; } @@ -76,7 +75,7 @@ export class Credentials { context.subscriptions.push( vscode.authentication.onDidChangeSessions(async (e) => { if (e.provider.id === GITHUB_AUTH_PROVIDER_ID) { - this.octokit = await this.createOctokit(false); + this.octokit = undefined; } }), ); From 8f34f6af2e1b1138d203d490db9be9bfe47e4c68 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 21 Dec 2022 14:41:17 +0000 Subject: [PATCH 5/8] Remove the createIfNone parameter from createOctokit At this point we are only ever passing true, so we may as well remove the parameter and simplify the code. --- extensions/ql-vscode/src/authentication.ts | 26 ++++++---------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index 830436f94..93a6a9d46 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -51,23 +51,17 @@ export class Credentials { return c; } - private async createOctokit( - createIfNone: boolean, - ): Promise { + private async createOctokit(): Promise { const session = await vscode.authentication.getSession( GITHUB_AUTH_PROVIDER_ID, SCOPES, - { createIfNone }, + { createIfNone: true }, ); - if (session) { - return new Octokit.Octokit({ - auth: session.accessToken, - retry, - }); - } else { - return undefined; - } + return new Octokit.Octokit({ + auth: session.accessToken, + retry, + }); } registerListeners(context: vscode.ExtensionContext): void { @@ -87,14 +81,8 @@ export class Credentials { * @returns An instance of Octokit. */ async getOctokit(): Promise { - if (this.octokit) { - return this.octokit; - } - - this.octokit = await this.createOctokit(true); - if (!this.octokit) { - throw new Error("Did not initialize Octokit."); + this.octokit = await this.createOctokit(); } return this.octokit; } From 551f76cc4e0114132841169fd9ffb80305971139 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 21 Dec 2022 14:45:20 +0000 Subject: [PATCH 6/8] Create a new octokit instance every time I believe this doesn't change the user-visible behaviour at all. The user won't be prompted to log in any more or less often than they would have done before. One benefit of this is that we can remove the registerListeners method because we no longer need to know if the cached octokit is still valid. Instead we just call vscode.authentication.getSession every time and it will return the current session, which might be different from the last time we called it. This might prompt the user to log in, but that would have happened anyway because when the session changed we would have overwritten our cached octokit instance. Another benefit is that we no longer need the extension context and this removed a surprisingly large amount of code where we are passing this parameter around because we need it for the credentials. The only downside I can see is that we call getSession more often and create more javascript objects in general. I believe the performance impact of this will be negligible and not worth worrying about. --- extensions/ql-vscode/src/authentication.ts | 49 +++++++------------ extensions/ql-vscode/src/extension.ts | 9 ++-- extensions/ql-vscode/src/query-history.ts | 4 +- .../analyses-results-manager.ts | 7 ++- .../src/remote-queries/export-results.ts | 15 +----- .../remote-queries/remote-queries-manager.ts | 9 ++-- .../remote-queries/remote-queries-monitor.ts | 7 +-- .../variant-analysis-manager.ts | 7 ++- .../variant-analysis-monitor.ts | 10 +--- .../variant-analysis-monitor.test.ts | 5 +- .../remote-queries/export-results.test.ts | 3 -- .../remote-query-history.test.ts | 1 - 12 files changed, 40 insertions(+), 86 deletions(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index 93a6a9d46..de8578b4a 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -13,42 +13,38 @@ const SCOPES = ["repo", "gist"]; * Handles authentication to GitHub, using the VS Code [authentication API](https://code.visualstudio.com/api/references/vscode-api#authentication). */ export class Credentials { + /** + * A specific octokit to return, otherwise a new authenticated octokit will be created when needed. + */ private octokit: Octokit.Octokit | undefined; // Explicitly make the constructor private, so that we can't accidentally call the constructor from outside the class // without also initializing the class. - // eslint-disable-next-line @typescript-eslint/no-empty-function - private constructor() {} + private constructor(octokit?: Octokit.Octokit) { + this.octokit = octokit; + } /** - * Initializes an instance of credentials with an octokit instance. + * Initializes a Credentials instance. This will generate octokit instances + * authenticated as the user. If there is not already an authenticated GitHub + * session availabeT then the user will be prompted to log in. * - * Do not call this method until you know you actually need an instance of credentials. - * since calling this method will require the user to log in. - * - * @param context The extension context. * @returns An instance of credentials. */ - static async initialize( - context: vscode.ExtensionContext, - ): Promise { - const c = new Credentials(); - c.registerListeners(context); - return c; + static async initialize(): Promise { + return new Credentials(); } /** * Initializes an instance of credentials with an octokit instance using - * a token from the user's GitHub account. This method is meant to be - * used non-interactive environments such as tests. + * a specific known token. This method is meant to be used non-interactive + * environments such as tests. * * @param overrideToken The GitHub token to use for authentication. * @returns An instance of credentials. */ static async initializeWithToken(overrideToken: string) { - const c = new Credentials(); - c.octokit = new Octokit.Octokit({ auth: overrideToken, retry }); - return c; + return new Credentials(new Octokit.Octokit({ auth: overrideToken, retry })); } private async createOctokit(): Promise { @@ -64,26 +60,15 @@ export class Credentials { }); } - registerListeners(context: vscode.ExtensionContext): void { - // Sessions are changed when a user logs in or logs out. - context.subscriptions.push( - vscode.authentication.onDidChangeSessions(async (e) => { - if (e.provider.id === GITHUB_AUTH_PROVIDER_ID) { - this.octokit = undefined; - } - }), - ); - } - /** * Creates or returns an instance of Octokit. * * @returns An instance of Octokit. */ async getOctokit(): Promise { - if (!this.octokit) { - this.octokit = await this.createOctokit(); + if (this.octokit) { + return this.octokit; } - return this.octokit; + return await this.createOctokit(); } } diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index abf49939b..c52097052 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -591,7 +591,7 @@ async function activateWithInstalledDistribution( qs, getContextStoragePath(ctx), ctx.extensionPath, - () => Credentials.initialize(ctx), + () => Credentials.initialize(), ); databaseUI.init(); ctx.subscriptions.push(databaseUI); @@ -1236,7 +1236,7 @@ async function activateWithInstalledDistribution( commandRunner( "codeQL.exportRemoteQueryResults", async (queryId: string) => { - await exportRemoteQueryResults(qhm, rqm, ctx, queryId); + await exportRemoteQueryResults(qhm, rqm, queryId); }, ), ); @@ -1251,7 +1251,6 @@ async function activateWithInstalledDistribution( filterSort?: RepositoriesFilterSortStateWithIds, ) => { await exportVariantAnalysisResults( - ctx, variantAnalysisManager, variantAnalysisId, filterSort, @@ -1356,7 +1355,7 @@ async function activateWithInstalledDistribution( "codeQL.chooseDatabaseGithub", async (progress: ProgressCallback, token: CancellationToken) => { const credentials = isCanary() - ? await Credentials.initialize(ctx) + ? await Credentials.initialize() : undefined; await databaseUI.handleChooseDatabaseGithub( credentials, @@ -1411,7 +1410,7 @@ async function activateWithInstalledDistribution( * Credentials for authenticating to GitHub. * These are used when making API calls. */ - const credentials = await Credentials.initialize(ctx); + const credentials = await Credentials.initialize(); const octokit = await credentials.getOctokit(); const userInfo = await octokit.users.getAuthenticated(); void showAndLogInformationMessage( diff --git a/extensions/ql-vscode/src/query-history.ts b/extensions/ql-vscode/src/query-history.ts index 0052b860d..4e1731ad3 100644 --- a/extensions/ql-vscode/src/query-history.ts +++ b/extensions/ql-vscode/src/query-history.ts @@ -397,7 +397,7 @@ export class QueryHistoryManager extends DisposableObject { private readonly variantAnalysisManager: VariantAnalysisManager, private readonly evalLogViewer: EvalLogViewer, private readonly queryStorageDir: string, - private readonly ctx: ExtensionContext, + ctx: ExtensionContext, private readonly queryHistoryConfigListener: QueryHistoryConfig, private readonly labelProvider: HistoryItemLabelProvider, private readonly doCompareCallback: ( @@ -633,7 +633,7 @@ export class QueryHistoryManager extends DisposableObject { } private getCredentials() { - return Credentials.initialize(this.ctx); + return Credentials.initialize(); } /** diff --git a/extensions/ql-vscode/src/remote-queries/analyses-results-manager.ts b/extensions/ql-vscode/src/remote-queries/analyses-results-manager.ts index b8974007f..04f94d68a 100644 --- a/extensions/ql-vscode/src/remote-queries/analyses-results-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/analyses-results-manager.ts @@ -1,7 +1,7 @@ import { pathExists } from "fs-extra"; import { EOL } from "os"; import { extname } from "path"; -import { CancellationToken, ExtensionContext } from "vscode"; +import { CancellationToken } from "vscode"; import { Credentials } from "../authentication"; import { Logger } from "../common"; @@ -26,7 +26,6 @@ export class AnalysesResultsManager { private readonly analysesResults: Map; constructor( - private readonly ctx: ExtensionContext, private readonly cliServer: CodeQLCliServer, readonly storagePath: string, private readonly logger: Logger, @@ -43,7 +42,7 @@ export class AnalysesResultsManager { return; } - const credentials = await Credentials.initialize(this.ctx); + const credentials = await Credentials.initialize(); void this.logger.log( `Downloading and processing results for ${analysisSummary.nwo}`, @@ -77,7 +76,7 @@ export class AnalysesResultsManager { (x) => !this.isAnalysisInMemory(x), ); - const credentials = await Credentials.initialize(this.ctx); + const credentials = await Credentials.initialize(); void this.logger.log("Downloading and processing analyses results"); diff --git a/extensions/ql-vscode/src/remote-queries/export-results.ts b/extensions/ql-vscode/src/remote-queries/export-results.ts index 1641269d4..47c51c0c9 100644 --- a/extensions/ql-vscode/src/remote-queries/export-results.ts +++ b/extensions/ql-vscode/src/remote-queries/export-results.ts @@ -4,7 +4,6 @@ import { ensureDir, writeFile } from "fs-extra"; import { commands, CancellationToken, - ExtensionContext, Uri, ViewColumn, window, @@ -74,7 +73,6 @@ export async function exportSelectedRemoteQueryResults( export async function exportRemoteQueryResults( queryHistoryManager: QueryHistoryManager, remoteQueriesManager: RemoteQueriesManager, - ctx: ExtensionContext, queryId: string, ): Promise { const queryHistoryItem = queryHistoryManager.getRemoteQueryById(queryId); @@ -107,7 +105,6 @@ export async function exportRemoteQueryResults( const exportedResultsDirectory = join(exportDirectory, "exported-results"); await exportRemoteQueryAnalysisResults( - ctx, exportedResultsDirectory, query, analysesResults, @@ -116,7 +113,6 @@ export async function exportRemoteQueryResults( } export async function exportRemoteQueryAnalysisResults( - ctx: ExtensionContext, exportedResultsPath: string, query: RemoteQuery, analysesResults: AnalysisResults[], @@ -126,7 +122,6 @@ export async function exportRemoteQueryAnalysisResults( const markdownFiles = generateMarkdown(query, analysesResults, exportFormat); await exportResults( - ctx, exportedResultsPath, description, markdownFiles, @@ -141,7 +136,6 @@ const MAX_VARIANT_ANALYSIS_EXPORT_PROGRESS_STEPS = 2; * The user is prompted to select the export format. */ export async function exportVariantAnalysisResults( - ctx: ExtensionContext, variantAnalysisManager: VariantAnalysisManager, variantAnalysisId: number, filterSort: RepositoriesFilterSortStateWithIds | undefined, @@ -255,7 +249,6 @@ export async function exportVariantAnalysisResults( ); await exportVariantAnalysisAnalysisResults( - ctx, exportedResultsDirectory, variantAnalysis, getAnalysesResults(), @@ -266,7 +259,6 @@ export async function exportVariantAnalysisResults( } export async function exportVariantAnalysisAnalysisResults( - ctx: ExtensionContext, exportedResultsPath: string, variantAnalysis: VariantAnalysis, analysesResults: AsyncIterable< @@ -297,7 +289,6 @@ export async function exportVariantAnalysisAnalysisResults( ); await exportResults( - ctx, exportedResultsPath, description, markdownFiles, @@ -341,7 +332,6 @@ async function determineExportFormat(): Promise<"gist" | "local" | undefined> { } export async function exportResults( - ctx: ExtensionContext, exportedResultsPath: string, description: string, markdownFiles: MarkdownFile[], @@ -354,7 +344,7 @@ export async function exportResults( } if (exportFormat === "gist") { - await exportToGist(ctx, description, markdownFiles, progress, token); + await exportToGist(description, markdownFiles, progress, token); } else if (exportFormat === "local") { await exportToLocalMarkdown( exportedResultsPath, @@ -366,7 +356,6 @@ export async function exportResults( } export async function exportToGist( - ctx: ExtensionContext, description: string, markdownFiles: MarkdownFile[], progress?: ProgressCallback, @@ -378,7 +367,7 @@ export async function exportToGist( message: "Creating Gist", }); - const credentials = await Credentials.initialize(ctx); + const credentials = await Credentials.initialize(); if (token?.isCancellationRequested) { throw new UserCancellationException("Cancelled"); diff --git a/extensions/ql-vscode/src/remote-queries/remote-queries-manager.ts b/extensions/ql-vscode/src/remote-queries/remote-queries-manager.ts index 065488131..10c9925b3 100644 --- a/extensions/ql-vscode/src/remote-queries/remote-queries-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/remote-queries-manager.ts @@ -81,20 +81,19 @@ export class RemoteQueriesManager extends DisposableObject { private readonly view: RemoteQueriesView; constructor( - private readonly ctx: ExtensionContext, + ctx: ExtensionContext, private readonly cliServer: CodeQLCliServer, private readonly storagePath: string, logger: Logger, ) { super(); this.analysesResultsManager = new AnalysesResultsManager( - ctx, cliServer, storagePath, logger, ); this.view = new RemoteQueriesView(ctx, logger, this.analysesResultsManager); - this.remoteQueriesMonitor = new RemoteQueriesMonitor(ctx, logger); + this.remoteQueriesMonitor = new RemoteQueriesMonitor(logger); this.remoteQueryAddedEventEmitter = this.push( new EventEmitter(), @@ -160,7 +159,7 @@ export class RemoteQueriesManager extends DisposableObject { progress: ProgressCallback, token: CancellationToken, ): Promise { - const credentials = await Credentials.initialize(this.ctx); + const credentials = await Credentials.initialize(); const { actionBranch, @@ -218,7 +217,7 @@ export class RemoteQueriesManager extends DisposableObject { remoteQuery: RemoteQuery, cancellationToken: CancellationToken, ): Promise { - const credentials = await Credentials.initialize(this.ctx); + const credentials = await Credentials.initialize(); const queryWorkflowResult = await this.remoteQueriesMonitor.monitorQuery( remoteQuery, diff --git a/extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts b/extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts index 7017c46ad..dd8c0c921 100644 --- a/extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts +++ b/extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts @@ -16,16 +16,13 @@ export class RemoteQueriesMonitor { private static readonly maxAttemptCount = 17280; private static readonly sleepTime = 5000; - constructor( - private readonly extensionContext: vscode.ExtensionContext, - private readonly logger: Logger, - ) {} + constructor(private readonly logger: Logger) {} public async monitorQuery( remoteQuery: RemoteQuery, cancellationToken: vscode.CancellationToken, ): Promise { - const credentials = await Credentials.initialize(this.extensionContext); + const credentials = await Credentials.initialize(); let attemptCount = 0; 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 392ac7b57..dd838ebc9 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts @@ -106,7 +106,6 @@ export class VariantAnalysisManager super(); this.variantAnalysisMonitor = this.push( new VariantAnalysisMonitor( - ctx, this.shouldCancelMonitorVariantAnalysis.bind(this), ), ); @@ -125,7 +124,7 @@ export class VariantAnalysisManager progress: ProgressCallback, token: CancellationToken, ): Promise { - const credentials = await Credentials.initialize(this.ctx); + const credentials = await Credentials.initialize(); const { actionBranch, @@ -479,7 +478,7 @@ export class VariantAnalysisManager await this.onRepoStateUpdated(variantAnalysis.id, repoState); - const credentials = await Credentials.initialize(this.ctx); + const credentials = await Credentials.initialize(); if (cancellationToken && cancellationToken.isCancellationRequested) { repoState.downloadStatus = @@ -577,7 +576,7 @@ export class VariantAnalysisManager ); } - const credentials = await Credentials.initialize(this.ctx); + const credentials = await Credentials.initialize(); void showAndLogInformationMessage( "Cancelling variant analysis. This may take a while.", diff --git a/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts b/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts index 267d2ec5b..cf9979c5e 100644 --- a/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts +++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts @@ -1,9 +1,4 @@ -import { - CancellationToken, - commands, - EventEmitter, - ExtensionContext, -} from "vscode"; +import { CancellationToken, commands, EventEmitter } from "vscode"; import { Credentials } from "../authentication"; import { getVariantAnalysis } from "./gh-api/gh-api-client"; @@ -32,7 +27,6 @@ export class VariantAnalysisMonitor extends DisposableObject { readonly onVariantAnalysisChange = this._onVariantAnalysisChange.event; constructor( - private readonly extensionContext: ExtensionContext, private readonly shouldCancelMonitor: ( variantAnalysisId: number, ) => Promise, @@ -44,7 +38,7 @@ export class VariantAnalysisMonitor extends DisposableObject { variantAnalysis: VariantAnalysis, cancellationToken: CancellationToken, ): Promise { - const credentials = await Credentials.initialize(this.extensionContext); + const credentials = await Credentials.initialize(); let attemptCount = 0; const scannedReposDownloaded: number[] = []; diff --git a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts index ca03e00b4..a5b42897d 100644 --- a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts @@ -61,10 +61,7 @@ describe("Variant Analysis Monitor", () => { "GitHub.vscode-codeql", )! .activate(); - variantAnalysisMonitor = new VariantAnalysisMonitor( - extension.ctx, - shouldCancelMonitor, - ); + variantAnalysisMonitor = new VariantAnalysisMonitor(shouldCancelMonitor); variantAnalysisMonitor.onVariantAnalysisChange(onVariantAnalysisChangeSpy); variantAnalysisManager = extension.variantAnalysisManager; diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/export-results.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/export-results.test.ts index 8e9a0100a..d11c41230 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/export-results.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/export-results.test.ts @@ -1,6 +1,5 @@ import { join } from "path"; import { readFile } from "fs-extra"; -import { createMockExtensionContext } from "../index"; import { Credentials } from "../../../authentication"; import * as markdownGenerator from "../../../remote-queries/remote-queries-markdown-generation"; import * as ghApiClient from "../../../remote-queries/gh-api/gh-api-client"; @@ -20,7 +19,6 @@ describe("export results", () => { .spyOn(ghApiClient, "createGist") .mockResolvedValue(undefined); - const ctx = createMockExtensionContext(); const query = JSON.parse( await readFile( join( @@ -41,7 +39,6 @@ describe("export results", () => { ); await exportRemoteQueryAnalysisResults( - ctx, "", query, analysesResults, diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/remote-query-history.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/remote-query-history.test.ts index 20a785575..e6bb15eb7 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/remote-query-history.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/remote-query-history.test.ts @@ -279,7 +279,6 @@ describe("Remote queries and query history manager", () => { jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials); arm = new AnalysesResultsManager( - {} as ExtensionContext, mockCliServer, join(STORAGE_DIR, "queries"), mockLogger, From abc025cb39a56fc1f7fa7d038755f7b5515f94da Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 21 Dec 2022 14:49:26 +0000 Subject: [PATCH 7/8] Inline the createOctokit method It's now only used from one place and inlining it doesn't make getOctokit too long to be unclear. --- extensions/ql-vscode/src/authentication.ts | 23 ++++++++++------------ 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index de8578b4a..9d14d904e 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -47,7 +47,16 @@ export class Credentials { return new Credentials(new Octokit.Octokit({ auth: overrideToken, retry })); } - private async createOctokit(): Promise { + /** + * Creates or returns an instance of Octokit. + * + * @returns An instance of Octokit. + */ + async getOctokit(): Promise { + if (this.octokit) { + return this.octokit; + } + const session = await vscode.authentication.getSession( GITHUB_AUTH_PROVIDER_ID, SCOPES, @@ -59,16 +68,4 @@ export class Credentials { retry, }); } - - /** - * Creates or returns an instance of Octokit. - * - * @returns An instance of Octokit. - */ - async getOctokit(): Promise { - if (this.octokit) { - return this.octokit; - } - return await this.createOctokit(); - } } From 6285ba7632f3aa57bfd61bf4fcc9c9f2be00544a Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 22 Dec 2022 10:25:34 +0000 Subject: [PATCH 8/8] fix typos --- extensions/ql-vscode/src/authentication.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index 9d14d904e..6abea03f1 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -27,7 +27,7 @@ export class Credentials { /** * Initializes a Credentials instance. This will generate octokit instances * authenticated as the user. If there is not already an authenticated GitHub - * session availabeT then the user will be prompted to log in. + * session available then the user will be prompted to log in. * * @returns An instance of credentials. */ @@ -37,8 +37,8 @@ export class Credentials { /** * Initializes an instance of credentials with an octokit instance using - * a specific known token. This method is meant to be used non-interactive - * environments such as tests. + * a specific known token. This method is meant to be used in + * non-interactive environments such as tests. * * @param overrideToken The GitHub token to use for authentication. * @returns An instance of credentials.