Add unit tests for `uploadPayload` by redsun82 · Pull Request #3185 · github/codeql-action

redsun82 · 2025-10-07T12:31:07Z

Risk assessment

Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

None, these are only tests.

How did/will you validate this change?

Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

CI

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Consider adding a changelog entry for this change.
Confirm the readme and docs have been updated if necessary.

Copilot

Pull Request Overview

This PR adds comprehensive unit tests for the uploadSpecifiedFiles function in the upload library. The tests cover various scenarios including single and multiple SARIF file uploads, category mapping for code quality analysis, SARIF dumping functionality, and different upload modes.

Key Changes:

Added 7 new test cases covering different upload scenarios
Created helper functions setupUploadEnvironment and stubUploadDependencies to reduce test code duplication
Enhanced the createMockSarif helper to support optional automation details for better test flexibility

src/upload-lib.test.ts

mbg

I appreciate the follow-up to #3180 to add tests. I started having a look, but stopped after the first couple of test cases:

In #3180, the main change was to uploadPayload, and I was expecting tests to focus around the behaviour of uploadPayload. While it's not currently exported, we have numerous examples of functions that are exported for testing purposes only.
Tests for uploadSpecifiedFiles would of course be welcome, but they would need to focus on the behaviour of uploadSpecifiedFiles itself, not primarily on the behaviour of uploadPayload.
The setupUploadEnvironment function is largely superfluous; setupTests already does this.
The tests would probably benefit from using a macro to set up what is needed for testing uploadSpecifiedFiles, rather than duplicating large chunks of setup code across every test case.
A lot of the comments are nonsensical (and show a lack of understanding of what's going on) or superfluous

src/upload-lib.test.ts

redsun82 · 2025-10-09T07:28:58Z

I probably let copilot a tad too loose on this 😅

redsun82 · 2025-10-09T10:34:01Z

@mbg I've decicided to ditch all that copilot had done and (for the moment) only add uploadPayload tests like you suggested. Now that function should be completely covered (though I chose not to test the different core.warnings logic, as that is logging only and not functional).

mbg

Generally this is a big improvement, thank you! There are a couple of points where the implementation of the tests here differs from established patterns for tests elsewhere in the codebase, so it might be good to consider whether it's worth having those differences or we can align it more with the established patterns. That said, there's nothing inherently wrong here, so I'd be happy to approve this if we think it makes sense to keep these tests as they are now.

mbg · 2025-10-09T12:01:18Z

src/upload-lib.test.ts

+  title: (providedTitle = "", options: { analysis: analyses.AnalysisConfig }) =>
+    `uploadPayload - ${options.analysis.name} - ${providedTitle}`,
+});
+
+for (const analysis of [CodeScanning, CodeQuality]) {
+  test("uploads successfully", uploadPayloadMacro, {


Minor: I was going to comment that it would probably make sense to include the analysis name in the test names, but I then saw that this already happens via the macro. I have no strong feelings on which is better. In other test files where we have top-level for-loops, we splice the variables into the title provided to test, so it might be good for consistency to do that here as well, but I can see how doing it in the macro is less repetitive.

mbg · 2025-10-09T12:07:21Z

src/upload-lib.test.ts

+    });
+  }
+
+  test("handles error", uploadPayloadMacro, {


Minor: maybe a better title would be

Suggested change

test("handles error", uploadPayloadMacro, {

test("wraps request errors using `wrapApiConfigurationError`", uploadPayloadMacro, {

mbg · 2025-10-09T12:16:46Z

src/upload-lib.test.ts

+      body: (
+        t: ExecutionContext<unknown>,
+        upload: () => Promise<string>,
+        requestStub: sinon.SinonStub,
+        mockData: {
+          payload: { sarif: string; commit_sha: string };
+          owner: string;
+          repo: string;
+          response: {
+            status: number;
+            data: { id: string };
+            headers: any;
+            url: string;
+          };
+        },
+      ) => void | Promise<void>;


I don't think we have this pattern with a continuation (here: body) for the macro to call elsewhere, but I can see why it could be useful. Were there any particular reasons why you chose to do it this way over the established pattern of having inputs/input modifiers and an expected result? (All tests but the last are expected to return a string so you could have a expectedId parameter, and you could just not use the macro for the test where you expect failure.)

fundamentally I wanted to have the setup that the macro does in common for all the tests. I had a version of this where all the test behaviour was being driven by flags in options (courtesy of copilot), but then the macro code was a mess of conditionals and the tests weren't as clear. The upload skipping tests too do some different checking that I find would be a tad bad to shove in the macro code.

Also, maybe that could be covered better by a setup function, rather than a macro?

maybe, considering the test macro is not doing any checks after calling body

yeah, I like that better 🙂

mbg · 2025-10-09T12:17:33Z

src/upload-lib.test.ts

+    t: ExecutionContext<unknown>,
+    options: {
+      analysis: analyses.AnalysisConfig;
+      body: (


Did you have any particular reason for having this options object rather than having analysis and body be parameters of exec?

initially I had more flags that profited from being named explicitly, but I agree that at this stage it's not really needed

redsun82 force-pushed the redsun82/skip-sarif-upload-tests branch 2 times, most recently from 528bbcc to 4cea67d Compare October 7, 2025 15:17

redsun82 mentioned this pull request Oct 7, 2025

Introduce CODEQL_ACTION_SKIP_SARIF_UPLOAD #3180

Merged

redsun82 added 2 commits October 8, 2025 09:34

Add unit tests for uploadSpecifiedFiles

4489a63

Fix test after rebase

a57997f

redsun82 force-pushed the redsun82/skip-sarif-upload-tests branch from c9101b1 to a57997f Compare October 8, 2025 07:34

Base automatically changed from redsun82/skip-sarif-upload to main October 8, 2025 10:09

redsun82 marked this pull request as ready for review October 8, 2025 10:13

redsun82 requested a review from a team as a code owner October 8, 2025 10:13

Copilot AI review requested due to automatic review settings October 8, 2025 10:13

Copilot AI reviewed Oct 8, 2025

View reviewed changes

Address copilot review

621809b

mbg requested changes Oct 8, 2025

View reviewed changes

redsun82 added 2 commits October 9, 2025 11:38

Merge branch 'main' into redsun82/skip-sarif-upload-tests

aeb12f6

Scratch uploadSpecifiedFiles tests, make uploadPayload tests instead

a841c54

redsun82 changed the title ~~Add unit tests for uploadSpecifiedFiles~~ Add unit tests for uploadPayload Oct 9, 2025

Simplify uploadPayload tests

ff2fc66

redsun82 requested a review from mbg October 9, 2025 10:31

mbg reviewed Oct 9, 2025

View reviewed changes

Address review

610c7c6

redsun82 requested a review from mbg October 9, 2025 13:35

mbg approved these changes Oct 10, 2025

View reviewed changes

redsun82 merged commit a8440d0 into main Oct 10, 2025
248 of 249 checks passed

redsun82 deleted the redsun82/skip-sarif-upload-tests branch October 10, 2025 12:00

This was referenced Oct 10, 2025

Merge main into releases/v4 #3198

Merged

Merge releases/v4 into releases/v3 #3201

Merged

	test("handles error", uploadPayloadMacro, {
	test("wraps request errors using `wrapApiConfigurationError`", uploadPayloadMacro, {

Conversation

redsun82 commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Risk assessment

Which use cases does this change impact?

How did/will you validate this change?

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?

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redsun82 commented Oct 9, 2025

Uh oh!

redsun82 commented Oct 9, 2025

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

redsun82 commented Oct 7, 2025 •

edited

Loading