Add option to pass additional arguments when running tests #785
Conversation
|
Tested after the changes and can confirm the option works as expected. In particular, I tried the |
extensions/ql-vscode/package.json
Outdated
| @@ -182,6 +182,12 @@ | |||
| "default": "%q on %d - %s, %r result count [%t]", | |||
| "description": "Default string for how to label query history items. %t is the time of the query, %q is the query name, %d is the database name, %r is the number of results, and %s is a status string." | |||
| }, | |||
| "codeQL.runningTests.additionalTestArguments": { | |||
| "scope": "window", | |||
There was a problem hiding this comment.
I suggest making this more restrictive: use machine just like we do for codeql.cli.executablePath. This ensure the setting can only be configured at the user level and prevents malicious workspaces from executing arbitrary code.
There was a problem hiding this comment.
Is this really a danger in this case? Can the run test command invoke arbitrary commands? I guess it would if you can specify a build command for a test database, but not sure if that's possible.
There was a problem hiding this comment.
Mainly being cautious. Saves us revisiting this in future if we introduce such an option. Right now, I think it's only potentially vulnerable through some indirection, e.g. if the controlled workspace points the test/extractor to a different folder it controls.
There was a problem hiding this comment.
That's fine by me, probably best to avoid. I just wanted to make sure there wasn't a specific flaw we need to prevent.
extensions/ql-vscode/src/cli.ts
Outdated
| @@ -509,12 +509,12 @@ export class CodeQLCliServer implements Disposable { | |||
| testPaths: string[], workspaces: string[], options: TestRunOptions | |||
| ): AsyncGenerator<TestCompleted, void, unknown> { | |||
|
|
|||
| const subcommandArgs = [ | |||
| const subcommandArgs = this.cliConfig.additionalTestArguments.toString().split(/\s+/).concat([ | |||
There was a problem hiding this comment.
Comment from @henrymercer. This will fail things like --string-arg "hello world". We will need some more sophisticated parsing here.
There was a problem hiding this comment.
You could change the type of the setting to array and each arg is one string element of the array, but then they can only be editted in the settings.json see: https://code.visualstudio.com/api/references/contribution-points#Configuration-property-schema
This may not be so bad as long as you have a proper description in the text.
extensions/ql-vscode/src/cli.ts
Outdated
| @@ -509,12 +509,12 @@ export class CodeQLCliServer implements Disposable { | |||
| testPaths: string[], workspaces: string[], options: TestRunOptions | |||
| ): AsyncGenerator<TestCompleted, void, unknown> { | |||
|
|
|||
| const subcommandArgs = [ | |||
| const subcommandArgs = this.cliConfig.additionalTestArguments.toString().split(/\s+/).concat([ | |||
There was a problem hiding this comment.
You could change the type of the setting to array and each arg is one string element of the array, but then they can only be editted in the settings.json see: https://code.visualstudio.com/api/references/contribution-points#Configuration-property-schema
This may not be so bad as long as you have a proper description in the text.
edoardopirovano
force-pushed
the
qltest-config
branch
from
cd35766
to
fc23a3a
Compare
|
Have addressed the above comments and squashed into one commit. |
|
@github/docs-content-dsp We probably need to add some documentation for this new functionality. |
Thanks! I've opened an internal docs issue. |
This PR addresses #606 by adding a configuration option to pass additional arguments to the CLI when running tests. By default, this option is empty and no additional arguments are passed.