-
Notifications
You must be signed in to change notification settings - Fork 5.4k
test: Use real storage in E2E #38093
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
Conversation
Builds ready [5c78682]
UI Startup Metrics (1256 ± 87 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [ba18647]
UI Startup Metrics (1339 ± 117 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [192b4d9]
UI Startup Metrics (1269 ± 108 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [e8cc962]
UI Startup Metrics (1254 ± 108 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| @@ -1,16 +1,16 @@ | |||
| { | |||
| "data": { | |||
| "AuthenticationController": { "isSignedIn": "boolean" }, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation for the big diff here is that previously we would rely on ReadOnlyNetworkStore here:
| ? new ReadOnlyNetworkStore() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @Gudahtt for a sanity check
Builds ready [0c65494]
UI Startup Metrics (1309 ± 103 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This reverts commit 6c29eeb.
0c65494 to
06c2ccb
Compare
06c2ccb to
cccca99
Compare
Builds ready [cccca99]
UI Startup Metrics (1287 ± 115 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [8b82805]
UI Startup Metrics (1218 ± 104 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| async reset(): Promise<void> { | ||
| this.#initialized = false; | ||
| this.#state = null; | ||
| await super.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Race condition in reset method
The reset method has a race condition where #initialized is set to false before creating a new #initializing promise. If get or set is called between setting #initialized = false and this.#initializing = this.#init(), they will await the old resolved promise instead of the new initialization, potentially accessing uninitialized or reset state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this matters in practice, but can fix it if anyone feels strongly about it
cryptodev-2s
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
This PR replaces
ReadOnlyNetworkStorewithFixtureExtensionStore. This new class extendsExtensionStorewhile still seeding the initial state using the fixture server. This effectively means that all E2Es leverage the real browser storage APIs instead of an in-memory store, bringing us closer to testing actual production builds.Due to this change in behavior, the
errors-before-init-opt-in-ui-state.jsonsnapshot had to be updated as well. The snapshot is now more realistic as it contains the initialized background state, instead of just the fixture data.Changelog
CHANGELOG entry: null
Related issues
Fixes:
Note
Replace ReadOnlyNetworkStore with FixtureExtensionStore (seeds from fixture server but uses real extension storage) and update tests and state snapshots accordingly.
FixtureExtensionStoreextendingExtensionStore, initializing fromhttp://localhost:12345/state.jsonand persisting via real browser storage; supports asyncget/set/reset.FixtureExtensionStorein test builds inapp/scripts/background.jsandlib/setup-initial-state-hooks.jsviaPersistenceManager.fixture-extension-store.test.ts(mockswebextension-polyfill, verifies network seeding and storage behavior).persistence-manager.test.tsto rely onExtensionStoremock (remove read-only store mock).errors.spec.ts: remove debug logs, add waits for state settle, and tweak Sentry assertions.errors-before-init-opt-in-ui-state.jsonto reflect initialized background state and new schema/version.PersistenceManagerwithFixtureExtensionStorein tests; add cleanup of most recent state during app-state acquisition.Written by Cursor Bugbot for commit 8b82805. This will update automatically on new commits. Configure here.