Skip to content
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

Closed
Gudahtt opened this issue Jan 22, 2025 · 0 comments · Fixed by #29862
Closed

[Bug]: Bundle size diffs appear to be incorrect #29858

Gudahtt opened this issue Jan 22, 2025 · 0 comments · Fixed by #29862
Assignees
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
Copy link
Member

Gudahtt commented Jan 22, 2025

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

@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Jan 22, 2025
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Jan 22, 2025
@Gudahtt Gudahtt self-assigned this Jan 22, 2025
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.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Jan 24, 2025
@metamaskbot 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
Projects
Archived in project
Status: Fixed
Development

Successfully merging a pull request may close this issue.

2 participants