Add tests for upload-sarif - #3166
Conversation
Sorry, something went wrong.
Note that this also fixes the format of the `sarif-ids` outputs to match what is documented
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the upload-sarif functionality by extracting upload logic from the action file into a separate module and adding comprehensive unit tests. The changes also fix the output format for sarif-ids and add error handling for cases where no SARIF files are found.
Key changes:
- Moves core upload logic from upload-sarif-action.ts to upload-sarif.ts for better separation of concerns
- Adds extensive unit tests covering various SARIF file scenarios and upload combinations
- Changes sarif-ids output format from array to object structure for easier consumption
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file| File | Description |
|---|---|
| src/upload-sarif.ts | New module containing extracted upload logic with findAndUpload and uploadSarif functions |
| src/upload-sarif.test.ts | Comprehensive unit tests for the new upload-sarif module functions |
| src/upload-sarif-action.ts | Refactored to use the new upload-sarif module and simplified upload flow |
| pr-checks/checks/upload-quality-sarif.yml | Updated test to use new sarif-ids object format |
| lib/upload-sarif-action.js | Generated JavaScript reflecting the TypeScript changes |
| .github/workflows/__upload-quality-sarif.yml | Updated workflow to use new sarif-ids object format |
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
esbena
left a comment
There was a problem hiding this comment.
LGTM.
It's not blocking, but I'm getting a bit concerned about the complexity of the tests, especially
2adc894. It's the classic tradeoff between what tests current behaviour/API and what is maintainable.
Sorry, something went wrong.
| analysisKind === AnalysisKind.CodeScanning | ||
| ? CodeScanning | ||
| : CodeQuality, |
There was a problem hiding this comment.
could this just be analysisKind? Or do we prefer the explicit casing?
Sorry, something went wrong.
There was a problem hiding this comment.
analysisKind: AnalysisKind while CodeScanning: AnalysisConfig and CodeQuality: AnalysisConfig. It maps the kind to the matching configuration. We could have this as a function for reusability instead of this ad-hoc mechanism, but we can't just use analysisKind here since we need an AnalysisConfig object for uploadSpecifiedFiles.
Sorry, something went wrong.
There was a problem hiding this comment.
right. My eyes missed the AnalysisKind. prefix.
Sorry, something went wrong.
I could modify UploadResult to include the array of files, but then we'd probably have to mock a bunch of other functions that uploadSpecifiedFiles calls or have more things for the tests to set up. I'm inclined to accept the tests as they are for now, since I am planning to make further changes anyway and they might affect these tests and allow us to simplify this again later. |
Sorry, something went wrong.
This PR takes parts of #3157:
This doesn't make any further changes to the actual upload logic beyond the hotfix we added in #3160. I am planning a follow-up PR to improve on that after having come up with an idea for a good approach over the weekend, but want to the tests, sarif-ids fix, and error for the case where no files were uploaded merged first.
Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist