Add unit tests for query history and remote queries by aeisenberg · Pull Request #1166 · github/vscode-codeql

aeisenberg · 2022-02-24T02:53:21Z

Adds some tests for reading in the history and manipulating.
There are some more tests to come later. Maybe in another PR, maybe in
this one.

Note that this PR uses a new node 16 API String.prototype.replaceAll.
I think this is ok since vscode ships with node 16. If this causes
problems, I can separate to a different PR and we can discuss there.

Checklist

[n/a] 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.

charisk

Thanks for adding some much needed tests!

charisk · 2022-02-24T08:59:43Z

    this.analysesResults.set(queryId, resultsForQuery);
-    void publishResults(resultsForQuery);
+    void publishResults([...resultsForQuery]);
+    const pos = resultsForQuery.length - 1;


Can we rename pos to something more descriptive for the function? e.g. analysisPosition?

Also, just a thought (that should be ignored for this PR!): we could have a better data structure for the query results that deals with stuff like that (replacing an analysis) instead of having to do it here. It'd then make the code a bit more easy to manage.

I've always like immutable.js for things like this, but I don't like introducing it into an ongoing project. Maybe it's not such a bad idea in this case.

We do need to be careful. Now that we are saving data across restarts, any change in format will cause error when there is an upgrade. It's not a terrible situation since the worst case is that we need to delete some old queries on upgrade.

I should add version number to each saved artifact and the extension can read this and upgrade it appropriately when we change versions.

I'd be up for using immutable.js or any other way to enforce immutability.

Agree about versions - we'll need to be careful. Perhaps the overall query history store has a version? We can always deal with it the first time we need a new version I think.

charisk · 2022-02-24T09:10:13Z

+  let qhm: QueryHistoryManager;
+  let rawQueryHistory: any;
+  let remoteQueryResult0: RemoteQueryResult;
+  // let remoteQueryResult1: RemoteQueryResult;


Should this be removed?

charisk · 2022-02-24T09:11:12Z

+    disposables = new DisposableBucket();
+    rawQueryHistory = fs.readJSONSync(path.join(STORAGE_DIR, 'workspace-query-history.json'));
+    remoteQueryResult0 = fs.readJSONSync(path.join(STORAGE_DIR, 'queries', rawQueryHistory[0].queryId, 'query-result.json'));
+    // remoteQueryResult1 = fs.readJSONSync(path.join(STORAGE_DIR, 'queries', rawQueryHistory[1].queryId, 'query-result.json'));


Should this be removed?

I'll be writing more tests and will likely need this variable.

Shall we add it then? Happy either way.

charisk · 2022-02-24T09:22:06Z

+    fs.removeSync(STORAGE_DIR);
+  }
+
+  function* walk(dir: string): IterableIterator<string> {


Nice! At first I was thinking it might be an overkill to have an a generator for tests, but there's no harm in having it and puts us in a good position if we want to test a lot/big files.

I should probably move this to a shared location since it might be useful in other places.

aeisenberg · 2022-02-24T16:20:21Z

Putting this into draft mode so I can work on it more.

aeisenberg · 2022-02-25T00:45:12Z

    "@types/mocha": "^9.0.0",
    "@types/nanoid": "^3.0.0",
-    "@types/node": "^12.14.1",
+    "@types/node": "^16.11.25",


Need to update to v16 to get the replaceAll method.

aeisenberg · 2022-02-25T00:45:17Z

      if (cancellationToken !== undefined) {
        cancellationRegistration = cancellationToken.onCancellationRequested(_e => {
-          tk(child.pid);
+          tk(child.pid || 0);


In v16, pid is potentially undefined.

aeisenberg · 2022-02-25T00:45:45Z

+      if (finalSingleItem.status === QueryStatus.Completed) {
+        await this._onWillOpenQueryItem.fire(finalSingleItem);
+      }


Fix small bug to avoid an error when someone clicks on an in progress item.

aeisenberg · 2022-02-25T01:06:45Z

@@ -1,11 +0,0 @@
-/**


With node 16, this is no longer needed.

aeisenberg · 2022-02-25T01:07:28Z

+ * A simple disposable object that does nothing other than contain a list of disposable objects.
+ * This is useful for implementing a `Disposable` that owns other disposable objects.
+ */
+export class DisposableBucket extends DisposableObject {


Used for tests.

Shall we move this to the tests dir?

Adds some tests for reading in the history and manipulating. There are some more tests to come later. Maybe in another PR, maybe in this one. Note that this PR uses a new node 16 API String.prototype.replaceAll. I think this is ok since vscode ships with node 16. If this causes problems, I can separate to a different PR and we can discuss there.

aeisenberg · 2022-02-25T14:59:45Z

@charisk can you take another look? I've made a lot of changes.

charisk · 2022-02-25T15:27:52Z

+ * A simple disposable object that does nothing other than contain a list of disposable objects.
+ * This is useful for implementing a `Disposable` that owns other disposable objects.
+ */
+export class DisposableBucket extends DisposableObject {


Shall we move this to the tests dir?

charisk · 2022-02-25T15:29:51Z

+      "fileSizeInBytes": 81237,
+      "downloadLink": {
+        "id": "171544171",
+        "urlPath": "/repos/dsp-testing/qc-run2/actions/artifacts/171544171",


Perhaps shouldn't use dsp-testing testing?

I can change the repo.

charisk · 2022-02-25T15:33:00Z

+    expect(qhm.treeDataProvider.allHistory.length).to.eq(2);
+  });
+
+  it('should remove and then a query from history', async () => {


Suggested change

it('should remove and then a query from history', async () => {

it('should remove and then add query from history', async () => {

charisk · 2022-02-25T15:36:26Z

+      );
+    });
+
+    it('should avoid downloading an analysis result', async () => {


Suggested change

it('should avoid downloading an analysis result', async () => {

it('should avoid re-downloading an analysis result', async () => {

charisk · 2022-02-25T15:44:49Z

+    });
+  });
+
+  // Since this test changes the state of the query history manager, we need to copy the original


This comment doesn't look like it belongs here exactly - should it be near the test?

charisk · 2022-02-25T15:47:39Z

+      const result1 = arm.getAnalysesResults(rawQueryHistory[1].queryId);
+
+      // Shoule be equal, but not equivalent
+      expect(result0).to.deep.eq((arm as any).analysesResults.get(rawQueryHistory[0].queryId));


This looks like it's interrogating the private state of the AnalysisResultsManager which doesn't feel right.

Should we not instead check that what we get back from getAnalysesResults is what we "downloaded"?

I'm specifically testing here that the results are an equivalent copy of what is stored by the AnalysesResultsManager. Instead of inspecting private state, I could call arm.getAnalysesResults twice and ensure that the objects returned are equivalent but different.

Ah I see, I think that would make more sense.

I added these tests because keeping them the same instance created weird race conditions, so just wanted to make sure we don't do this again.

Another reason to use immutable.

charisk · 2022-02-25T15:48:39Z

@@ -0,0 +1,21 @@
+import * as path from 'path';


Can I make a suggestion that we start using more specific util modules? e.g. file-utils.ts rather than test-helpers which could be a bucket for pretty much anything.

charisk

Approving since all the comments are actually small suggestions!

- More explicit test helper module names - Fix unit test names - Better sanitization of repo names in tests

aeisenberg · 2022-02-25T17:58:30Z

All comments addressed.

aeisenberg requested review from a team as code owners February 24, 2022 02:53

charisk approved these changes Feb 24, 2022

View reviewed changes

Base automatically changed from aeisenberg/avoid-download to main February 24, 2022 16:11

aeisenberg marked this pull request as draft February 24, 2022 16:19

aeisenberg force-pushed the aeisenberg/remote-queries-unit-tests branch 3 times, most recently from b3fa7f6 to 9792a58 Compare February 25, 2022 00:18

aeisenberg marked this pull request as ready for review February 25, 2022 00:20

aeisenberg force-pushed the aeisenberg/remote-queries-unit-tests branch 2 times, most recently from 7c1dff9 to 76d6dfa Compare February 25, 2022 00:43

aeisenberg changed the base branch from main to aeisenberg/query-history-version February 25, 2022 00:46

aeisenberg commented Feb 25, 2022

View reviewed changes

aeisenberg force-pushed the aeisenberg/remote-queries-unit-tests branch from ee99427 to 9d17703 Compare February 25, 2022 02:10

aeisenberg force-pushed the aeisenberg/remote-queries-unit-tests branch from 9d17703 to 4a928f1 Compare February 25, 2022 07:10

aeisenberg requested a review from charisk February 25, 2022 14:59

charisk reviewed Feb 25, 2022

View reviewed changes

Base automatically changed from aeisenberg/query-history-version to main February 25, 2022 16:11

charisk approved these changes Feb 25, 2022

View reviewed changes

Small cleanups around remote-queries tests

791e7e9

- More explicit test helper module names - Fix unit test names - Better sanitization of repo names in tests

aeisenberg enabled auto-merge February 25, 2022 17:58

aeisenberg merged commit 94a311a into main Feb 25, 2022

aeisenberg deleted the aeisenberg/remote-queries-unit-tests branch February 25, 2022 18:07

charisk mentioned this pull request Mar 1, 2022

Update ECMAScript version to be the same across the codebase #1177

Merged

3 tasks

	it('should remove and then a query from history', async () => {
	it('should remove and then add query from history', async () => {

	it('should avoid downloading an analysis result', async () => {
	it('should avoid re-downloading an analysis result', async () => {

Conversation

aeisenberg commented Feb 24, 2022

Checklist

Uh oh!

charisk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Feb 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Feb 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charisk left a comment

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aeisenberg Feb 25, 2022 •

edited

Loading