Skip to content

test: Fix invalid fixture builder #29783

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

Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jan 17, 2025

Description

Two of the permission fixture builders were adding permissions for accounts that do not exist. They have been updated to only grant permissions for the selected account, which is the only account guaranteed to exist in the default fixture.

Open in GitHub Codespaces

Related issues

This was extracted from #27847

Manual testing steps

See that E2E tests still pass

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.

Two of the permission fixture builders were adding permissions for
accounts that do not exist. They have been updated to only grant
permissions for the selected account, which is the only account
guaranteed to exist in the default fixture.
Copy link
Contributor

@adonesky1 adonesky1 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 Gudahtt marked this pull request as ready for review January 17, 2025 14:46
@@ -594,10 +591,7 @@ class FixtureBuilder {
caveats: [
{
type: 'restrictReturnedAccounts',
value: [
'0x5cfe73b6021e818b776b421b1c4db2474086a7e1',
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 is the default selected account. I can update this in a separate PR to use the variable here instead.

@metamaskbot
Copy link
Collaborator

Builds ready [6ecf015]
Page Load Metrics (1610 ± 87 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25022881497427205
domContentLoaded14202279159418288
load14392290161018187
domInteractive258840189
backgroundConnect56418199
firstReactRender1581432613
getState45010126
initialActions00000
loadScripts9811686116414570
setupStore6569115
uiStartup16442479181919292
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -135 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt enabled auto-merge January 17, 2025 15:11
@Gudahtt Gudahtt added this pull request to the merge queue Jan 17, 2025
Merged via the queue into main with commit 582ec93 Jan 17, 2025
82 checks passed
@Gudahtt Gudahtt deleted the remove-non-existent-accounts-from-permission-fixture-builders branch January 17, 2025 17:05
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2025
@metamaskbot metamaskbot added the release-12.12.0 Issue or pull request that will be included in release 12.12.0 label Jan 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.12.0 Issue or pull request that will be included in release 12.12.0 team-extension-platform Extension Platform team team-wallet-api-platform team-wallet-framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants