-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug]: Bundle size diffs appear to be incorrect #29858
Labels
area-CI
release-12.12.0
Issue or pull request that will be included in release 12.12.0
team-extension-platform
type-bug
Comments
Gudahtt
added a commit
that referenced
this issue
Jan 22, 2025
Fix bundle size diffs that are included in `metamaskbot` comments and tracked historically in https://github.com/MetaMask/extension_bundlesize_stats The diffs were incorrect because the "merge base" commit was wrong. The merge base commit is the commit we're comparing the branch against. We were calculating this using `git merge-base origin/main HEAD`, which locally does give the correct answer. But in GitHub Actions, HEAD was not set to the tip of the branch. Instead it was a new comment generated during the checkout step, representing the target branch merged into the current branch. This resulted in the "merge base" always being the tip of `main` (at the time of this workflow run). This in turn resulted in the "bundle diff" including changes that were already made on `main` Fixes #29858.
Gudahtt
added a commit
that referenced
this issue
Jan 22, 2025
Fix bundle size diffs that are included in `metamaskbot` comments and tracked historically in https://github.com/MetaMask/extension_bundlesize_stats The diffs were incorrect because the "merge base" commit was wrong. The merge base commit is the commit we're comparing the branch against. We were calculating this using `git merge-base origin/main HEAD`, which locally does give the correct answer. But in GitHub Actions, HEAD was not set to the tip of the branch. Instead it was a new comment generated during the checkout step, representing the target branch merged into the current branch. This resulted in the "merge base" always being the tip of `main` (at the time of this workflow run). This in turn resulted in the "bundle diff" including changes that were already made on `main` Fixes #29858.
github-merge-queue bot
pushed a commit
that referenced
this issue
Jan 24, 2025
## **Description** Fix bundle size diffs that are included in `metamaskbot` comments. The diffs were incorrect because the "merge base" commit was wrong. The merge base commit is the commit we're comparing the branch against. We were calculating this using `git merge-base origin/main HEAD`, which locally does give the correct answer. But in GitHub Actions, HEAD was not set to the tip of the branch. Instead it was a new comment generated during the checkout step, representing the target branch merged into the current branch. This resulted in the "merge base" always being the tip of `main` (at the time of this workflow run). This in turn resulted in the "bundle diff" including changes that were already made on `main` [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29862?quickstart=1) ## **Related issues** Fixes #29858. ## **Manual testing steps** I created a second draft PR to add diagnostic logs for testing this: #29859 You can see an example of these logs here: https://github.com/MetaMask/metamask-extension/actions/runs/12917863547/job/36025202434?pr=29859 Note that the `HEAD` resolves to a totally different commit than the real head of the branch (which is accessible as `pull_request.head.sha`, resulting in a different merge base. ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] 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). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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.
metamaskbot
added
the
release-12.12.0
Issue or pull request that will be included in release 12.12.0
label
Jan 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area-CI
release-12.12.0
Issue or pull request that will be included in release 12.12.0
team-extension-platform
type-bug
Describe the bug
We've noticed recently that bundle size diffs appear to be incorrect since migrating to GitHub Actions. Specifically, we're comparing bundle sizes to the wrong commit (we're supposed to use the merge-base for the current PR, but in practice it's comparing using some other commit, unclear why)
Expected behavior
Bundle size diffs shown in
metamaskbot
comments should be reflective of the impact the given PR has on bundle size.Screenshots/Recordings
No response
Steps to reproduce
Example: #29852 (comment)
Error messages or log output
Detection stage
On main branch
Version
main
Build type
Other (please specify exactly where you obtained this build in "Additional Context" section)
Browser
Other (please elaborate in the "Additional Context" section)
Operating system
Other (please elaborate in the "Additional Context" section)
Hardware wallet
No response
Additional context
No response
Severity
No response
The text was updated successfully, but these errors were encountered: