Skip to content

Commit 54b473b

Browse files
authored
build: commit hash parameter from Compare and comment step in Page Load Benchmark workflow (#35554)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** The "Compare and comment" step in the page load benchmark workflow is currently posting comments with the wrong commit hash. This occurs because: - **Workflow Architecture Issue**: The `benchmark-pr.yml` is a `workflow_call` workflow that gets called by `main.yml` - **Context Variable Mismatch**: When a workflow is called by another workflow, `github.sha` refers to the commit that triggered the calling workflow (`main.yml`), not necessarily the latest commit on the PR branch - **Stale Commit Reference**: This results in the benchmark comment showing an outdated commit hash instead of the actual commit being benchmarked The "Upload benchmark results step" is **_NOT affected_**, as this step correctly uses the proper commit hash when the PR is merged to `main` , as it runs in the context of the `main` branch workflow. This PR separates and calls the workflow for page load benchmark comment at a point we are sure the commit hash parameter from the "Compare and comment" step is the correct one. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/35554?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/WAPI-692 ## **Manual testing steps** 1. No user facing changes, so no testing steps required. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <img width="990" height="950" alt="Screenshot 2025-09-06 at 16 25 30" src="https://github.com/user-attachments/assets/356d35d9-abf3-479b-ac60-0921ac30e2f2" /> <!-- [screenshots/recordings] --> ### **After** <img width="1033" height="961" alt="Screenshot 2025-09-06 at 16 25 43" src="https://github.com/user-attachments/assets/732e9eb0-6337-4587-8346-251fcce45f1b" /> <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent 2945564 commit 54b473b

File tree

5 files changed

+51
-15
lines changed

5 files changed

+51
-15
lines changed
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
name: 'Page load benchmark comment'
2+
description: 'Download benchmark results, create directory structure, and comment on PR'
3+
inputs:
4+
head-commit-hash:
5+
description: 'The head commit hash to pass to the benchmark comment script'
6+
required: true
7+
pr-comment-token:
8+
description: 'GitHub token for commenting on PR'
9+
required: true
10+
runs:
11+
using: 'composite'
12+
steps:
13+
- name: Download page load benchmark results
14+
uses: actions/download-artifact@v4
15+
with:
16+
name: benchmark-results
17+
18+
- name: Create expected page load benchmark results directory structure
19+
shell: bash
20+
run: |
21+
mkdir -p test-artifacts/benchmarks
22+
mv benchmark-results.json test-artifacts/benchmarks/benchmark-results.json
23+
24+
- name: Page load benchmark compare and comment
25+
shell: bash
26+
run: yarn tsx development/page-load-benchmark-pr-comment.ts
27+
env:
28+
PR_COMMENT_TOKEN: ${{ inputs.pr-comment-token }}
29+
OWNER: ${{ github.repository_owner }}
30+
REPOSITORY: ${{ github.event.repository.name }}
31+
PR_NUMBER: ${{ github.event.pull_request.number }}
32+
HEAD_COMMIT_HASH: ${{ inputs.head-commit-hash }}

.github/workflows/main.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,11 +240,11 @@ jobs:
240240
s3-bucket: ${{ vars.AWS_S3_BUCKET }}/${{ github.event.repository.name }}/${{ github.run_id }}/bundle-size
241241
path: test-artifacts/chrome
242242

243-
benchmark-pr:
243+
page-load-benchmark-pr:
244244
needs:
245245
- build-dist-browserify
246246
if: ${{ github.event_name == 'pull_request' || (github.event_name == 'push' && github.ref_name == 'main') }}
247-
uses: ./.github/workflows/benchmark-pr.yml
247+
uses: ./.github/workflows/page-load-benchmark-pr.yml
248248
with:
249249
browser-loads: '10'
250250
page-loads: '10'

.github/workflows/benchmark-pr.yml renamed to .github/workflows/page-load-benchmark-pr.yml

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: Benchmark PR Performance
1+
name: Page load benchmark
22

33
on:
44
workflow_call:
@@ -20,7 +20,7 @@ on:
2020
required: true
2121

2222
jobs:
23-
benchmark-pr:
23+
page-load-benchmark-pr:
2424
name: Run Page Load Benchmarks
2525
runs-on: ubuntu-latest
2626
env:
@@ -48,17 +48,13 @@ jobs:
4848
- name: Run page load benchmarks
4949
run: yarn test:e2e:benchmark
5050

51-
- name: Compare and comment
52-
if: ${{ github.event_name == 'pull_request' }}
53-
run: yarn tsx development/benchmark-pr-comment.ts
54-
env:
55-
PR_COMMENT_TOKEN: ${{ secrets.PR_COMMENT_TOKEN }}
56-
OWNER: ${{ github.repository_owner }}
57-
REPOSITORY: ${{ github.event.repository.name }}
58-
PR_NUMBER: ${{ github.event.pull_request.number }}
59-
GITHUB_SHA: ${{ github.event.pull_request.head.sha }}
51+
- name: Upload benchmark results (github artifacts)
52+
uses: actions/upload-artifact@v4
53+
with:
54+
name: benchmark-results
55+
path: test-artifacts/benchmarks/benchmark-results.json
6056

61-
- name: Upload benchmark results
57+
- name: Upload benchmark results (historical data)
6258
if: github.ref == 'refs/heads/main'
6359
run: .github/scripts/benchmark-stats-commit.sh
6460
env:

.github/workflows/publish-prerelease.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,13 @@ jobs:
6060
echo "MERGE_BASE_COMMIT_HASH=${merge_base_commit_hash}" >> "${GITHUB_ENV}"
6161
echo "The merge base commit hash is '${merge_base_commit_hash}'"
6262
63+
- name: Page load benchmark comment
64+
if: ${{ github.event_name == 'pull_request' }}
65+
uses: ./.github/actions/page-load-benchmark-comment
66+
with:
67+
head-commit-hash: ${{ env.HEAD_COMMIT_HASH }}
68+
pr-comment-token: ${{ secrets.PR_COMMENT_TOKEN }}
69+
6370
- name: Publish prerelease
6471
if: ${{ env.MERGE_BASE_COMMIT_HASH && env.PR_NUMBER && env.PR_COMMENT_TOKEN && vars.AWS_CLOUDFRONT_URL }}
6572
run: yarn tsx ./development/metamaskbot-build-announce.ts

development/benchmark-pr-comment.ts renamed to development/page-load-benchmark-pr-comment.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,7 @@ function generateBenchmarkComment(
534534
* @throws {Error} When GitHub API request fails or required environment variables are missing
535535
*/
536536
async function main(): Promise<void> {
537-
const { PR_COMMENT_TOKEN, OWNER, REPOSITORY, PR_NUMBER } =
537+
const { PR_COMMENT_TOKEN, OWNER, REPOSITORY, PR_NUMBER, HEAD_COMMIT_HASH } =
538538
process.env as Record<string, string>;
539539
const N_COMMITS = 10;
540540

@@ -568,6 +568,7 @@ async function main(): Promise<void> {
568568
return;
569569
}
570570

571+
benchmarkData.commit = HEAD_COMMIT_HASH;
571572
const referenceData = await fetchLatestMainBenchmarkData(N_COMMITS);
572573

573574
const commentBody = generateBenchmarkComment(benchmarkData, referenceData);

0 commit comments

Comments
 (0)