Implement submitting a live-results variant analysis by robertbrignull · Pull Request #1538 · github/vscode-codeql

robertbrignull · 2022-09-22T16:39:09Z

Implements submitting a variant analysis to the new live results endpoint when the live results config is set. Doesn't yet implement the full notification when the analysis is submitted and doesn't implement any monitoring, so currently the variant analysis ID is printed to the logs but I expect that to change in the future.

Also adds a dedicated getControllerRepoId method before we get into anything else. This is not strictly necessary but it lets us give better error messages and means that later on we can sure that the controller repo exists.

Finally, also adds some tests that have the live results config set. I've copied the existing tests by adding all four cases of different query packs. Not sure it's strictly needed to have all four in both cases but I've gone with consistency.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
- Not necessary. Not yet a visible change.
Issues have been created for any UI or other user-facing changes made by this pull request.
- No user-facing changes
[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.

koesie10 · 2022-09-23T07:40:32Z

+
+    // Consider live results disabled unless specifically enabled in a test
+    await setVariantAnalysisLiveResultsEnabled(false);


We are now setting this value before each test, but that means that the value after this test file has run is that the value might be set to true depending on the order of tests. Since other test files will execute in the same VSCode instance, that might result in some different behavior for those tests as well.

I think it would be better to move this to the afterEach hook to reset the value after every test; that will ensure that the value will be false for certain after the test file has completed.

Thanks for bringing this up. So values like this aren't reset between tests? I wasn't sure so I added this before each test, but was hoping to clarify before merging the PR.

Unless it's too expensive (which I doubt) should we do it before and after, so we don't rely on other tests leaving a consistent state? I note we don't go to those lengths for the other config settings, but that's a different issue and doesn't mean we can't do something else.

What I'd like even more is a resetConfig method that would set all of the config to a consistent state. Then every test could call that in its beforeEach method and not have to worry about any leftover config changes. If you think that's a good idea I can open an issue to look at it.

I don't think the config is reset between each test, the only integration between VSCode and the test runner is the script in extensions/ql-vscode/src/vscode-tests/index-template.ts which doesn't seem to reset the config.

I think it would indeed be a good idea to have a resetConfig function which will reset all config values. We would then be able to run this before every test and have a consistent state without needing to reset the state before/after tests.

koesie10 · 2022-09-23T07:43:44Z

+
+async function getControllerRepoId(credentials: Credentials, owner: string, repo: string): Promise<number> {
+  try {
+    return await ghApiClient.getRepositoryIdFromNwo(credentials, owner, repo);


Would it make sense to drop the owner and repo parameters from this method and instead call getRemoteControllerRepo in this method? The only thing that now differentiates this from the getRepositoryIdFromNwo method is the way error messages are handled; it doesn't actually just get the controller repo ID, you can pass any arbitrary owner/repo combination into it.

I think that would be a good way of cleaning this code up by putting more implementation into a method. Unfortunately it's not quite as clean as it could be, mostly because we use the owner and repo fields later on in runRemoteQuery so we have to pass those back as well. But a nice way of doing that could be changing to passing around a Repository object instead of just the ID or owner or repo name separately. I've had a go at this and I think it's worked out quite nicely. It's slightly more code changes than strictly necessary but I think it's all going in the right direction.

koesie10 · 2022-09-23T07:45:37Z

+          name: queryName,
+          filePath: queryFile,
+          pack: base64Pack,
+          language: language as VariantAnalysisQueryLanguage,


Since this is being cast to the VariantAnalysisQueryLanguage, should we validate that the language is a supported MRVA language?

I'm not sure. On the one hand it's nice to have accurate types, but then it's seems weird to do the type checking here. We are storing the language as a string elsewhere in the extension, so shouldn't we be applying this type earlier on (I realise we've only just introduced the type which is why it's not used elsewhere).

On the other hand the API itself does validation that the language is from the known set of languages, and returns an error message. So just passing the string to the API and letting it decide if the language is ok or not also seems like a good option to me.

Another question is are we going to ever do anything in the VSCode extension that depends on the exact language? I can't think of many cases. We'll generally either just display the language or pass it to other places like the CodeQL CLI or the github API. We might want to display a better formatted name (i.e. "JavaScript" instead of "javascript") but that's about all.

So my preference would actually be to delete the VariantAnalysisQueryLanguage type and just treat the language as a string. We could also keep the type but make it an alias of string. I just don't know if that's too radical of a decision to make.

What do you think?

I'm changing my mind. For this PR I'll just add a method to check if the language is valid or not, and we'll throw an exception if it's not. I'll leave the discussion of whether we want this type of not for a later time.

koesie10 · 2022-09-23T07:48:44Z

  });

+  it('should run a variant analysis that is part of a qlpack', async () => {
+    await setVariantAnalysisLiveResultsEnabled(true);


Perhaps we can move this to a nested describe block which has this as a beforeEach hook? Something like:

describe('when live results are enabled', () => { beforeEach(async () => { await setVariantAnalysisLiveResultsEnabled(true); }); it('should run a variant analysis that is part of a qlpack', async () => { // ... }); // ... });

Ooh, that's a good idea. I wasn't aware you could do nested describes with their own before/after hooks.

This has increased the total number of lines changed quite a lot, but if you enable the mode to ignore whitespace then it'll look a lot more manageable.

…rollerRepo This involved changing a few different methods to take a Repository object instead of taking owner and repo separately. Overall I think this is a good change.

robertbrignull · 2022-09-23T12:07:31Z

+import { expect } from 'chai';
+import { parseVariantAnalysisQueryLanguage, VariantAnalysisQueryLanguage } from '../../src/remote-queries/shared/variant-analysis';
+
+describe('parseVariantAnalysisQueryLanguage', () => {


Maybe this function is too simple to require / benefit from a test. I'm happy to remove it if that would be simpler.

I also wasn't sure where to put it. The test is completely pure in that it doesn't need a workspace, and I have found other examples of tests in the test/pure-tests directory that test things from src/remove-queries. But I could move it back to src/vscode-tests/no-workspace/remote-queries/shared/ if that would be a better location for it.

I'm also not quite sure how the distinction between those tests was originally determined. However, I do think we should always prefer the test directory over vscode-tests; the former is much quicker to run and less flaky. We could also consider placing it in a directory like test/remote-queries and adding that to the test:unit package.json script.

I think I'll leave it here for now unless you have strong thoughts on this. Moving it in the future will be very easy.

robertbrignull · 2022-09-23T13:49:40Z

I've hit barriers which have necessitated a few more changes. The main one is that I've removed the setVariantAnalysisLiveResultsEnabled method and instead am using sandbox.stub(config, 'isVariantAnalysisLiveResultsEnabled') to control whether live results is enabled.

The reason here is that to set a value in this way, VSCode appears to require that the config field is registered in a defined list of properties and doing this would make the liveResults configuration option appear as an autocomplete option and in the generated extension settings in the UI. It's my understanding that we don't want to do this and we want to hide this setting from anybody who isn't reading our code or pull requests. Using a stub is also the way that we control isCanary in the tests.

robertbrignull · 2022-09-23T15:04:38Z

Hmm, the new tests are timing out but I can't see why it's happening. I can't see anything obviously wrong or different. I can use the new code paths manually and they all seem to function, so perhaps the issue is in the tests. I just can't see where 😦

  1) Remote queries
       when live results are enabled
         should run a variant analysis that is part of a qlpack:
     Error: Timeout of 180000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/vscode-codeql/vscode-codeql/extensions/ql-vscode/out/vscode-tests/cli-integration/remote-queries/run-remote-query.test.js)
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7)

robertbrignull · 2022-09-26T15:16:46Z

All tests successful 🥳

robertbrignull added 3 commits September 22, 2022 11:35

Add getControllerRepoId method

7d721d9

Implement submitting a variant analysis

52f993f

Add tests for runRemoteQuery when in live results mode

7874a34

robertbrignull added the secexp label Sep 22, 2022

robertbrignull requested review from a team as code owners September 22, 2022 16:39

koesie10 reviewed Sep 23, 2022

View reviewed changes

robertbrignull added 7 commits September 23, 2022 10:41

Set live results mode to off both before and after tests

847082c

Move more implementation of fetching the controller repo into getCont…

50aaf3b

…rollerRepo This involved changing a few different methods to take a Repository object instead of taking owner and repo separately. Overall I think this is a good change.

Use separate describe blocks for live results enabled vs disabled

24c2663

Add parseVariantAnalysisQueryLanguage method

7538ad1

Fix tests that use parseResponse

91344a7

Add tests of parseVariantAnalysisQueryLanguage

8086294

Move test to pure-tests directory

4d33858

robertbrignull commented Sep 23, 2022

View reviewed changes

robertbrignull added 3 commits September 23, 2022 14:11

Remove settings duplicated in beforeEach

0e23dd5

Stub getRepositoryFromNwoStub

f47d6ec

Stub isVariantAnalysisLiveResultsEnabled instead of updating setting

910c1b7

robertbrignull added 2 commits September 23, 2022 15:41

Remove unnecessary async

abf6c6f

Move duplicate definition to constant

9b2c40b

elenatanasoiu reviewed Sep 23, 2022

View reviewed changes

Comment thread extensions/ql-vscode/src/remote-queries/run-remote-query.ts Outdated

robertbrignull added 3 commits September 23, 2022 17:25

Fix typo

47e53da

Don't wait for dialog box

519c303

Promise resolves successfully but result is undefined

5dce5e8

elenatanasoiu reviewed Sep 27, 2022

View reviewed changes

Comment thread extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/run-remote-query.test.ts Outdated

Return a VariantAnalysis from runRemoteQuery

ce7c711

elenatanasoiu approved these changes Sep 27, 2022

View reviewed changes

robertbrignull merged commit 0b638b6 into main Sep 27, 2022

robertbrignull deleted the robertbrignull/submit-variant-analysis branch September 27, 2022 09:36

elenatanasoiu mentioned this pull request Sep 28, 2022

Implement monitoring for variant analysis live results #1545

Merged

3 tasks


		// Consider live results disabled unless specifically enabled in a test
		await setVariantAnalysisLiveResultsEnabled(false);

Conversation

robertbrignull commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

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!

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!

robertbrignull commented Sep 23, 2022

Uh oh!

robertbrignull commented Sep 23, 2022

Uh oh!

Uh oh!

robertbrignull commented Sep 26, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robertbrignull commented Sep 22, 2022 •

edited

Loading