Skip to content

New setting to specify number of paths per alert #931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shati-patel merged 2 commits into from
Aug 25, 2021
Merged

New setting to specify number of paths per alert #931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
450de1c
New setting to specify number of paths per alert
shati-patel Aug 23, 2021
2d06477
Address review comments
shati-patel Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions extensions/ql-vscode/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- Add support for filename pattern in history view. [#930](https://github.com/github/vscode-codeql/pull/930)
- Add an option _View Results (CSV)_ to view the results of a non-alert query. The existing options for alert queries have been renamed to _View Alerts_ to avoid confusion. [#929](https://github.com/github/vscode-codeql/pull/929)
- Allow users to specify the number of paths to display for each alert. [#931](https://github.com/github/vscode-codeql/pull/931)

## 1.5.3 - 18 August 2021

Expand Down
7 changes: 7 additions & 0 deletions extensions/ql-vscode/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,13 @@
"default": false,
"description": "Enable debug logging and tuple counting when running CodeQL queries. This information is useful for debugging query performance."
},
"codeQL.runningQueries.maxPaths": {
"type": "integer",
"default": 4,
Comment thread
adityasharad marked this conversation as resolved.
"minimum": 1,
"maximum": 256,
"markdownDescription": "Max number of paths to display for each alert found by a path query (`@kind path-problem`)."
},
"codeQL.runningQueries.autoSave": {
"type": "boolean",
"default": false,
Expand Down
5 changes: 5 additions & 0 deletions extensions/ql-vscode/src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,11 @@ export class CodeQLCliServer implements Disposable {
this.cliConfig.numberThreads.toString(),
);

args.push(
Comment thread
adityasharad marked this conversation as resolved.
'--max-paths',
this.cliConfig.maxPaths.toString(),
);

args.push(resultsPath);
await this.runCodeQlCliCommand(['bqrs', 'interpret'], args, 'Interpreting query results');
}
Expand Down
8 changes: 7 additions & 1 deletion extensions/ql-vscode/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ const CACHE_SIZE_SETTING = new Setting('cacheSize', RUNNING_QUERIES_SETTING);
const TIMEOUT_SETTING = new Setting('timeout', RUNNING_QUERIES_SETTING);
const MEMORY_SETTING = new Setting('memory', RUNNING_QUERIES_SETTING);
const DEBUG_SETTING = new Setting('debug', RUNNING_QUERIES_SETTING);
const MAX_PATHS = new Setting('maxPaths', RUNNING_QUERIES_SETTING);
const RUNNING_TESTS_SETTING = new Setting('runningTests', ROOT_SETTING);
const RESULTS_DISPLAY_SETTING = new Setting('resultsDisplay', ROOT_SETTING);

Expand Down Expand Up @@ -112,12 +113,13 @@ export interface QueryHistoryConfig {
onDidChangeConfiguration: Event<void>;
}

const CLI_SETTINGS = [ADDITIONAL_TEST_ARGUMENTS_SETTING, NUMBER_OF_TEST_THREADS_SETTING, NUMBER_OF_THREADS_SETTING];
const CLI_SETTINGS = [ADDITIONAL_TEST_ARGUMENTS_SETTING, NUMBER_OF_TEST_THREADS_SETTING, NUMBER_OF_THREADS_SETTING, MAX_PATHS];

export interface CliConfig {
additionalTestArguments: string[];
numberTestThreads: number;
numberThreads: number;
maxPaths: number;
Copy link
Copy Markdown
Contributor

@adityasharad adityasharad Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can unit test this - take a look at https://github.com/github/vscode-codeql/blob/main/extensions/ql-vscode/src/vscode-tests/minimal-workspace/config.test.ts and add it to the list of values. (I think it will go under the CLIConfigListener rather than the QueryServerConfigListener.)

Copy link
Copy Markdown
Contributor Author

@shati-patel shati-patel Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it to that test file here. Silly question, but what should values be? I couldn't work out if there's a logic to it 🙃 Is it just any valid values?

Copy link
Copy Markdown
Contributor

@adityasharad adityasharad Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a silly question, I wondered the same while looking at it. I think the intent is to test a range of values that we expect to be handled: for this kind of test it's a good idea to check the minimum, the maximum, some common value in the middle, and a zero/empty value. If the test is capable of handling errors, then also test some value that shouldn't be allowed.

Copy link
Copy Markdown
Contributor Author

@shati-patel shati-patel Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the test only lets me test 2 values?

vscode-codeql/extensions/ql-vscode/src/vscode-tests/minimal-workspace/config.test.ts

Lines 34 to 36 in e119218

name: string;
property: string;
values: [T, T];

I can't quite work out if the test is doing anything with the values themselves, or just listening for when they change 😕

Copy link
Copy Markdown
Contributor

@adityasharad adityasharad Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh you are right. The test is just making sure the listeners handle a change. So [0, 1] sounds fine.
We can look separately at creating an integration test that produces multiple paths - right now we have none so there's nothing to update.

onDidChangeConfiguration?: Event<void>;
}

Expand Down Expand Up @@ -264,6 +266,10 @@ export class CliConfigListener extends ConfigListener implements CliConfig {
return NUMBER_OF_THREADS_SETTING.getValue<number>();
}

public get maxPaths(): number {
return MAX_PATHS.getValue<number>();
}

protected handleDidChangeConfiguration(e: ConfigurationChangeEvent): void {
this.handleDidChangeConfigurationForRelevantSettings(CLI_SETTINGS, e);
}
Expand Down
4 changes: 4 additions & 0 deletions extensions/ql-vscode/src/vscode-tests/minimal-workspace/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ describe('config listeners', function() {
name: 'codeQL.runningTests.numberOfThreads',
property: 'numberTestThreads',
values: [1, 0]
}, {
name: 'codeQL.runningQueries.maxPaths',
property: 'maxPaths',
values: [0, 1]
}]
},
{
Expand Down