test: fix stx opt-in opt-out fixture #33264
Open
+49
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
seaona
commented
May 29, 2025
| @@ -845,20 +845,14 @@ class FixtureBuilder { | |||
| }); | |||
| } | |||
|
|
|||
| withPreferencesControllerSmartTransactionsOptedIn() { | |||
There was a problem hiding this comment.
we don't need this method as the opt in status is already in the default fixture, or it is set to true by default in the onboarding case
seaona
commented
May 29, 2025
| /** | ||
| * @deprecated this method should not be used, as the `smartTransactionsOptInStatus` value is overridden by the migration 135 | ||
| * Use the step by step flow to disable this setting. | ||
| */ | ||
| withPreferencesControllerSmartTransactionsOptedOut() { |
There was a problem hiding this comment.
ℹ️ leaving the method with the @deprecated info, so this is not used.
I could have deleted it, but I'm afraid this can be re-introduced again, so leaving this here for a while before removing it completely.
Side: removed the tokenNetworkFilter as it's irrelevant here
seaona
commented
May 29, 2025
| @@ -26,7 +26,6 @@ describe('Gas Fee Tokens - Smart Transactions', function (this: Suite) { | |||
| dapp: true, | |||
| fixtures: new FixtureBuilder({ inputChainId: CHAIN_IDS.MAINNET }) | |||
| .withPermissionControllerConnectedToTestDapp() | |||
| .withPreferencesControllerSmartTransactionsOptedIn() | |||
There was a problem hiding this comment.
this is already true by default
seaona
commented
May 29, 2025
| @@ -84,7 +83,6 @@ describe('Gas Fee Tokens - Smart Transactions', function (this: Suite) { | |||
| dapp: true, | |||
| fixtures: new FixtureBuilder({ inputChainId: CHAIN_IDS.MAINNET }) | |||
| .withPermissionControllerConnectedToTestDapp() | |||
| .withPreferencesControllerSmartTransactionsOptedIn() | |||
There was a problem hiding this comment.
this is already true by default
seaona
commented
May 29, 2025
| @@ -27,7 +27,6 @@ async function withFixturesForSmartTransactions( | |||
| { | |||
| fixtures: new FixtureBuilder() | |||
| .withPermissionControllerConnectedToTestDapp() | |||
| .withPreferencesControllerSmartTransactionsOptedIn() | |||
There was a problem hiding this comment.
this is already true by default
Builds ready [1e55823]
UI Startup Metrics (1199 ± 60 ms)
Benchmark value 46 exceeds gate value 32 for chrome webpack home mean setupStore Benchmark value 2501 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 310 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 1635 exceeds gate value 1615 for firefox webpack home mean uiStartup Benchmark value 1402 exceeds gate value 1380 for firefox webpack home mean load Benchmark value 1402 exceeds gate value 1380 for firefox webpack home mean domContentLoaded Benchmark value 45 exceeds gate value 38 for firefox webpack home mean firstReactRender Benchmark value 1381 exceeds gate value 1360 for firefox webpack home mean loadScripts Benchmark value 2096 exceeds gate value 1935 for firefox webpack home p95 uiStartup Benchmark value 1693 exceeds gate value 1660 for firefox webpack home p95 load Benchmark value 1693 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded Benchmark value 53 exceeds gate value 50 for firefox webpack home p95 firstReactRender Benchmark value 1672 exceeds gate value 1630 for firefox webpack home p95 loadScripts Sum of mean exceeds: 106ms | Sum of p95 exceeds: 570ms Sum of all benchmark exceeds: 676ms Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [196b546]
UI Startup Metrics (1193 ± 54 ms)
Benchmark value 39 exceeds gate value 32 for chrome webpack home mean setupStore Benchmark value 302 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 43 exceeds gate value 38 for firefox webpack home mean firstReactRender Sum of mean exceeds: 12ms | Sum of p95 exceeds: 242ms Sum of all benchmark exceeds: 254ms Bundle size diffs [🚀 Bundle size reduced!]
|
jvbriones
previously approved these changes
May 29, 2025
| @@ -116,6 +116,21 @@ describe('Switch Ethereum Chain for two dapps', function () { | |||
| async ({ driver }) => { | |||
| await unlockWallet(driver); | |||
|
|
|||
| // disable smart transactions step by step | |||
| // we cannot use fixtures because migration 135 overrides the opt in value to true | |||
| await driver.clickElement( | |||
There was a problem hiding this comment.
Maybe nice to use the toggleSmartTransactions function from the already existing page test/e2e/page-objects/pages/settings/advanced-settings.ts here?
There was a problem hiding this comment.
good catch. I haven't used any page object related methods in this spec, because this test is not migrated yet and we have a weak validation for "knowing" which tests are using page objects, by checking imports.
If I added this method, it would "seem" as we are using page objects here, but that won't be accurate.
That's why I continued the legacy approach from the test, and once this is migrated altogether, the validation will accurately show that's a migrated test.
We could improve the logic for that validation in a separate PR, to handle cases where "mixed-approaches" are made
There was a problem hiding this comment.
ℹ️ Adding the flow for the spec using page objects only (eip7702 one), and we can use this flow to this specific legacy test once that is migrated
Builds ready [7425610]
UI Startup Metrics (1216 ± 71 ms)
Benchmark value 37 exceeds gate value 32 for chrome webpack home mean setupStore Benchmark value 2460 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 259 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 40 exceeds gate value 38 for firefox webpack home mean firstReactRender Sum of mean exceeds: 7ms | Sum of p95 exceeds: 207ms Sum of all benchmark exceeds: 214ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
racitores
approved these changes
Jun 2, 2025
cmd-ob
approved these changes
Jun 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR does 2 things:
withPreferencesControllerSmartTransactionsOptedInfrom fixtures, as this value is always true by default (check the value default fixture, and for the onboarding fixture, there is logic in the app which sets this to true)withPreferencesControllerSmartTransactionsOptedOutfrom fixtures, as this value is always overridden by the migration 135 which will set it totrueif the value isfalse.After that, I've updated the e2e accordingly.
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/MMQA-605?atlOrigin=eyJpIjoiNDEyNWNjY2I4ZmJhNDhjNDhhMDRiYjk1NmJlYWJlMGEiLCJwIjoiaiJ9
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist