Try to fix flakes in query history scrubber tests

[robertbrignull]

I believe that these changes might help with the flakes in the query history scrubber tests.

Most of the test failures that I've seen look like:

FAIL query-history/query-history-scrubber.test.ts (5.957 s)
  ● query history scrubber › should not throw an error when the query directory does not exist

    expect(received).toBe(expected) // Object.is equality

    Expected: 0
    Received: 16

      69 |     await wait();
      70 |     // "Should not have called the scrubber"
    > 71 |     expect(runCount).toBe(0);
         |                      ^
      72 |
      73 |     jest.advanceTimersByTime(ONE_HOUR_IN_MS - 1);
      74 |     await wait();

So this means when the test started the runCount variable wasn't set to zero! 🤯

I believe this is because individual test run is receiving a reference to the same runCount variable, because it's defined in the describe block, and the variable isn't reset between tests because the = 0 only applies when the variable is first defined. The fix is to avoid reusing the variable, or to reset the variable between tests. In this PR I opt to use a different variable for each test, and also to use a jest.fn() so we can use toHaveBeenCalledTimes.

I also try to clean up the tests in other ways so it matches other tests and it's clear we won't get any other inter-test dependencies that'll cause other flakes. Each change is in a separate commit.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[aeisenberg]

Nice. This makes a lot of sense. Thanks for improving. Hopefully, this will make tests more stable.

[aeisenberg]

Wait...this was never reset? How did this ever work?

[robertbrignull]

I confess I don't really understand the semantics of jest. I think I went a bit far there saying it's never reset between tests and you make a good point that how would it ever work, so it must have been reset most/some of the time.

[robertbrignull]

Ok from a bit of research it appears I was right and variables like this are not reset between tests. So the reason it worked is that only the first of the two tests in this file references this variable, and tests within a file are run sequentially in order (at least by default).

The reason it was failing like this is that we have jest configured to retry failed tests, and on the second try it would fail because runCount wasn't reset. The full log, which I'm embarrassed I didn't read more fully earlier on, is:

PASS test/vscode-tests/no-workspace/query-history/variant-analysis-history.test.ts
 LOGGING RETRY ERRORS  query history scrubber should not throw an error when the query directory does not exist
 RETRY 1 

    TreeError [codeQLQueryHistory] Data tree node not found: [object Object]

      at we.A (../../../vscode-file:/vscode-app/home/runner/work/vscode-codeql/vscode-codeql/extensions/ql-vscode/.vscode-test/vscode-linux-x64-1.77.3/resources/app/out/vs/workbench/workbench.desktop.main.js:554:18061)
      at we.reveal (../../../vscode-file:/vscode-app/home/runner/work/vscode-codeql/vscode-codeql/extensions/ql-vscode/.vscode-test/vscode-linux-x64-1.77.3/resources/app/out/vs/workbench/workbench.desktop.main.js:554:17723)
      at Me.reveal (../../../vscode-file:/vscode-app/home/runner/work/vscode-codeql/vscode-codeql/extensions/ql-vscode/.vscode-test/vscode-linux-x64-1.77.3/resources/app/out/vs/workbench/workbench.desktop.main.js:2526:13663)
      at g.j (../../../vscode-file:/vscode-app/home/runner/work/vscode-codeql/vscode-codeql/extensions/ql-vscode/.vscode-test/vscode-linux-x64-1.77.3/resources/app/out/vs/workbench/workbench.desktop.main.js:1586:70284)

 RETRY 2 

    expect(received).toBe(expected) // Object.is equality

    Expected: 0
    Received: 16

      69 |     await wait();
      70 |     // "Should not have called the scrubber"
    > 71 |     expect(runCount).toBe(0);
         |                      ^
      72 |
      73 |     jest.advanceTimersByTime(ONE_HOUR_IN_MS - 1);
      74 |     await wait();

      at Object.<anonymous> (query-history/query-history-scrubber.test.ts:71:22)

So you can see the first failure had a different reason, and then the subsequent failures were because of runCount.

So with this in mind I'm pretty confident this PR will at least make the test behave consistently on retries. It appears we may have another reason for test failure, but if that's not consistent then it may be handled by the automatic retries.