Conversation
aeisenberg
changed the base branch from
main
to
aeisenberg/run-with-all-data-extensions
This commit adds back the variant analysis tests that were inadvertently deleted after this commit https://github.com/github/vscode-codeql/blob/82ada54103f591228d9c61ac6b3c93c85d6ffd1c/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/run-remote-query.test.ts#L67 They are not identical to the removed tests. I refactored them so that there is a single test function invoked three times with different parameters.
aeisenberg
force-pushed
the
aeisenberg/variant-analysis-tests
branch
from
e2b7e03 to
baba68d
Compare
| qlxFilesThatExist: string[]; | ||
| filesThatDoNotExist: string[]; | ||
| }) { | ||
| const fileUri = getFile(queryPath); |
There was a problem hiding this comment.
The getFile function only seems to be used here, so would this make it clearer?
| const fileUri = getFile(queryPath); | |
| const fileUri = Uri.file(join(baseDir, queryPath)); |
There was a problem hiding this comment.
This is used throughout the test file.
There was a problem hiding this comment.
Ah, sorry, I only checked the diff, I missed that it's also used by the existing tests.
| const packFileName = packFS.fileExists("qlpack.yml") | ||
| ? "qlpack.yml" | ||
| : "codeql-pack.yml"; | ||
| const qlpackContents: any = load( |
There was a problem hiding this comment.
Do we need these any type annotations or is there a reason you've added them?
There was a problem hiding this comment.
This was from the previous tests. I can see if I can get rid of them.
| }); | ||
| }); | ||
|
|
||
| async function doVariantAnalysisTest({ |
There was a problem hiding this comment.
An alternative for creating a function for this would be to use it.each. That would avoid the need to create a function, which might make the tests easier to read (since all expectations are located within the it).
There was a problem hiding this comment.
This might work, but I'll have to see since in a follow-up PR I'm adding a new test that requires a bit of work before running the assertions.
Actually...I think I can get this done by passing in a closure that can be invoked at the start of the .each function.
There was a problem hiding this comment.
Hmmmm...I don't think this is a great approach:
- it prevents you from adding custom names for each test
- it is more cumbersome to run individual tests since it doesn't play nicely with the jest extension for vscode
There was a problem hiding this comment.
It's totally fine to not do it, it was just something we might be able to try. Both look like valid points, so let's just keep it like this for now.
| }) { | ||
| const fileUri = getFile(queryPath); | ||
|
|
||
| await variantAnalysisManager.runVariantAnalysis( |
There was a problem hiding this comment.
Since this is testing the query pack generation instead of specifically the variant analysis, would it make sense to make these tests of a new test suite for run-remote-query.ts which tests the prepareRemoteQueryRun function? I think that makes the test easier since you don't need to mock anything, although you'll lose some of the end-to-end testing that we're doing now.
There was a problem hiding this comment.
I started going down this path, but setting up the parameters for prepareRemoteQueryRun is no simpler than using the variantAnalysisManager already existing. Since the tests are already set up for this, we can keep things simpler by reusing existing test infrastructure.
|
Thanks for your review. It's ready for another look. |
2 participants
This commit adds back the variant analysis tests that were inadvertently deleted after this commit
vscode-codeql/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/run-remote-query.test.ts
Line 67 in 82ada54
They are not identical to the removed tests. I refactored them so that there is a single test function invoked three times with different parameters.
Replace this with a description of the changes your pull request makes.
Checklist
ready-for-doc-reviewlabel there.