Move variant analysis tests out of cli-integration directory #1628

elenatanasoiu · 2022-10-19T10:27:57Z

We've added some tests to the cli-integration folder which don't require
the CLI to be run successfully. Let's move them in the no-workspace
folder.

The three files are:

cli-integration/remote-queries/variant-analysis-manager.test.ts
cli-integration/remote-queries/variant-analysis-monitor.test.ts
cli-integration/remote-queries/variant-analysis-results-manager.test

We're also moving the variant-analysis-processor.test.ts file into
tests/pure-tests/remote-queries as the processor contains helper methods
and doesn't depend on the CLI.

While we're here, we're also documenting what the cli-integration tests are for
since the name is confusing.

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.

elenatanasoiu · 2022-10-19T10:36:46Z

The CI failure is not related to this PR and fails on main as well:

1) "before each" hook: beforeEach for "should avoid re-downloading an analysis result"

robertbrignull

Thanks for adding more documentation and cleaning the tests up! ❤️

robertbrignull · 2022-10-19T10:48:29Z

...vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-results-manager.test.ts

@@ -10,7 +10,7 @@ import * as path from 'path';
 import { VariantAnalysisResultsManager } from '../../../remote-queries/variant-analysis-results-manager';
 import { createMockVariantAnalysisRepoTask } from '../../factories/remote-queries/gh-api/variant-analysis-repo-task';
 import { CodeQLCliServer } from '../../../cli';
-import { storagePath } from '../global.helper';
+import { storagePath } from '../../cli-integration/global.helper';


Should we still be relying on this file from the cli-integration directory, or should that file be moved to be somewhere more generic? Although I'm also aware we don't want to bloat the PR unless necessary.

There are still three files in the cli-integration folder that depend on that helper:

extensions/ql-vscode/src/vscode-tests/cli-integration/databases.test.ts

extensions/ql-vscode/src/vscode-tests/cli-integration/new-query.test.ts

extensions/ql-vscode/src/vscode-tests/cli-integration/queries.test.ts

I don't think the storagePath is set outside of the cli-integration tests (it's set in a helper that is only loaded by the cli-integration tests), so this would be undefined in these tests. Where are the results actually stored when running these tests now?

It was actually failing even earlier, when it was trying to mock the extension. I've had to change the way we stub things (including the storage path): ca3bf04 and 37bd51c.

CONTRIBUTING.md

elenatanasoiu · 2022-10-19T14:55:04Z

Taking a look at this, but running the no-workspace tests via VSCode while switching branches is proving to be difficult. It seems to keep the state of the previous branch.

Running the tests via the terminal works fine.

I'm less convinced we should be telling people in the README.md to try to run the no-workspace tests from VSCode.

We've added some tests to the cli-integration folder which don't require the CLI to be run successfully. Let's move them in the no-workspace folder. The three files are: - cli-integration/remote-queries/variant-analysis-manager.test.ts - cli-integration/remote-queries/variant-analysis-monitor.test.ts - cli-integration/remote-queries/variant-analysis-results-manager.test We're also moving the variant-analysis-process.test.ts file into tests/pure-tests/remote-queries as the processor contains helper methods rather than being a core class.

elenatanasoiu · 2022-10-19T15:46:19Z

Have rebased to account for #1631

elenatanasoiu · 2022-10-19T17:39:35Z

This is proving a bit more complicated so I've pulled out the README change into a small PR: #1635

We're no longer able to stub the extension and its context in the same way we did for our CLI integration tests. We do however have helper methods for creating smaller dummy versions in the `no-workspace` folder. We've had to extract the `createMockCliServer` method into a separate file so we can re-use it. We've also had to redefine `storagePath` since it can no longer be loaded from `global.helper.ts`. The `global.helper.ts` file only has methods that execute when we run the CLI integration tests, but not the `no-workspace` tests.

Since we no longer use a fully formed extension object for our tests, it will not have access to the real commands we've defined on it. This means we'll have to stub the `commands.executeCommand` call. The side-effect is that we're no longer able to test that our `autoDownloadVariantAnalysisResult` is really calling the API the way we expect it to do so we'll have to throw away that test.

elenatanasoiu · 2022-10-20T08:41:58Z

Thanks for the reviews @koesie10 and @robertbrignull. I've had to change the way we mock some things in these tests now that they're in the no-workspace folder, including the storage path.

More details in the commit message here: ca3bf04

This is now ready for review again.

...ions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-manager.test.ts

...ions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-monitor.test.ts

...vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-results-manager.test.ts

robertbrignull · 2022-10-21T14:13:43Z

...ions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-manager.test.ts

-      cli = extension.cliServer;
-      variantAnalysisManager = new VariantAnalysisManager(extension.ctx, cli, storagePath, logger);
+      const ctx = createMockExtensionContext();
+      cli = createMockCliServer({


Sorry if this is a silly question, but if we're having to provide a mock CLI server, does that indicate that these tests should be staying in the CLI-integration test suite?

I don't think so, but @elenatanasoiu please correct me if I'm wrong. The cli-integration tests will also actually call the cli, but in these specific tests we're not actually calling the cli. The only place where the cli would be called from these tests is if we're decoding BQRS results, but that isn't something we're testing from these tests.

Yeah, as @koesie10 said, we don't actually need the CLI for our tests. However, both the variantAnalysisManager and variantAnalysisResultsManager tests require some sort of CLI instance when they're first initialised so we need a dummy version of it without actually using it.

And add add a default value for it in the server setup. This means we can now pass only the `sandbox` param in order to have a working CLI server.

Since we've had to stub the `extension.commands` call in order to move these tests to a different folder, we're no longer calling anything on the variant analysis manager. This would normally happen by triggering an extension command. Let's assert that we call the command insteaed of the manager in our monitor tests and get rid of the manager setup.

…folder This was missed when we first moved the test files.

Since we actually have data here that we use in the tests, we might as well point the storagePath at the same directory.

robertbrignull · 2022-10-24T10:05:32Z

Just a note I'll leave the remaining review of this up to @koesie10. But do let me know if you'd like me to actively review.

koesie10 · 2022-10-24T12:05:24Z

extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/factories/cli.ts

+  sandbox: sinon.SinonSandbox,
+  mockOperations: Record<string, any[]> = {}
+): CodeQLCliServer {
+  if (mockOperations == {}) {


I don't think this will work since objects are reference types, so even {} == {} will be false. Instead, you can add the defaults as part of the function arguments or make the default undefined (although then you will also need to change the argument type).

However, I don't think we are even using these operations. So, would it also work to simply have an empty default (i.e. no operations are mocked)?

Thanks for the review @koesie10! @charisk and I paired a bit on writing some integration tests and decided we're going to go in a different direction: #1628 (comment).

I think the fact that we keep needing to adapt the mocking methods (and deleting tests) is an indicator that the no-workspace folder isn't the right place for these tests.

koesie10 · 2022-10-24T12:07:31Z

...ions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-manager.test.ts

-      variantAnalysisManager = new VariantAnalysisManager(extension.ctx, cli, storagePath, logger);
+      const ctx = createMockExtensionContext();
+      cli = createMockCliServer(sandbox);
+      const storagePath = path.join(__dirname);


Do we need this storage path to be set to an existing path or can we just use an empty string instead? I don't think we're actually storing any results in these tests

charisk · 2022-10-24T13:41:52Z

It looks like no-workspace means mocking out the extension, which from my chat with @elenatanasoiu is not exactly what we want for these tests, so moving to minimal-workspace might make more sense.

elenatanasoiu · 2022-10-24T13:49:26Z

Thanks @charisk. While pairing together, we realized that in order to be able to write some more tests for the monitor and add more integration tests to the query history, we are going to need to be able to listen to API calls.

Unfortunately, moving the tests to the no-workspace folder means we are using a very basic version of the extension object which doesn't allow us to trigger commands. This in turn means we don't get as far as triggering API calls from those commands so it limits our ability to write tests (especially integration tests that check how multiple components interact).

We decided that moving these tests into the minimal-workspace folder is a good compromise since we can make use of
a real extension object we have set up there instead of trying to make this dummy one work.

I'm going to close this PR and open a new one.

elenatanasoiu marked this pull request as ready for review October 19, 2022 10:37

elenatanasoiu requested review from a team as code owners October 19, 2022 10:37

elenatanasoiu added the secexp label Oct 19, 2022

robertbrignull reviewed Oct 19, 2022

View reviewed changes

elenatanasoiu force-pushed the elenatanasoiu/move-test-files branch 2 times, most recently from e683ec6 to ef64dbe Compare October 19, 2022 11:11

elenatanasoiu force-pushed the elenatanasoiu/move-test-files branch from ef64dbe to 37cc927 Compare October 19, 2022 15:45

elenatanasoiu force-pushed the elenatanasoiu/move-test-files branch from 9665d46 to 7bbfea6 Compare October 19, 2022 17:39

elenatanasoiu force-pushed the elenatanasoiu/move-test-files branch 4 times, most recently from 5a0399b to 11ecb3e Compare October 20, 2022 08:12

elenatanasoiu added 2 commits October 20, 2022 09:19

elenatanasoiu force-pushed the elenatanasoiu/move-test-files branch from 11ecb3e to 37bd51c Compare October 20, 2022 08:20

koesie10 reviewed Oct 21, 2022

View reviewed changes

robertbrignull reviewed Oct 21, 2022

View reviewed changes

elenatanasoiu added 5 commits October 24, 2022 10:20

Reverse order of params for createMockCliServer

4341354

Make mockOperations argument optional for createMockCliServer

91523bd

And add add a default value for it in the server setup. This means we can now pass only the `sandbox` param in order to have a working CLI server.

Move dummy zip file used in variant analysis tests into no-workspace …

da011c1

…folder This was missed when we first moved the test files.

Set storagePath to point at data/remote-queries

d5dacd5

Since we actually have data here that we use in the tests, we might as well point the storagePath at the same directory.

koesie10 reviewed Oct 24, 2022

View reviewed changes

elenatanasoiu closed this Oct 24, 2022

elenatanasoiu deleted the elenatanasoiu/move-test-files branch October 25, 2022 08:27

Move variant analysis tests out of cli-integration directory #1628

Move variant analysis tests out of cli-integration directory #1628

Uh oh!

Conversation

elenatanasoiu commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

elenatanasoiu commented Oct 19, 2022

Uh oh!

robertbrignull 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!

Uh oh!

elenatanasoiu commented Oct 19, 2022

Uh oh!

elenatanasoiu commented Oct 19, 2022

Uh oh!

elenatanasoiu commented Oct 19, 2022

Uh oh!

elenatanasoiu commented Oct 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elenatanasoiu Oct 24, 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!

robertbrignull commented Oct 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!

charisk commented Oct 24, 2022

Uh oh!

elenatanasoiu commented Oct 24, 2022

Uh oh!

Uh oh!

elenatanasoiu commented Oct 19, 2022 •

edited

Loading

elenatanasoiu Oct 24, 2022 •

edited

Loading