Merge pull request #2396 from github/koesie10/suppress-error-message

Suppress similar error messages when monitor MRVA

Author: koesie10

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

@@ -14,6 +14,7 @@ import { sleep } from "../pure/time";
 import { getErrorMessage } from "../pure/helpers-pure";
 import { showAndLogWarningMessage } from "../helpers";
 import { App } from "../common/app";
+import { extLogger } from "../common";
 
 export class VariantAnalysisMonitor extends DisposableObject {
   // With a sleep of 5 seconds, the maximum number of attempts takes
@@ -41,6 +42,8 @@ export class VariantAnalysisMonitor extends DisposableObject {
     let attemptCount = 0;
     const scannedReposDownloaded: number[] = [];
 
+    let lastErrorShown: string | undefined = undefined;
+
     while (attemptCount <= VariantAnalysisMonitor.maxAttemptCount) {
       await sleep(VariantAnalysisMonitor.sleepTime);
 
@@ -56,13 +59,22 @@ export class VariantAnalysisMonitor extends DisposableObject {
           variantAnalysis.id,
         );
       } catch (e) {
-        void showAndLogWarningMessage(
-          `Error while monitoring variant analysis ${
-            variantAnalysis.query.name
-          } (${variantAnalysis.query.language}) [${new Date(
-            variantAnalysis.executionStartTime,
-          ).toLocaleString(env.language)}]: ${getErrorMessage(e)}`,
-        );
+        const errorMessage = getErrorMessage(e);
+
+        const message = `Error while monitoring variant analysis ${
+          variantAnalysis.query.name
+        } (${variantAnalysis.query.language}) [${new Date(
+          variantAnalysis.executionStartTime,
+        ).toLocaleString(env.language)}]: ${errorMessage}`;
+
+        // If we have already shown this error to the user, don't show it again.
+        if (lastErrorShown === errorMessage) {
+          void extLogger.log(message);
+        } else {
+          void showAndLogWarningMessage(message);
+          lastErrorShown = errorMessage;
+        }
+
         continue;
       }
 
@@ -84,6 +96,9 @@ export class VariantAnalysisMonitor extends DisposableObject {
       }
 
       attemptCount++;
+
+      // Reset the last error shown if we have successfully retrieved the variant analysis.
+      lastErrorShown = undefined;
     }
   }
 

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

@@ -22,6 +22,7 @@ import {
 import { createMockVariantAnalysis } from "../../../factories/variant-analysis/shared/variant-analysis";
 import { createMockApp } from "../../../__mocks__/appMock";
 import { createMockCommandManager } from "../../../__mocks__/commandsMock";
+import * as helpers from "../../../../src/helpers";
 
 jest.setTimeout(60_000);
 
@@ -196,6 +197,93 @@ describe("Variant Analysis Monitor", () => {
       });
     });
 
+    describe("when some responses fail", () => {
+      let showAndLogWarningMessageSpy: jest.SpiedFunction<
+        typeof helpers.showAndLogWarningMessage
+      >;
+
+      let scannedRepos: ApiVariantAnalysisScannedRepository[];
+
+      beforeEach(async () => {
+        showAndLogWarningMessageSpy = jest
+          .spyOn(helpers, "showAndLogWarningMessage")
+          .mockResolvedValue(undefined);
+
+        scannedRepos = createMockScannedRepos([
+          "pending",
+          "in_progress",
+          "in_progress",
+          "in_progress",
+          "pending",
+          "pending",
+        ]);
+        mockApiResponse = createMockApiResponse("in_progress", scannedRepos);
+        mockGetVariantAnalysis.mockResolvedValueOnce(mockApiResponse);
+
+        mockGetVariantAnalysis.mockRejectedValueOnce(
+          new Error("No internet connection"),
+        );
+        mockGetVariantAnalysis.mockRejectedValueOnce(
+          new Error("No internet connection"),
+        );
+        mockGetVariantAnalysis.mockRejectedValueOnce(
+          new Error("My different error"),
+        );
+
+        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);
+
+        mockGetVariantAnalysis.mockRejectedValueOnce(
+          new Error("My different error"),
+        );
+        mockGetVariantAnalysis.mockRejectedValueOnce(
+          new Error("My different error"),
+        );
+        mockGetVariantAnalysis.mockRejectedValueOnce(
+          new Error("Another different error"),
+        );
+
+        nextApiResponse = {
+          ...mockApiResponse,
+          scanned_repositories: [...scannedRepos.map((r) => ({ ...r }))],
+        };
+        nextApiResponse.scanned_repositories[2].analysis_status = "succeeded";
+        nextApiResponse.scanned_repositories[3].analysis_status = "succeeded";
+        nextApiResponse.scanned_repositories[4].analysis_status = "failed";
+        nextApiResponse.scanned_repositories[5].analysis_status = "succeeded";
+        nextApiResponse.status = "succeeded";
+        mockGetVariantAnalysis.mockResolvedValueOnce(nextApiResponse);
+      });
+
+      it("should only trigger the warning once per error", async () => {
+        await variantAnalysisMonitor.monitorVariantAnalysis(variantAnalysis);
+
+        expect(showAndLogWarningMessageSpy).toBeCalledTimes(4);
+        expect(showAndLogWarningMessageSpy).toHaveBeenNthCalledWith(
+          1,
+          expect.stringMatching(/No internet connection/),
+        );
+        expect(showAndLogWarningMessageSpy).toHaveBeenNthCalledWith(
+          2,
+          expect.stringMatching(/My different error/),
+        );
+        expect(showAndLogWarningMessageSpy).toHaveBeenNthCalledWith(
+          3,
+          expect.stringMatching(/My different error/),
+        );
+        expect(showAndLogWarningMessageSpy).toHaveBeenNthCalledWith(
+          4,
+          expect.stringMatching(/Another different error/),
+        );
+      });
+    });
+
     describe("when there are no repos to scan", () => {
       beforeEach(async () => {
         scannedRepos = [];