From f65fed889e56954a4d856177a68fe51ecd583865 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Trze=C5=9Bniewski?= Date: Thu, 28 Mar 2024 08:38:54 +0100 Subject: [PATCH] feat: Comment on PR and auto update comment (#223) * create PR comment when PR is available * auto-update existing PR comment * add comment-on-alert and gh token to ci jobs --- .github/workflows/ci-results-repo.yml | 2 +- .github/workflows/ci.yml | 22 +++++++- CHANGELOG.md | 1 + src/comment/benchmarkCommentTags.ts | 13 +++++ src/comment/findExistingPRReviewId.ts | 28 +++++++++++ src/comment/leaveCommitComment.ts | 25 ++++++++++ src/comment/leavePRComment.ts | 72 +++++++++++++++++++++++++++ src/write.ts | 32 +++++------- test/write.spec.ts | 6 ++- 9 files changed, 178 insertions(+), 23 deletions(-) create mode 100644 src/comment/benchmarkCommentTags.ts create mode 100644 src/comment/findExistingPRReviewId.ts create mode 100644 src/comment/leaveCommitComment.ts create mode 100644 src/comment/leavePRComment.ts diff --git a/.github/workflows/ci-results-repo.yml b/.github/workflows/ci-results-repo.yml index 933a52e39..5a1ca41de 100644 --- a/.github/workflows/ci-results-repo.yml +++ b/.github/workflows/ci-results-repo.yml @@ -378,7 +378,7 @@ jobs: cp ./dist/other-repo/dev/bench/data.js before_data.js - name: Store benchmark result - uses: benchmark-action/github-action-benchmark@v1 + uses: ./ with: name: Criterion.rs Benchmark tool: 'cargo' diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 993ac985c..071287f70 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,8 @@ jobs: skip-fetch-gh-pages: true fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'Benchmark.Net Benchmark' benchmarkjs: @@ -69,6 +71,8 @@ jobs: skip-fetch-gh-pages: true fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'Benchmark.js Benchmark' catch2-framework: @@ -104,6 +108,8 @@ jobs: skip-fetch-gh-pages: true fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'Catch2 Benchmark' cpp-framework: @@ -141,6 +147,8 @@ jobs: skip-fetch-gh-pages: true fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'C++ Benchmark' go: @@ -174,6 +182,8 @@ jobs: skip-fetch-gh-pages: true fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'Go Benchmark' java-jmh: @@ -211,6 +221,8 @@ jobs: skip-fetch-gh-pages: true fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'JMH Benchmark' julia-benchmark: @@ -250,6 +262,8 @@ jobs: skip-fetch-gh-pages: true fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'Julia benchmark' pytest-benchmark: @@ -286,6 +300,8 @@ jobs: skip-fetch-gh-pages: true fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'Python Benchmark with pytest-benchmark' rust: @@ -316,6 +332,8 @@ jobs: output-file-path: examples/rust/output.txt fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'Rust Benchmark' rust-criterion-rs-framework: @@ -339,13 +357,15 @@ jobs: cp ./dev/bench/data.js before_data.js git checkout - - name: Store benchmark result - uses: benchmark-action/github-action-benchmark@v1 + uses: ./ with: name: Criterion.rs Benchmark tool: 'cargo' output-file-path: examples/criterion-rs/output.txt fail-on-alert: true summary-always: true + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-on-alert: true - run: node ./dist/scripts/ci_validate_modification.js before_data.js 'Criterion.rs Benchmark' only-alert-with-cache: diff --git a/CHANGELOG.md b/CHANGELOG.md index b6b3225c5..f503963f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased - **fix** Rust benchmarks not comparing to baseline (#235) +- **feat** Comment on PR and auto update comment (#223) # [v1.19.3](https://github.com/benchmark-action/github-action-benchmark/releases/tag/v1.19.3) - 02 Feb 2024 diff --git a/src/comment/benchmarkCommentTags.ts b/src/comment/benchmarkCommentTags.ts new file mode 100644 index 000000000..6e88bffb2 --- /dev/null +++ b/src/comment/benchmarkCommentTags.ts @@ -0,0 +1,13 @@ +const BENCHMARK_COMMENT_TAG = 'github-benchmark-action-comment'; + +export function benchmarkStartTag(commentId: string) { + return ``; +} + +export function benchmarkEndTag(commentId: string) { + return ``; +} + +export function wrapBodyWithBenchmarkTags(commentId: string, body: string) { + return `${benchmarkStartTag(commentId)}\n${body}\n${benchmarkEndTag(commentId)}`; +} diff --git a/src/comment/findExistingPRReviewId.ts b/src/comment/findExistingPRReviewId.ts new file mode 100644 index 000000000..b41c594fc --- /dev/null +++ b/src/comment/findExistingPRReviewId.ts @@ -0,0 +1,28 @@ +import * as github from '@actions/github'; +import { benchmarkStartTag } from './benchmarkCommentTags'; +import * as core from '@actions/core'; + +export async function findExistingPRReviewId( + repoOwner: string, + repoName: string, + pullRequestNumber: number, + benchName: string, + token: string, +) { + core.debug('findExistingPRReviewId start'); + const client = github.getOctokit(token); + + const existingReviewsResponse = await client.rest.pulls.listReviews({ + owner: repoOwner, + repo: repoName, + // eslint-disable-next-line @typescript-eslint/naming-convention + pull_number: pullRequestNumber, + }); + + const existingReview = existingReviewsResponse.data.find((review) => + review.body.startsWith(benchmarkStartTag(benchName)), + ); + + core.debug('findExistingPRReviewId start'); + return existingReview?.id; +} diff --git a/src/comment/leaveCommitComment.ts b/src/comment/leaveCommitComment.ts new file mode 100644 index 000000000..c25b67145 --- /dev/null +++ b/src/comment/leaveCommitComment.ts @@ -0,0 +1,25 @@ +import * as github from '@actions/github'; +import * as core from '@actions/core'; +import { wrapBodyWithBenchmarkTags } from './benchmarkCommentTags'; + +export async function leaveCommitComment( + repoOwner: string, + repoName: string, + commitId: string, + body: string, + commentId: string, + token: string, +) { + core.debug('leaveCommitComment start'); + const client = github.getOctokit(token); + const response = await client.rest.repos.createCommitComment({ + owner: repoOwner, + repo: repoName, + // eslint-disable-next-line @typescript-eslint/naming-convention + commit_sha: commitId, + body: wrapBodyWithBenchmarkTags(commentId, body), + }); + console.log(`Comment was sent to ${response.url}. Response:`, response.status, response.data); + core.debug('leaveCommitComment end'); + return response; +} diff --git a/src/comment/leavePRComment.ts b/src/comment/leavePRComment.ts new file mode 100644 index 000000000..d9bb94f2e --- /dev/null +++ b/src/comment/leavePRComment.ts @@ -0,0 +1,72 @@ +import * as github from '@actions/github'; +import { wrapBodyWithBenchmarkTags } from './benchmarkCommentTags'; +import { findExistingPRReviewId } from './findExistingPRReviewId'; +import * as core from '@actions/core'; + +export async function leavePRComment( + repoOwner: string, + repoName: string, + pullRequestNumber: number, + body: string, + commentId: string, + token: string, +) { + try { + core.debug('leavePRComment start'); + const client = github.getOctokit(token); + + const bodyWithTags = wrapBodyWithBenchmarkTags(commentId, body); + + const existingCommentId = await findExistingPRReviewId( + repoOwner, + repoName, + pullRequestNumber, + commentId, + token, + ); + + if (!existingCommentId) { + core.debug('creating new pr comment'); + const createReviewResponse = await client.rest.pulls.createReview({ + owner: repoOwner, + repo: repoName, + // eslint-disable-next-line @typescript-eslint/naming-convention + pull_number: pullRequestNumber, + event: 'COMMENT', + body: bodyWithTags, + }); + + console.log( + `Comment was created via ${createReviewResponse.url}. Response:`, + createReviewResponse.status, + createReviewResponse.data, + ); + + core.debug('leavePRComment end'); + return createReviewResponse; + } + + core.debug('updating existing pr comment'); + const updateReviewResponse = await client.rest.pulls.updateReview({ + owner: repoOwner, + repo: repoName, + // eslint-disable-next-line @typescript-eslint/naming-convention + pull_number: pullRequestNumber, + // eslint-disable-next-line @typescript-eslint/naming-convention + review_id: existingCommentId, + body: bodyWithTags, + }); + + console.log( + `Comment was updated via ${updateReviewResponse.url}. Response:`, + updateReviewResponse.status, + updateReviewResponse.data, + ); + core.debug('leavePRComment end'); + + return updateReviewResponse; + } catch (err) { + console.log('error', err); + throw err; + } +} diff --git a/src/write.ts b/src/write.ts index 3c71823a7..27e13798e 100644 --- a/src/write.ts +++ b/src/write.ts @@ -7,6 +7,8 @@ import * as git from './git'; import { Benchmark, BenchmarkResult } from './extract'; import { Config, ToolType } from './config'; import { DEFAULT_INDEX_HTML } from './default_index_html'; +import { leavePRComment } from './comment/leavePRComment'; +import { leaveCommitComment } from './comment/leaveCommitComment'; export type BenchmarkSuites = { [name: string]: Benchmark[] }; export interface DataJson { @@ -236,24 +238,15 @@ function buildAlertComment( return lines.join('\n'); } -async function leaveComment(commitId: string, body: string, token: string) { +async function leaveComment(commitId: string, body: string, commentId: string, token: string) { core.debug('Sending comment:\n' + body); const repoMetadata = getCurrentRepoMetadata(); - const repoUrl = repoMetadata.html_url ?? ''; - const client = github.getOctokit(token); - const res = await client.rest.repos.createCommitComment({ - owner: repoMetadata.owner.login, - repo: repoMetadata.name, - // eslint-disable-next-line @typescript-eslint/naming-convention - commit_sha: commitId, - body, - }); - - const commitUrl = `${repoUrl}/commit/${commitId}`; - console.log(`Comment was sent to ${commitUrl}. Response:`, res.status, res.data); + const pr = github.context.payload.pull_request; - return res; + return await (pr?.number + ? leavePRComment(repoMetadata.owner.login, repoMetadata.name, pr.number, body, commentId, token) + : leaveCommitComment(repoMetadata.owner.login, repoMetadata.name, commitId, body, commentId, token)); } async function handleComment(benchName: string, curSuite: Benchmark, prevSuite: Benchmark, config: Config) { @@ -272,7 +265,7 @@ async function handleComment(benchName: string, curSuite: Benchmark, prevSuite: const body = buildComment(benchName, curSuite, prevSuite); - await leaveComment(curSuite.commit.id, body, githubToken); + await leaveComment(curSuite.commit.id, body, `${benchName} Summary`, githubToken); } async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Benchmark, config: Config) { @@ -292,14 +285,13 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be core.debug(`Found ${alerts.length} alerts`); const body = buildAlertComment(alerts, benchName, curSuite, prevSuite, alertThreshold, alertCommentCcUsers); let message = body; - let url = null; if (commentOnAlert) { 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, githubToken); - url = res.data.html_url; + const res = await leaveComment(curSuite.commit.id, body, `${benchName} Alert`, githubToken); + const url = res.data.html_url; message = body + `\nComment was generated at ${url}`; } @@ -364,7 +356,7 @@ function addBenchmarkToDataJson( return prevBench; } -function isRemoteRejectedError(err: unknown) { +function isRemoteRejectedError(err: unknown): err is Error { if (err instanceof Error) { return ['[remote rejected]', '[rejected]'].some((l) => err.message.includes(l)); } @@ -443,7 +435,7 @@ async function writeBenchmarkToGitHubPagesWithRetry( console.log( `Automatically pushed the generated commit to ${ghPagesBranch} branch since 'auto-push' is set to true`, ); - } catch (err: any) { + } catch (err: unknown) { if (!isRemoteRejectedError(err)) { throw err; } diff --git a/test/write.spec.ts b/test/write.spec.ts index fee85a224..121ce517b 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -8,6 +8,7 @@ import { Benchmark } from '../src/extract'; import { DataJson, writeBenchmark } from '../src/write'; import { expect } from '@jest/globals'; import { FakedOctokit, fakedRepos } from './fakedOctokit'; +import { wrapBodyWithBenchmarkTags } from '../src/comment/benchmarkCommentTags'; const ok: (x: any, msg?: string) => asserts x = (x, msg) => { try { @@ -771,7 +772,10 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe // Last line is appended only for failure message const messageLines = caughtError.message.split('\n'); ok(messageLines.length > 0); - const expectedMessage = messageLines.slice(0, -1).join('\n'); + const expectedMessage = wrapBodyWithBenchmarkTags( + 'Test benchmark Alert', + messageLines.slice(0, -1).join('\n'), + ); ok( fakedRepos.spyOpts.length > 0, `len: ${fakedRepos.spyOpts.length}, caught: ${caughtError.message}`,