Skip to content

Commit

Permalink
feat: Comment on PR and auto update comment (#223)
Browse files Browse the repository at this point in the history
* create PR comment when PR is available
* auto-update existing PR comment
* add comment-on-alert and gh token to ci jobs
  • Loading branch information
ktrz authored Mar 28, 2024
1 parent a6c4f1c commit f65fed8
Show file tree
Hide file tree
Showing 9 changed files with 178 additions and 23 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci-results-repo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
22 changes: 21 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## Unreleased
- **fix** Rust benchmarks not comparing to baseline (#235)
- **feat** Comment on PR and auto update comment (#223)

<a name="v1.19.3"></a>
# [v1.19.3](https://github.com/benchmark-action/github-action-benchmark/releases/tag/v1.19.3) - 02 Feb 2024
Expand Down
13 changes: 13 additions & 0 deletions src/comment/benchmarkCommentTags.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const BENCHMARK_COMMENT_TAG = 'github-benchmark-action-comment';

export function benchmarkStartTag(commentId: string) {
return `<!-- ${BENCHMARK_COMMENT_TAG}(start): ${commentId} -->`;
}

export function benchmarkEndTag(commentId: string) {
return `<!-- ${BENCHMARK_COMMENT_TAG}(end): ${commentId} -->`;
}

export function wrapBodyWithBenchmarkTags(commentId: string, body: string) {
return `${benchmarkStartTag(commentId)}\n${body}\n${benchmarkEndTag(commentId)}`;
}
28 changes: 28 additions & 0 deletions src/comment/findExistingPRReviewId.ts
Original file line number Diff line number Diff line change
@@ -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;
}
25 changes: 25 additions & 0 deletions src/comment/leaveCommitComment.ts
Original file line number Diff line number Diff line change
@@ -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;
}
72 changes: 72 additions & 0 deletions src/comment/leavePRComment.ts
Original file line number Diff line number Diff line change
@@ -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;
}
}
32 changes: 12 additions & 20 deletions src/write.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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}`;
}

Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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;
}
Expand Down
6 changes: 5 additions & 1 deletion test/write.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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}`,
Expand Down

0 comments on commit f65fed8

Please sign in to comment.