Merge pull request #2667 from github/robertbrignull/scrubber_tests

Try to fix flakes in query history scrubber tests

Author: robertbrignull

extensions/ql-vscode/src/query-history/query-history-scrubber.ts

@@ -10,10 +10,6 @@ import { getErrorMessage } from "../common/helpers-pure";
 
 const LAST_SCRUB_TIME_KEY = "lastScrubTime";
 
-type Counter = {
-  increment: () => void;
-};
-
 /**
  * Registers an interval timer that will periodically check for queries old enought
  * to be deleted.
@@ -37,8 +33,8 @@ export function registerQueryHistoryScrubber(
   qhm: QueryHistoryManager,
   ctx: ExtensionContext,
 
-  // optional counter to keep track of how many times the scrubber has run
-  counter?: Counter,
+  // optional callback to keep track of how many times the scrubber has run
+  onScrubberRun?: () => void,
 ): Disposable {
   const deregister = setInterval(
     scrubQueries,
@@ -48,7 +44,7 @@ export function registerQueryHistoryScrubber(
     queryHistoryDirs,
     qhm,
     ctx,
-    counter,
+    onScrubberRun,
   );
 
   return {
@@ -64,7 +60,7 @@ async function scrubQueries(
   queryHistoryDirs: QueryHistoryDirs,
   qhm: QueryHistoryManager,
   ctx: ExtensionContext,
-  counter?: Counter,
+  onScrubberRun?: () => void,
 ) {
   const lastScrubTime = ctx.globalState.get<number>(LAST_SCRUB_TIME_KEY);
   const now = Date.now();
@@ -76,7 +72,7 @@ async function scrubQueries(
 
     let scrubCount = 0; // total number of directories deleted
     try {
-      counter?.increment();
+      onScrubberRun?.();
       void extLogger.log(
         "Cleaning up query history directories. Removing old entries.",
       );

extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-scrubber.test.ts

@@ -13,77 +13,61 @@ import {
   TWO_HOURS_IN_MS,
 } from "../../../../src/common/time";
 import { mockedObject } from "../../utils/mocking.helpers";
+import { DirResult } from "tmp";
 
-describe("query history scrubber", () => {
-  const now = Date.now();
+const now = Date.now();
+// We don't want our times to align exactly with the hour,
+// so we can better mimic real life
+const LESS_THAN_ONE_DAY = ONE_DAY_IN_MS - 1000;
 
+describe("query history scrubber", () => {
   let deregister: vscode.Disposable | undefined;
-  let mockCtx: vscode.ExtensionContext;
-  let runCount = 0;
-
-  // We don't want our times to align exactly with the hour,
-  // so we can better mimic real life
-  const LESS_THAN_ONE_DAY = ONE_DAY_IN_MS - 1000;
-  const tmpDir = dirSync({
-    unsafeCleanup: true,
-  });
+  let tmpDir: DirResult;
 
   beforeEach(() => {
+    tmpDir = dirSync({
+      unsafeCleanup: true,
+    });
+
     jest.spyOn(extLogger, "log").mockResolvedValue(undefined);
 
     jest.useFakeTimers({
       doNotFake: ["setTimeout"],
       now,
     });
-
-    mockCtx = {
-      globalState: {
-        lastScrubTime: now,
-        get(key: string) {
-          if (key !== "lastScrubTime") {
-            throw new Error(`Unexpected key: ${key}`);
-          }
-          return this.lastScrubTime;
-        },
-        async update(key: string, value: any) {
-          if (key !== "lastScrubTime") {
-            throw new Error(`Unexpected key: ${key}`);
-          }
-          this.lastScrubTime = value;
-        },
-      },
-    } as any as vscode.ExtensionContext;
   });
 
   afterEach(() => {
     if (deregister) {
       deregister.dispose();
       deregister = undefined;
     }
+    tmpDir.removeCallback();
   });
 
   it("should not throw an error when the query directory does not exist", async () => {
-    registerScrubber("idontexist");
+    const mockCtx = createMockContext();
+    const runCounter = registerScrubber("idontexist", mockCtx);
 
     jest.advanceTimersByTime(ONE_HOUR_IN_MS);
     await wait();
     // "Should not have called the scrubber"
-    expect(runCount).toBe(0);
+    expect(runCounter).toHaveBeenCalledTimes(0);
 
     jest.advanceTimersByTime(ONE_HOUR_IN_MS - 1);
     await wait();
     // "Should not have called the scrubber"
-    expect(runCount).toBe(0);
+    expect(runCounter).toHaveBeenCalledTimes(0);
 
     jest.advanceTimersByTime(1);
     await wait();
     // "Should have called the scrubber once"
-    expect(runCount).toBe(1);
+    expect(runCounter).toHaveBeenCalledTimes(1);
 
     jest.advanceTimersByTime(TWO_HOURS_IN_MS);
     await wait();
     // "Should have called the scrubber a second time"
-    expect(runCount).toBe(2);
+    expect(runCounter).toHaveBeenCalledTimes(2);
 
     expect((mockCtx.globalState as any).lastScrubTime).toBe(
       now + TWO_HOURS_IN_MS * 2,
@@ -97,7 +81,7 @@ describe("query history scrubber", () => {
       TWO_HOURS_IN_MS,
       THREE_HOURS_IN_MS,
     );
-    registerScrubber(queryDir);
+    registerScrubber(queryDir, createMockContext());
 
     jest.advanceTimersByTime(TWO_HOURS_IN_MS);
     await wait();
@@ -176,7 +160,31 @@ describe("query history scrubber", () => {
     return `query-${timestamp}`;
   }
 
-  function registerScrubber(dir: string) {
+  function createMockContext(): vscode.ExtensionContext {
+    return {
+      globalState: {
+        lastScrubTime: now,
+        get(key: string) {
+          if (key !== "lastScrubTime") {
+            throw new Error(`Unexpected key: ${key}`);
+          }
+          return this.lastScrubTime;
+        },
+        async update(key: string, value: any) {
+          if (key !== "lastScrubTime") {
+            throw new Error(`Unexpected key: ${key}`);
+          }
+          this.lastScrubTime = value;
+        },
+      },
+    } as any as vscode.ExtensionContext;
+  }
+
+  function registerScrubber(
+    dir: string,
+    ctx: vscode.ExtensionContext,
+  ): jest.Mock {
+    const onScrubberRun = jest.fn();
     deregister = registerQueryHistoryScrubber(
       ONE_HOUR_IN_MS,
       TWO_HOURS_IN_MS,
@@ -187,11 +195,10 @@ describe("query history scrubber", () => {
           return Promise.resolve();
         },
       }),
-      mockCtx,
-      {
-        increment: () => runCount++,
-      },
+      ctx,
+      onScrubberRun,
     );
+    return onScrubberRun;
   }
 
   async function wait(ms = 500) {