New setting to specify number of paths per alert#931
Conversation
adityasharad
left a comment
There was a problem hiding this comment.
Nice! A few suggestions but overall structure looks good :)
| additionalTestArguments: string[]; | ||
| numberTestThreads: number; | ||
| numberThreads: number; | ||
| maxPaths: number; |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It looks like the test only lets me test 2 values?
I can't quite work out if the test is doing anything with the values themselves, or just listening for when they change 😕
There was a problem hiding this comment.
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.
6a04a1d to
3241942
Compare
|
Thanks for the review and the helpful pointers! 🎉 I've addressed comments in the latest commit (and added some more questions) |
6f2a652 to
2d06477
Compare
Will fix #919 .
Added a new setting
codeQL.runningQueries.maxPathsthat lets you configure how many paths to display 🙂 I tested this with a few databases/queries (e.g. this one)Checklist
@github/docs-content-codeqlhas been cc'd in all issues for UI or other user-facing changes made by this pull request.