Introduce wrapper around codeql #93
Conversation
sampart
left a comment
sampart
left a comment
There was a problem hiding this comment.
Requesting changes because of my suggested code change about adding [ ... ], since that looks like a bug. The point about the parameter name database being overloaded is fairly important for code clarity too, I think.
src/autobuild.ts
Outdated
| const cmdName = process.platform === 'win32' ? 'autobuild.cmd' : 'autobuild.sh'; | ||
| const autobuildCmd = path.join(path.dirname(codeqlCmd), language, 'tools', cmdName); | ||
|
|
||
| const autobuildCmd = path.join(codeQL.getDir(), language, 'tools', cmdName); |
There was a problem hiding this comment.
What made you decide to not include autobuilding as a method on the CodeQL object? I'm not saying you should have done, I'd just like to understand your thinking.
There was a problem hiding this comment.
The interface offered by CodeQL may change (for example it may become an official command). If anything that points more strongly in favour of making it a method on the CodeQL object.
There was a problem hiding this comment.
Yes, I agree I think it fits better as being encapsulated within a method on the CodeQL object.
| * Extract code for a scanned language using 'codeql database trace-command' | ||
| * and running the language extracter. | ||
| */ | ||
| extractScannedLanguage(database: string, language: string): Promise<void>; |
There was a problem hiding this comment.
Would databasePath be more meaningful than just database here (and in finalizeDatabase)?
There was a problem hiding this comment.
Or databaseFolder to match what's used elsewhere (although I prefer databasePath).
There was a problem hiding this comment.
In general, the term database is used as an argument in various places, and sometimes it's a full path and sometimes just a database name (assuming I've understood correctly). I'd recommend using databasePath and databaseName to differentiate, rather than using the ambiguous database for both.
There was a problem hiding this comment.
Very good points. I agree better names would be worth it here. I was a bit lazy when writing this. I'll improve them all to be more descriptive.
src/codeql.ts
Outdated
| }, | ||
| getTracerEnv: async function(database: string, compilerSpec: string | undefined) { | ||
| let envFile = path.resolve(database, 'working', 'env.tmp'); | ||
| const compilerSpecArg = compilerSpec ? "--compiler-spec=" + compilerSpec : []; |
There was a problem hiding this comment.
| const compilerSpecArg = compilerSpec ? "--compiler-spec=" + compilerSpec : []; | |
| const compilerSpecArg = compilerSpec ? ["--compiler-spec=" + compilerSpec] : []; |
without the square brackets here, ...compilerSpecArg a few lines later will split the string ""--compiler-spec=____" into an array of individual characters, which I don't think is what you want.
The deleted version of this function in src/setup-tracer.ts had the square brackets.
There was a problem hiding this comment.
Thank you. Good spot.
|
I like the idea, should help a lot with testing. |
robertbrignull
force-pushed
the
codeql_mock
branch
from
d5bc56c to
5ab09ae
Compare
|
I think the new alerts are false positives due to the inaccuracy of our custom query tracking calls in javascript. Because the entire CodeQL object is defined in that method it's deeming that all the methods are called, even though they're not. I think that's what's happening anyway. |
|
I'll fix the queries in a separate PR |
I'd be interested to know what people think of this. This PR puts all calls to CodeQL behind an object with an interface that we can override in unit tests. Currently it's not possible to write tests for any area that makes CodeQL calls because it won't be available and you don't want to be actually running queries or creating databases during the unit tests.
I'm doing this now because I'd like to move processing of external queries earlier in the process to where we parse the config, and that requires running CodeQL in codepaths that we currently unit test.
As well as making more code testable, this also hopefully makes things easier to understand by keeping all the codeql invocations in one place and imposing that there is a named function interface to it with documentation so it's more clear what each call does.
I had to use a bit of judgement (i.e. random guesses) at where to draw the boundary of the interface. In some cases it includes a little bit of logic, and in one case it's actually two CodeQL calls combined into one. If you have any suggestions for improvements then please say.
@sampart, @Daverlo, @chrisgavin, @rneatherway
Merge / deployment checklist