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.

Sign up for GitHub

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

Jump to bottom

fix: Fix bundle size tracking #29486

Merged
merged 2 commits into from
Jan 9, 2025
Merged

fix: Fix bundle size tracking #29486

merged 2 commits into from
Jan 9, 2025
+175 −0

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jan 7, 2025 •
edited by github-actions bot
Loading

Description

The bundle size tracking was accidentally broken in #29408. This restores the bundle size tracking.

Open in GitHub Codespaces

Related issues

Fixes #29485

Resolves bug introduced by #29408

Manual testing steps

  1. Check the "mv3: Bundle Size Stats" link in the metamaskbot comment
  2. Once this is merged, check the bundle size tracker to ensure it's working again: https://github.com/MetaMask/extension_bundlesize_stats
  • Unfortunately I am not sure how to easily test this on the PR. The tracker is only updated when commits are made to main.

Screenshots/Recordings

N/A

Pre-merge author checklist

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.

@Gudahtt
fix; Fix bundle size tracking
8fa79fb 
The bundle size tracking was accidentally broken in #29408. This
restores the bundle size tracking.
@Gudahtt Gudahtt mentioned this pull request Jan 7, 2025
Merged
7 tasks
@Gudahtt Gudahtt changed the title fix; Fix bundle size tracking fix: Fix bundle size tracking Jan 7, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [e41b583]
Page Load Metrics (1629 ± 116 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint139821671630244117
domContentLoaded137020871606240115
load137921001629242116
domInteractive2292422110
backgroundConnect7110272311
firstReactRender156930189
getState45912157
initialActions01000
loadScripts98317021203208100
setupStore683142110
uiStartup156523811857264127

@Gudahtt Gudahtt marked this pull request as ready for review January 7, 2025 21:50
@Gudahtt Gudahtt requested a review from a team as a code owner January 7, 2025 21:50
Gudahtt
Gudahtt commented Jan 9, 2025
View reviewed changes
test/e2e/mv3-perf-stats/bundle-size.js
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was restored exactly as it was before, apart from the description change here: e41b583

mikesposito
mikesposito reviewed Jan 9, 2025
View reviewed changes
Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we see the section again in this PR's build announce comment?

Gudahtt
Gudahtt commented Jan 9, 2025
View reviewed changes
.circleci/config.yml
@@ -1270,6 +1274,36 @@ jobs:
paths:
- test-artifacts

bundle-size:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step used to also measure some other MV3-related stats, which have since been removed. It has been renamed to reflect that it now just measures bundle size. The steps are identical to what they were before, except with the deleted tests removed.

Gudahtt
Gudahtt commented Jan 9, 2025
View reviewed changes
package.json
@@ -28,6 +28,7 @@
"start:test:mv2:flask": "ENABLE_MV3=false yarn start:test:flask --apply-lavamoat=false --snow=false",
"start:test:mv2": "ENABLE_MV3=false BLOCKAID_FILE_CDN=static.cx.metamask.io/api/v1/confirmations/ppom yarn start:test --apply-lavamoat=false --snow=false",
"benchmark:chrome": "SELENIUM_BROWSER=chrome ts-node test/e2e/benchmark.js",
"bundle-size": "SELENIUM_BROWSER=chrome ts-node test/e2e/mv3-perf-stats/bundle-size.js",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old version of this command ran a few different types of tests. When restoring this, I renamed it to bundle-size because it's now only measuring bundle size.

@Gudahtt
Copy link
Member Author

Gudahtt commented Jan 9, 2025

Shouldn't we see the section again in this PR's build announce comment?

We do, it is here:

mv3: Bundle Size Stats

@mikesposito
Copy link
Member

mikesposito commented Jan 9, 2025 •
edited
Loading

Oh, I see sorry, the bundle size commitment to the external repo was also disabled and so we also don't see the Bundle Size section in metamaskbot's comment, gotcha.

mikesposito
mikesposito approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Gudahtt
Copy link
Member Author

Gudahtt commented Jan 9, 2025 •
edited
Loading

Oh sorry you meant the part where it says whether size has increased or not. Sorry I hadn't noticed that was missing, but you're right, it appears to be due to the lack of data on the main branch commit in the bundle size tracker. Seeing this error from the logs:

Error constructing bundle size diffs results: 'TypeError: Cannot read properties of undefined (reading 'background')'

The value that is undefined is the merge base commit.

This should fix that going forward as well, for new main commits at least.

github-merge-queue bot pushed a commit that referenced this pull request Jan 9, 2025
@Gudahtt
ci: Migrate metamaskbot PR comment (#29373)
87c524e 
## **Description**

Migrate the `metamaskbot` PR comment from CircleCI to GitHub Actions.
CircleCI is still used for artifact storage for linked artifacts.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29373?quickstart=1)

## **Related issues**

Relates to #28572

These changes were extracted from #29256

## **Manual testing steps**

* Test that all links in the `metamaskbot` comment work correctly,
except those that are already broken. Links already broken on `main`
include:
  * Some build links (fixed in #29403 ):
    - Beta builds
    - Firefox test build, and Firefox flask-test build
  * MV3 performance stats reports (fixed in #29408 ):
    * mv3: Background Module Init Stats (link works, page is broken)
    * mv3: UI Init Stats (link works, page is broken)
    * mv3: Module Load Stats (link works, page is broken)
  * Coverage report (fixed by #29410)
  * mv3 bundle size stats (fixed by #29486)

## **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.
desi
desi approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@desi desi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through all the comments and checked that I also see the bundle size in the comment

@Gudahtt Gudahtt added this pull request to the merge queue Jan 9, 2025
Merged via the queue into main with commit edaae77 Jan 9, 2025
80 of 81 checks passed
@Gudahtt Gudahtt deleted the restore-bundle-size-tracking branch January 9, 2025 23:10
@github-actions github-actions bot locked and limited conversation to collaborators Jan 9, 2025
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Jan 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Welbert5753 Welbert5753 Welbert5753 approved these changes

@mikesposito mikesposito mikesposito approved these changes

@FrederikBolding FrederikBolding FrederikBolding approved these changes

@desi desi desi approved these changes

Assignees
No one assigned
Labels
release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-extension-platform team-wallet-framework
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Fix bundle size tracking
6 participants
@Gudahtt @metamaskbot @mikesposito @desi @FrederikBolding @Welbert5753