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

test: remove defaultGanacheOptions #30728

Merged
merged 3 commits into from
Mar 7, 2025
Merged

Conversation

seaona
Copy link
Contributor

@seaona seaona commented Mar 4, 2025

Description

In this PR we remove the defaultGanacheOptions variable from all specs. 2 reasons:

  • the default options, are already handled in the node class level, so no need to pass default options in any spec, only custom options
  • we are going to use anvil, or other nodes, so this custom variable makes no sense anymore

Note: skip-e2e-quality-gate is added because we modify several spec files, which can take long to run, reaching timeout as they'll all be rerun + 5 times extra, but there should be no functional change, so e2e should continue to work as before

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4348

Manual testing steps

  1. All specs should continue to work normally

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 Mar 4, 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-qa QA team label Mar 4, 2025
@seaona seaona added the area-qa Relating to QA work (Quality Assurance) label Mar 4, 2025
@seaona seaona self-assigned this Mar 4, 2025
@seaona seaona marked this pull request as ready for review March 4, 2025 17:26
@metamaskbot
Copy link
Collaborator

Builds ready [6fc55bf]
Page Load Metrics (1880 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26522541811393189
domContentLoaded16402182184916981
load16652260188017685
domInteractive287842157
backgroundConnect1379362010
firstReactRender1478392512
getState56519189
initialActions01000
loadScripts12221621138813866
setupStore887172010
uiStartup189227162159237114
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

accounts: [
{
secretKey: PRIVATE_KEY,
balance: convertETHToHexGwei(DEFAULT_GANACHE_ETH_BALANCE_DEC),
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 is '25ETH' to the account corresponding to the default SRP (both ganache and anvil), so it can be removed, as it's in the node class level, and adding this to a spec has no effect

accounts: [
{
secretKey: defaultGanacheOptions.accounts[0].secretKey,
balance: initialBalanceInHex,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no reason at all to start with a random balance, as this is not checked in the test, which we just check metric related topics, so we can remove it safely

@DDDDDanica
Copy link
Contributor

LGTM !

const defaultGanacheOptionsForType2Transactions = {
...defaultGanacheOptions,
// EVM version that supports type 2 transactions (EIP1559)
hardfork: 'london',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You might be able to remove this one as well right now. The default hardfork in ganache already supports type2 transactions.

Copy link
Contributor Author

@seaona seaona Mar 4, 2025

Choose a reason for hiding this comment

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

good observation !! So at the time the Ganache class was defined, it was set as hardfork: 'muirGlacier', as default, so the majority of specs are using that, unless specifically passing the london flag. In Anvil I also set the same hardfork as default, as several of specs were breaking otherwise, as were relying to type 1 (gas checks and conf screen). We could change that and use london as default in the class, as I guess most of the cases should use type 2 now, so it makes more sense.
I'll take a note on that, if that make sense I can update it in a separate PR, as this change might need some other spec tweaks? I'll wait on your feedback, thank you in advance 🙏

Copy link
Contributor Author

@seaona seaona Mar 6, 2025

Choose a reason for hiding this comment

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

ℹ️ Added a task here for me to address this next: https://github.com/MetaMask/MetaMask-planning/issues/4368

@seaona seaona added this pull request to the merge queue Mar 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 6, 2025
@seaona seaona added this pull request to the merge queue Mar 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 6, 2025
@seaona seaona enabled auto-merge March 7, 2025 09:45
@seaona seaona added this pull request to the merge queue Mar 7, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [a0e8022]
Page Load Metrics (1745 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33423091679342164
domContentLoaded15132288171015173
load15352313174515173
domInteractive26190483617
backgroundConnect137640178
firstReactRender146622136
getState65118157
initialActions00000
loadScripts11001720127311756
setupStore8521294
uiStartup17162422196413766
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Merged via the queue into main with commit 9811f62 Mar 7, 2025
74 checks passed
@seaona seaona deleted the default-ganache-opts-removal branch March 7, 2025 11:08
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2025
@metamaskbot metamaskbot added the release-12.15.0 Issue or pull request that will be included in release 12.15.0 label Mar 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-qa Relating to QA work (Quality Assurance) release-12.15.0 Issue or pull request that will be included in release 12.15.0 skip-e2e-quality-gate team-qa QA team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants