Skip to content

test: throw on foundryup installation issues #32641

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

Closed
wants to merge 5 commits into from

Conversation

davidmurdoch
Copy link
Contributor

@davidmurdoch davidmurdoch commented May 7, 2025

Description

We now throw if an existing anvil install cannot be removed in order to install a new one.

Additionally, we now throw on installation errors and on the cleanup step in order to reveal problems with filesystem access. The new errors can look something like this (I had to had to mess with the file system in order to get it to error multiple ways at once):

Error: AggregateError: This is a bug; you should report it.
    at extractFrom (/home/david/code/metamask-extension/test/helpers/foundry/extract.ts:98:13)
    at async checkAndDownloadBinaries (/home/david/code/metamask-extension/test/helpers/foundry/foundryup.ts:96:7)
    at async downloadAndInstallFoundryBinaries (/home/david/code/metamask-extension/test/helpers/foundry/foundryup.ts:173:30) {
  [errors]: [
    Error: ENOTEMPTY: directory not empty, rename '/home/david/code/metamask-extension/.metamask/cache/e8263365c086121ae7aba67089b92654b5c38a8c27f0562c4d1b6ba97100f4fa.downloading' -> '/home/david/code/metamask-extension/.metamask/cache/e8263365c086121ae7aba67089b92654b5c38a8c27f0562c4d1b6ba97100f4fa'
        at async rename (node:internal/fs/promises:782:10)
        at async extractFrom (/home/david/code/metamask-extension/test/helpers/foundry/extract.ts:79:5)
        at async checkAndDownloadBinaries (/home/david/code/metamask-extension/test/helpers/foundry/foundryup.ts:96:7)
        at async downloadAndInstallFoundryBinaries (/home/david/code/metamask-extension/test/helpers/foundry/foundryup.ts:173:30) {
      errno: -39,
      code: 'ENOTEMPTY',
      syscall: 'rename',
      path: '/home/david/code/metamask-extension/.metamask/cache/e8263365c086121ae7aba67089b92654b5c38a8c27f0562c4d1b6ba97100f4fa.downloading',
      dest: '/home/david/code/metamask-extension/.metamask/cache/e8263365c086121ae7aba67089b92654b5c38a8c27f0562c4d1b6ba97100f4fa'
    },
    [Error: ENOTDIR: not a directory, scandir '/home/david/code/metamask-extension/.metamask/cache/e8263365c086121ae7aba67089b92654b5c38a8c27f0562c4d1b6ba97100f4fa/anvil'] {
      errno: -20,
      code: 'ENOTDIR',
      syscall: 'scandir',
      path: '/home/david/code/metamask-extension/.metamask/cache/e8263365c086121ae7aba67089b92654b5c38a8c27f0562c4d1b6ba97100f4fa/anvil'
    }
  ]
}

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

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.

Copy link
Contributor

github-actions bot commented May 7, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-extension-platform Extension Platform team label May 7, 2025
// if things fail for any reason try to clean up a bit. it is very important
// to not leave `dir` behind, as its existence is a signal that the binaries
// are installed.
await Promise.all([rm(tempDir, rmOpts), rm(dir, rmOpts)]).catch(noop);
throw e;
const rmErrors = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same logic as before, but ALSO throws errors for the clean up rm calls.

// this directory shouldn't exist, but if two simultaneous `yarn foundryup`
// processes are running, it might. Last process wins, so we remove other
// `dir`s just in case.
await rm(dir, rmOpts);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change might prevent rename issues, even though it should be impossible to get into this function is dir already exists.

@metamaskbot
Copy link
Collaborator

Builds ready [42a87e6]
UI Startup Metrics (1230 ± 68 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1230111414996812781354
load107796612556211091220
domContentLoaded107094812506311021213
domInteractive17144951625
firstPaint71384126443510871214
backgroundConnect84546721
firstReactRender21156072236
getState1664292232
initialActions001001
loadScripts82371798760858952
setupStore85233915
WebpackHomeuiStartup21731713256617223022385
load16911326200413017581927
domContentLoaded16851322199812917541923
domInteractive161172111345
firstPaint1596133158178282
backgroundConnect299364363451
firstReactRender17054385111301357
getState144163221527
initialActions316145
loadScripts16801321197512917531900
setupStore197266262133
FirefoxBrowserifyHomeuiStartup13531173178411714141572
load12061039157310812721431
domContentLoaded12061039157310812721431
domInteractive973719126103147
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect2113138162032
firstReactRender23195772335
getState11421923822
initialActions001001
loadScripts11871025154110712541381
setupStore64243610
WebpackHomeuiStartup16021417225017516742015
load13681203190514414581648
domContentLoaded13681203190414414571647
domInteractive82382412791126
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect2715296282648
firstReactRender37306363949
getState1054771032
initialActions001011
loadScripts13451184177313614361622
setupStore18558565932
Benchmark value 1077 exceeds gate value 1070 for chrome browserify home mean load
Benchmark value 1070 exceeds gate value 1061 for chrome browserify home mean domContentLoaded
Benchmark value 16 exceeds gate value 15 for chrome browserify home mean getState
Benchmark value 1220 exceeds gate value 1190 for chrome browserify home p95 load
Benchmark value 1214 exceeds gate value 1180 for chrome browserify home p95 domContentLoaded
Benchmark value 1215 exceeds gate value 1180 for chrome browserify home p95 firstPaint
Benchmark value 22 exceeds gate value 18 for chrome browserify home p95 backgroundConnect
Benchmark value 953 exceeds gate value 940 for chrome browserify home p95 loadScripts
Benchmark value 28 exceeds gate value 26 for firefox webpack home mean backgroundConnect
Benchmark value 18 exceeds gate value 13 for firefox webpack home mean setupStore
Benchmark value 2015 exceeds gate value 1935 for firefox webpack home p95 uiStartup
Benchmark value 32 exceeds gate value 28 for firefox webpack home p95 setupStore
Sum of mean exceeds: 24ms | Sum of p95 exceeds: 200ms
Sum of all benchmark exceeds: 224ms

Bundle size diffs [🚀 Bundle size reduced!]
  • background: -7.34 KiB (-0.14%)
  • ui: 67.74 KiB (0.96%)
  • common: -19.99 KiB (-0.21%)

@metamaskbot
Copy link
Collaborator

Builds ready [d3c5cfe]
UI Startup Metrics (1246 ± 93 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1246108915519312871461
load108295213718011161251
domContentLoaded107594813668011111240
domInteractive17134541726
firstPaint69381137544010861232
backgroundConnect93315822
firstReactRender20155172037
getState1554182232
initialActions001001
loadScripts828702109673865962
setupStore75142811
WebpackHomeuiStartup21701760250116722922389
load16801375215413217581905
domContentLoaded16741372215013117541895
domInteractive15115381341
firstPaint1606231056186286
backgroundConnect271091153265
firstReactRender17253373106286352
getState154319311525
initialActions316145
loadScripts16691370214013117481879
setupStore206299292233
FirefoxBrowserifyHomeuiStartup13511167194312614201582
load12041051179911512781422
domContentLoaded12031051179911512781422
domInteractive1024918125115153
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect21136792142
firstReactRender24205662437
getState84385816
initialActions001001
loadScripts11851037177011412541391
setupStore10446246611
WebpackHomeuiStartup15641374215414416331881
load13391179189813214071600
domContentLoaded13391179189713214071599
domInteractive82353183185133
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect22146172336
firstReactRender36305053846
getState12527527925
initialActions002111
loadScripts13191163187513213841584
setupStore85253816
Benchmark value 1247 exceeds gate value 1234 for chrome browserify home mean uiStartup
Benchmark value 1083 exceeds gate value 1070 for chrome browserify home mean load
Benchmark value 1075 exceeds gate value 1061 for chrome browserify home mean domContentLoaded
Benchmark value 1461 exceeds gate value 1365 for chrome browserify home p95 uiStartup
Benchmark value 1251 exceeds gate value 1190 for chrome browserify home p95 load
Benchmark value 1241 exceeds gate value 1180 for chrome browserify home p95 domContentLoaded
Benchmark value 1232 exceeds gate value 1180 for chrome browserify home p95 firstPaint
Benchmark value 22 exceeds gate value 18 for chrome browserify home p95 backgroundConnect
Benchmark value 962 exceeds gate value 940 for chrome browserify home p95 loadScripts
Benchmark value 11 exceeds gate value 9 for firefox browserify home mean setupStore
Sum of mean exceeds: 42ms | Sum of p95 exceeds: 296ms
Sum of all benchmark exceeds: 338ms

Bundle size diffs
  • background: 0 Bytes (0%)
  • ui: 0 Bytes (0%)
  • common: 0 Bytes (0%)

@davidmurdoch
Copy link
Contributor Author

moved to MetaMask/core#5810

@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-extension-platform Extension Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants