Surface fatal CLI errors in interpret-results and run-queries - #1407
angelapwen merged 8 commits into
Conversation
Sorry, something went wrong.
|
I have some unit test errors resulting from my changes. Now that database interpret-command is called using toolRunnerErrorMatcher, when the database interpret-command tests run, they fail a line in the error matching code: Rejected promise returned by test. Reason:
Error {
message: 'Could not find program codeql-for-testing on PATH.',
}
› Object.safeWhich (node_modules/@chrisgavin/safe-which/src/index.ts:39:8)
› toolrunnerErrorCatcher (src/toolrunner-error-catcher.ts:51:7)
› Object.databaseInterpretResults (src/codeql.ts:1062:27)
› <anonymous> (src/codeql.test.ts:893:3)
It looks like safeWhich tries to find the CodeQL object, but since we mocked it, it's not able to be found. I'm not sure how to stub this method in the error matcher so that it doesn't run, though. |
Sorry, something went wrong.
I think we'll need to stub toolrunnerErrorCatcher instead of the tool runner constructor, so we can check the arguments passed to toolrunnerErrorCatcher. |
Sorry, something went wrong.
Ugh! Not sure how I missed that line. Thanks!! |
Sorry, something went wrong.
|
Actually, I see that toolrunnerErrorCatcher calls toolRunner as well so I think the original test stands. I've stubbed the safeWhich method and the test is passing locally. |
Sorry, something went wrong.
Co-authored-by: Henry Mercer <henry.mercer@me.com>
henrymercer
left a comment
There was a problem hiding this comment.
Nice!
Sorry, something went wrong.
Our error logging often doesn't highlight the most relevant part of CLI errors, when they occur (see #1283 for an example, where the actual error from CLI was hidden amongst other emitted logs).
This change only addresses this specific case; it:
In the future, we will refactor the error logging mechanism or use the existing mechanism to match more classes of errors.
I have tested this change on a private repository (for Hubbers, link). Once the step fails, the log for the analyze step ends with:
which will allow users to scroll to the bottom of the log and find the appropriate error as they expect.
(It's perhaps not ideal that the stack trace shows the stack trace of the error matcher itself, which correctly matches the error and throws it, but this was the mechanism in place and I haven't been able to think of something better. The appropriate CLI error prints just before the stack trace).
Merge / deployment checklist