Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/upload-lib.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

127 changes: 126 additions & 1 deletion src/upload-lib.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import * as fs from "fs";
import * as path from "path";

import test from "ava";
import * as github from "@actions/github";
import { HTTPError } from "@actions/tool-cache";
import test, { ExecutionContext } from "ava";
import * as sinon from "sinon";

import * as analyses from "./analyses";
import { AnalysisKind, CodeQuality, CodeScanning } from "./analyses";
import * as api from "./api-client";
import { getRunnerLogger, Logger } from "./logging";
import { setupTests } from "./testing-utils";
import * as uploadLib from "./upload-lib";
Expand Down Expand Up @@ -867,3 +872,123 @@ function createMockSarif(id?: string, tool?: string) {
],
};
}

const uploadPayloadMacro = test.macro({
exec: async (
t: ExecutionContext<unknown>,
options: {
analysis: analyses.AnalysisConfig;
body: (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I like that better 🙂

},
) => {
const mockData = {
payload: { sarif: "base64data", commit_sha: "abc123" },
owner: "test-owner",
repo: "test-repo",
response: {
status: 200,
data: { id: "uploaded-sarif-id" },
headers: {},
url: options.analysis.target,
},
};

const client = github.getOctokit("123");
sinon.stub(api, "getApiClient").value(() => client);
const requestStub = sinon.stub(client, "request");

const upload = async () =>
uploadLib.uploadPayload(
mockData.payload,
{
owner: mockData.owner,
repo: mockData.repo,
},
getRunnerLogger(true),
options.analysis,
);

await options.body(t, upload, requestStub, mockData);
},
title: (providedTitle = "", options: { analysis: analyses.AnalysisConfig }) =>
`uploadPayload - ${options.analysis.name} - ${providedTitle}`,
});

for (const analysis of [CodeScanning, CodeQuality]) {
test("uploads successfully", uploadPayloadMacro, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

analysis,
body: async (t, upload, requestStub, mockData) => {
requestStub
.withArgs(analysis.target, {
owner: mockData.owner,
repo: mockData.repo,
data: mockData.payload,
})
.onFirstCall()
.returns(Promise.resolve(mockData.response));
const result = await upload();
t.is(result, mockData.response.data.id);
t.true(requestStub.calledOnce);
},
});

for (const envVar of [
"CODEQL_ACTION_SKIP_SARIF_UPLOAD",
"CODEQL_ACTION_TEST_MODE",
]) {
test(`skips upload when ${envVar} is set`, uploadPayloadMacro, {
analysis,
body: async (t, upload, requestStub, mockData) =>
withTmpDir(async (tmpDir) => {
process.env.RUNNER_TEMP = tmpDir;
process.env[envVar] = "true";
const result = await upload();
t.is(result, "dummy-sarif-id");
t.false(requestStub.called);

const payloadFile = path.join(
tmpDir,
`payload-${analysis.kind}.json`,
);
t.true(fs.existsSync(payloadFile));

const savedPayload = JSON.parse(fs.readFileSync(payloadFile, "utf8"));
t.deepEqual(savedPayload, mockData.payload);
}),
});
}

test("handles error", uploadPayloadMacro, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: maybe a better title would be

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

analysis,
body: async (t, upload, requestStub) => {
const wrapApiConfigurationErrorStub = sinon.stub(
api,
"wrapApiConfigurationError",
);
const originalError = new HTTPError(404);
const wrappedError = new Error("Wrapped error message");
requestStub.rejects(originalError);
wrapApiConfigurationErrorStub
.withArgs(originalError)
.returns(wrappedError);
await t.throwsAsync(upload, {
is: wrappedError,
});
},
});
}
9 changes: 6 additions & 3 deletions src/upload-lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,9 +346,12 @@ function getAutomationID(
return api.computeAutomationID(analysis_key, environment);
}

// Upload the given payload.
// If the request fails then this will retry a small number of times.
async function uploadPayload(
/**
* Upload the given payload.
* If the request fails then this will retry a small number of times.
* This is exported for testing purposes only.
*/
export async function uploadPayload(
payload: any,
repositoryNwo: RepositoryNwo,
logger: Logger,
Expand Down
Loading