diff --git a/README.md b/README.md index 9e446ee02..c0b605c9e 100644 --- a/README.md +++ b/README.md @@ -435,6 +435,15 @@ GitHub API access token. Ref to use for reporting the commit +#### `pull-request-number` (Optional) + +- Type: Number +- Default: N/A + +Pull request number to use for comments instead of the pull request in the event payload. This is useful +for events such as `repository_dispatch`, which can pass a pull request number in `client_payload` but do +not have a top-level `pull_request` field. + #### `auto-push` (Optional) - Type: Boolean diff --git a/action.yml b/action.yml index 3b15a7c88..8e20c72e3 100644 --- a/action.yml +++ b/action.yml @@ -33,6 +33,9 @@ inputs: ref: description: 'optional Ref to use when finding commit' required: false + pull-request-number: + description: 'Optional pull request number to use for comments instead of the event payload' + required: false auto-push: description: 'Push GitHub Pages branch to remote automatically. This option requires github-token input' required: false diff --git a/src/config.ts b/src/config.ts index 6a728ba51..d83589496 100644 --- a/src/config.ts +++ b/src/config.ts @@ -25,6 +25,7 @@ export interface Config { externalDataJsonPath: string | undefined; maxItemsInChart: number | null; ref: string | undefined; + pullRequestNumber: number | null; goForcePackageSuffix: boolean; } @@ -209,6 +210,12 @@ function validateMaxItemsInChart(max: number | null) { } } +function validatePullRequestNumber(pullRequestNumber: number | null) { + if (pullRequestNumber !== null && pullRequestNumber <= 0) { + throw new Error(`'pull-request-number' input value must be one or more but got ${pullRequestNumber}`); + } +} + function validateAlertThreshold(alertThreshold: number | null, failThreshold: number | null): asserts alertThreshold { if (alertThreshold === null) { throw new Error("'alert-threshold' input must not be empty"); @@ -229,6 +236,7 @@ export async function configFromJobInput(): Promise { const name: string = core.getInput('name'); const githubToken: string | undefined = core.getInput('github-token') || undefined; const ref: string | undefined = core.getInput('ref') || undefined; + const pullRequestNumber = getUintInput('pull-request-number'); const autoPush = getBoolInput('auto-push'); const skipFetchGhPages = getBoolInput('skip-fetch-gh-pages'); const commentAlways = getBoolInput('comment-always'); @@ -264,6 +272,7 @@ export async function configFromJobInput(): Promise { validateAlertCommentCcUsers(alertCommentCcUsers); externalDataJsonPath = await validateExternalDataJsonPath(externalDataJsonPath, autoPush); validateMaxItemsInChart(maxItemsInChart); + validatePullRequestNumber(pullRequestNumber); if (failThreshold === null) { failThreshold = alertThreshold; } @@ -289,6 +298,7 @@ export async function configFromJobInput(): Promise { maxItemsInChart, failThreshold, ref, + pullRequestNumber, goForcePackageSuffix, }; } diff --git a/src/write.ts b/src/write.ts index 4e9ebad6c..d85b94116 100644 --- a/src/write.ts +++ b/src/write.ts @@ -239,19 +239,25 @@ function buildAlertComment( return lines.join('\n'); } -async function leaveComment(commitId: string, body: string, commentId: string, token: string) { +async function leaveComment( + commitId: string, + body: string, + commentId: string, + token: string, + pullRequestNumber: number | null, +) { core.debug('Sending comment:\n' + body); const repoMetadata = getCurrentRepoMetadata(); - const pr = github.context.payload.pull_request; + const pullRequest = pullRequestNumber ?? github.context.payload.pull_request?.number; - return await (pr?.number - ? leavePRComment(repoMetadata.owner.login, repoMetadata.name, pr.number, body, commentId, token) + return await (pullRequest + ? leavePRComment(repoMetadata.owner.login, repoMetadata.name, pullRequest, body, commentId, token) : leaveCommitComment(repoMetadata.owner.login, repoMetadata.name, commitId, body, commentId, token)); } async function handleComment(benchName: string, curSuite: Benchmark, prevSuite: Benchmark, config: Config) { - const { commentAlways, githubToken } = config; + const { commentAlways, githubToken, pullRequestNumber } = config; if (!commentAlways) { core.debug('Comment check was skipped because comment-always is disabled'); @@ -266,7 +272,7 @@ async function handleComment(benchName: string, curSuite: Benchmark, prevSuite: const body = buildComment(benchName, curSuite, prevSuite); - await leaveComment(curSuite.commit.id, body, `${benchName} Summary`, githubToken); + await leaveComment(curSuite.commit.id, body, `${benchName} Summary`, githubToken, pullRequestNumber); } async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Benchmark, config: Config) { @@ -291,7 +297,13 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be if (!githubToken) { throw new Error("'comment-on-alert' input is set but 'github-token' input is not set"); } - const res = await leaveComment(curSuite.commit.id, body, `${benchName} Alert`, githubToken); + const res = await leaveComment( + curSuite.commit.id, + body, + `${benchName} Alert`, + githubToken, + config.pullRequestNumber, + ); const url = res.data.html_url; message = body + `\nComment was generated at ${url}`; } diff --git a/test/config.spec.ts b/test/config.spec.ts index 31989b3da..281dd7544 100644 --- a/test/config.spec.ts +++ b/test/config.spec.ts @@ -44,6 +44,7 @@ describe('configFromJobInput()', function () { 'alert-comment-cc-users': '', 'external-data-json-path': '', 'max-items-in-chart': '', + 'pull-request-number': '', }; const validationTests: Array<{ @@ -139,6 +140,22 @@ describe('configFromJobInput()', function () { }, expected: /'max-items-in-chart' input value must be one or more/, }, + { + what: 'pull-request-number must be an integer', + inputs: { + ...defaultInputs, + 'pull-request-number': '3.14', + }, + expected: /'pull-request-number' input must be unsigned integer but got '3.14'/, + }, + { + what: 'pull-request-number must not be zero', + inputs: { + ...defaultInputs, + 'pull-request-number': '0', + }, + expected: /'pull-request-number' input value must be one or more/, + }, { what: 'alert-threshold must not be empty', inputs: { @@ -183,6 +200,7 @@ describe('configFromJobInput()', function () { alertCommentCcUsers: string[]; hasExternalDataJsonPath: boolean; maxItemsInChart: null | number; + pullRequestNumber: null | number; failThreshold: number | null; } @@ -200,6 +218,7 @@ describe('configFromJobInput()', function () { alertCommentCcUsers: [], hasExternalDataJsonPath: false, maxItemsInChart: null, + pullRequestNumber: null, failThreshold: null, }; @@ -271,6 +290,11 @@ describe('configFromJobInput()', function () { inputs: { ...defaultInputs, 'max-items-in-chart': '50' }, expected: { ...defaultExpected, maxItemsInChart: 50 }, }, + { + what: 'pull request number', + inputs: { ...defaultInputs, 'pull-request-number': '3197' }, + expected: { ...defaultExpected, pullRequestNumber: 3197 }, + }, { what: 'different failure threshold from alert threshold', inputs: { ...defaultInputs, 'fail-threshold': '300%' }, @@ -303,6 +327,7 @@ describe('configFromJobInput()', function () { A.ok(path.isAbsolute(actual.outputFilePath), actual.outputFilePath); A.ok(path.isAbsolute(actual.benchmarkDataDirPath), actual.benchmarkDataDirPath); A.equal(actual.maxItemsInChart, test.expected.maxItemsInChart); + A.equal(actual.pullRequestNumber, test.expected.pullRequestNumber); if (test.expected.failThreshold === null) { A.equal(actual.failThreshold, test.expected.alertThreshold); } else { diff --git a/test/fakedOctokit.ts b/test/fakedOctokit.ts index 70526f506..bd3076423 100644 --- a/test/fakedOctokit.ts +++ b/test/fakedOctokit.ts @@ -1,4 +1,6 @@ type OctokitOpts = { owner: string; repo: string; commit_sha: string; body: string }; +type PullRequestOpts = { owner: string; repo: string; pull_number: number }; +type CreateReviewOpts = PullRequestOpts & { event: string; body: string }; class FakedOctokitRepos { spyOpts: OctokitOpts[]; constructor() { @@ -23,9 +25,35 @@ class FakedOctokitRepos { export const fakedRepos = new FakedOctokitRepos(); +class FakedOctokitPulls { + createdReviews: CreateReviewOpts[] = []; + + listReviews() { + return Promise.resolve({ data: [] }); + } + + createReview(opt: CreateReviewOpts) { + this.createdReviews.push(opt); + return Promise.resolve({ + url: 'https://dummy-review-url', + status: 200, + data: { + html_url: 'https://dummy-review-url', + }, + }); + } + + clear() { + this.createdReviews = []; + } +} + +export const fakedPulls = new FakedOctokitPulls(); + export class FakedOctokit { rest = { repos: fakedRepos, + pulls: fakedPulls, }; opt: { token: string }; constructor(token: string) { diff --git a/test/write.spec.ts b/test/write.spec.ts index 1cf7ce729..2f8ca7345 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -7,7 +7,7 @@ import { Config } from '../src/config'; import { Benchmark } from '../src/extract'; import { DataJson, writeBenchmark } from '../src/write'; import { expect } from '@jest/globals'; -import { FakedOctokit, fakedRepos } from './fakedOctokit'; +import { FakedOctokit, fakedPulls, fakedRepos } from './fakedOctokit'; import { wrapBodyWithBenchmarkTags } from '../src/comment/benchmarkCommentTags'; const ok: (x: any, msg?: string) => asserts x = (x, msg) => { @@ -132,6 +132,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe afterEach(function () { fakedRepos.clear(); + fakedPulls.clear(); }); // Utilities for test data @@ -188,6 +189,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe maxItemsInChart: null, failThreshold: 2.0, ref: undefined, + pullRequestNumber: null, goForcePackageSuffix: false, }; @@ -215,6 +217,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe expectedAdded?: Benchmark; error?: string[]; commitComment?: string; + pullRequestComment?: number; repoPayload?: null | RepositoryPayloadSubset; gitServerUrl?: string; }> = [ @@ -243,6 +246,36 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe }, gitServerUrl: serverUrl, }, + { + it: 'uses pull request number override for comments', + config: { + ...defaultCfg, + githubToken: 'dummy token', + commentAlways: true, + pullRequestNumber: 3197, + }, + data: { + lastUpdate, + repoUrl, + entries: { + 'Test benchmark': [ + { + commit: commit('prev commit id'), + date: lastUpdate - 1000, + tool: 'cargo', + benches: [bench('bench_fib_10', 100)], + }, + ], + }, + }, + added: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'cargo', + benches: [bench('bench_fib_10', 135)], + }, + pullRequestComment: 3197, + }, { it: 'appends new result to existing data with normalized units - new unit smaller', config: { ...defaultCfg, tool: 'catch2' }, @@ -827,7 +860,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe ]; it.each(normalCases)('$it', async function (t) { - const { data, added, config, repoPayload, error, commitComment } = t; + const { data, added, config, repoPayload, error, commitComment, pullRequestComment } = t; const expectedAdded = t.expectedAdded ?? added; gitHubContext.payload.repository = { @@ -930,6 +963,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe expect('github-action-benchmark').toEqual(actionLink.text()); expect('https://github.com/marketplace/actions/continuous-benchmark').toEqual(actionLink.attr('href')); } + + if (pullRequestComment !== undefined) { + expect(fakedRepos.spyOpts).toHaveLength(0); + expect(fakedPulls.createdReviews).toHaveLength(1); + const review = fakedPulls.createdReviews[0]; + expect(review.pull_number).toEqual(pullRequestComment); + expect( + review.body.startsWith(''), + ).toBe(true); + } }); }); @@ -1007,6 +1050,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe maxItemsInChart: null, failThreshold: 2.0, ref: undefined, + pullRequestNumber: null, goForcePackageSuffix: false, };