Report some types of errors in start-proxy status reports
#3444
Conversation
src/start-proxy.test.ts
Outdated
|
|
||
| setupTests(test); | ||
|
|
||
| test("sendFailedStatusReport - does not report arbitrary error messages", async (t) => { |
There was a problem hiding this comment.
From what I can tell, the idea behind this test is that while the DownloadFailed error message would normally be regarded as safe, here the error object is not an instance of StartProxyError and it is therefore rejected.
I think it would be clearer to have a test that throws a different error message, and verifies the message that is sent to createStatusReportBase precisely (i.e. checking for "Error from start-proxy Action omitted" rather than the full original error message not being present).
Also, it might be worth adding a similar test for the top-level runWrapper. Note we'd have to follow the pattern of analyze-action in having one file per test case.
There was a problem hiding this comment.
I think it would be clearer to have a test that throws a different error message, and verifies the message that is sent to createStatusReportBase precisely (i.e. checking for "Error from start-proxy Action omitted" rather than the full original error message not being present).
I have added more tests here to cover various cases, and named them differently.
Also, it might be worth adding a similar test for the top-level runWrapper. Note we'd have to follow the pattern of analyze-action in having one file per test case.
I have some thoughts on this, but they are outside of the scope of this PR I think.
src/start-proxy.ts
Outdated
| // If the error is a `StartProxyError`, the constructor ensures that the | ||
| // message comes from `StartProxyErrorType`. | ||
| if (error instanceof StartProxyError) { | ||
| return error.message; | ||
| } |
There was a problem hiding this comment.
This check relies on type safety. While we try to make sure our code is type safe, that isn't something TypeScript can guarantee, and it would be safer to check the allowed values in StartProxyErrorType explicitly.
There was a problem hiding this comment.
As discussed, I have changed the approach here slightly so that the enum doesn't have the associated string values anymore (and we don't store any string values in the StartProxyError objects), but instead store the (numeric) key and then resolve it to an error message in getSafeErrorMessage.
github-actions
bot
added
size/XL
This PR allows the
start-proxyaction to report some types of errors in status reports. The basic idea here is that we have aStartProxyErrorTypeenum to represent types of failures we care about, with associated, fixed error messages. We then have aStartProxyErrortype for exceptions which only accepts members of theStartProxyErrorTypeenum as messages.Most of the size of this PR comes from refactoring functions from
start-proxy-actiontostart-proxyto make it easier to handle the individual error cases, and to add better unit test coverage.Best reviewed commit-by-commit.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, CCR, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist