When rehydrating, always trigger a monitoring command unless the variant analysis is fully complete by robertbrignull · Pull Request #1672 · github/vscode-codeql

robertbrignull · 2022-10-27T16:18:48Z

The point of this PR is to trigger a monitoring command more often when we rehydrate a variant analysis. This should help with handling cases like when the analysis is complete but not all results are downloaded.

My end goal here is that it's always safe to just fire off a monitoring command and it'll do the right thing and get the variant analysis moving. This PR doesn't cover making the monitoring command handle all possible states perfectly, but I think in the currently implementation it won't be anything bad, so I think it's ok to consider this PR on its own and I'll look at opening a separate PR to make the monitoring command do the right thing in all cases.

This is the largest piece of code I've written in the extension on my own to date, and the tests are a bit complicated and I'm not sure I've done them correctly. Please don't hold back and do help me to do things properly.

Questions / concerns I have right now are:

I haven't run the tests locally yet, because it's the end of the day and I'm not sure I've ever run the CLI tests locally. If they all fail I'll look that them tomorrow.
I made isVariantAnalysisRepoDownloaded public. This seems fine to me but please let me know if not. Because I made it public I added some simple tests of it. Was this the right thing to do?
I don't like that isVariantAnalysisComplete is public but it seemed the best solution for testing, so for the tests of rehydrating we can stub isVariantAnalysisComplete and then have separate simpler tests of just that method. Otherwise there are a lot of different cases to cover and it didn't seem sensible doing the whole rehydrating in all of them.
It feels like isVariantAnalysisComplete should not be in VariantAnalysisManager, but I'm not sure where to put it otherwise and also not sure it can be pulled out because it depends on the results manager.
In the tests I've nested my describes very heavily and tried to test only one thing at a time. Is this good style or not helpful?

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.

…ysis is not complete

koesie10 · 2022-10-28T09:23:25Z

+  public async isVariantAnalysisComplete(variantAnalysis: VariantAnalysis): Promise<boolean> {
+    // It's only acceptable to have no scanned repos if the variant analysis is not in a final state.
+    // Otherwise it means the analysis hit some kind of internal error or there were no repos to scan.
+    if (variantAnalysis.scannedRepos === undefined || variantAnalysis.scannedRepos.length === 0) {
+      return variantAnalysis.status !== VariantAnalysisStatus.InProgress;
+    }
+
+    return (await Promise.all(variantAnalysis.scannedRepos.map(repo => this.isVariantAnalysisRepoComplete(variantAnalysis.id, repo)))).every(x => x);
+  }
+
+  private async isVariantAnalysisRepoComplete(variantAnalysisId: number, repo: VariantAnalysisScannedRepository): Promise<boolean> {
+    if (!hasRepoScanCompleted(repo)) {
+      return false;
+    } else if (!repoScanHasResults(repo)) {
+      return true;
+    } else {
+      const storageLocation = this.getVariantAnalysisStorageLocation(variantAnalysisId);
+      return await this.variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(storageLocation, repo.repository.fullName);
+    }
+  }


Since this doesn't depend on any instance variables, I think you can extract this to a function which is outside of the class. We could have a simple function like isVariantAnalysisComplete(variantAnalysisResultsManager: VariantAnalysisResultsManager, variantAnalysisStorageLocation: string, variantAnalysis: VariantAnalysis) which can be called from anywhere. This would also solve the problem of this being unnecessarily public since we can just export it and not think about its visibility.

Agree with @koesie10 - this function can be extracted and unit tested/mocked independently.

If we want to avoid passing in the VariantAnalysisResultsManager, we could instead pass in a function that helps decide whether a repo has downloads.

Thanks for the advice. That makes sense to me, to extract the method and pass in a function that checks whether the repo has downloads or not. This way we can keep the logic pure and contained to variant-analysis.ts (unless you have suggestions for somewhere better to put it). And we can test the logic of this method and the logic of rehydrating separately.

koesie10 · 2022-10-28T09:25:27Z

   * should be deleted.
   */
-  private async prepareStorageDirectory(variantAnalysisId: number): Promise<void> {
+  public async prepareStorageDirectory(variantAnalysisId: number): Promise<void> {


I don't really think this should be public since it's quite specific to the manager. I'm not sure how we can improve this though, but since this is public to be used in tests I think we can just copy the code to the tests.

If you think it's better to keep this method private but duplicate the implementation in the tests, I'm happy to go with that approach.

… manager

robertbrignull · 2022-10-31T11:57:21Z

Had to do a slightly complex merge resolution. The main conflicts were in extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts.

I also ended up doing fef3159 because once I'd merge main I wasn't able to commit the changes otherwise.

I was getting

> git commit
husky > pre-commit (node v16.17.0)

> vscode-codeql@1.7.4 format-staged
> lint-staged

✔ Preparing...
⚠ Running tasks...
  ✔ Running tasks for ./**/*.{json,css,scss}
  ❯ Running tasks for ./**/*.{ts,tsx}
    ✔ tsfmt -r
    ✖ eslint --fix [FAILED]
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up...

✖ eslint --fix:

/Users/robertbrignull/github/vscode-codeql/extensions/ql-vscode/scripts/fix-scenario-file-numbering.ts
  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: scripts/fix-scenario-file-numbering.ts.
The file must be included in at least one of the projects provided

✖ 1 problem (1 error, 0 warnings)

husky > pre-commit hook failed (add --no-verify to bypass)

robertbrignull · 2022-10-31T12:00:15Z

I also ended up doing fef3159 because once I'd merge main I wasn't able to commit the changes otherwise.

As discussed on slack, I'll not include this change in this PR. We'll address it separately.

koesie10

LGTM, thanks for making these changes!

I believe the failing tests are due to missing an await on the isVariantAnalysisComplete calls in extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts.

The method no longer accepts a second argument

robertbrignull · 2022-10-31T15:51:21Z

Ok. I've now run all three test suites locally and they all pass. That had better be it 😦

robertbrignull · 2022-10-31T16:26:07Z

Yay I think this is good to go, though I will wait until tomorrow because it may make sense to merge #1687 first otherwise we risk being unable to load the extension cleanly for all MRVA developers.

robertbrignull added 4 commits October 27, 2022 16:58

Rename variantAnalysis to be more specific

c4b1134

Move test fixtures earlier in file

51589e9

Make isVariantAnalysisRepoDownloaded public

caeaba2

When rehydrating, always trigger a monitoring command if variant anal…

599a9ed

…ysis is not complete

robertbrignull added the secexp label Oct 27, 2022

robertbrignull requested review from a team as code owners October 27, 2022 16:18

koesie10 reviewed Oct 28, 2022

View reviewed changes

robertbrignull added 6 commits October 31, 2022 10:24

Stop checking result count and rename repoScanHasResults

7748f82

Convert to using sinon spies

7e59d4c

Move isVariantAnalysisComplete implementation out of variant analysis…

b53366f

… manager

make public prepareStorageDirectory private

b497c4f

Check directory exists before deleting

b751cee

Merge branch 'main' into robertbrignull/always_trigger_monitoring

c36fa0f

robertbrignull force-pushed the robertbrignull/always_trigger_monitoring branch from 01e3fca to c36fa0f Compare October 31, 2022 12:00

koesie10 approved these changes Oct 31, 2022

View reviewed changes

robertbrignull added 4 commits October 31, 2022 14:29

Add awaits

c9038f5

Fix unit test expectated results 🤦

6dc684f

Fix rehydrateVariantAnalysis integration tests

12d5255

The method no longer accepts a second argument

Merge branch 'main' into robertbrignull/always_trigger_monitoring

952f033

robertbrignull merged commit ae31a17 into main Nov 1, 2022

robertbrignull deleted the robertbrignull/always_trigger_monitoring branch November 1, 2022 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When rehydrating, always trigger a monitoring command unless the variant analysis is fully complete#1672

When rehydrating, always trigger a monitoring command unless the variant analysis is fully complete#1672
robertbrignull merged 14 commits intomainfrom
robertbrignull/always_trigger_monitoring

robertbrignull commented Oct 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

koesie10 Oct 28, 2022

Uh oh!

charisk Oct 31, 2022

Uh oh!

robertbrignull Oct 31, 2022

Uh oh!

koesie10 Oct 28, 2022

Uh oh!

robertbrignull Oct 31, 2022

Uh oh!

robertbrignull commented Oct 31, 2022

Uh oh!

robertbrignull commented Oct 31, 2022

Uh oh!

koesie10 left a comment

Uh oh!

robertbrignull commented Oct 31, 2022

Uh oh!

robertbrignull commented Oct 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

robertbrignull commented Oct 27, 2022

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

koesie10 Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

charisk Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

robertbrignull Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

koesie10 Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

robertbrignull Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

robertbrignull commented Oct 31, 2022

Uh oh!

robertbrignull commented Oct 31, 2022

Uh oh!

koesie10 left a comment

Choose a reason for hiding this comment

Uh oh!

robertbrignull commented Oct 31, 2022

Uh oh!

robertbrignull commented Oct 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants