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: add user-storage to privacy snapshot and update user-storage mocks #27292

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Sep 19, 2024

note - we need to update our library to support generic paths and mock X-Sync data features

Description

This fixes the flaky test for updating the privacy snapshot for user-storage API.
We also better mock the user-storage API to fail fast when testing Account-Syncing.

NOTE - we need to update our library to support generic paths, and also have mock data for testing the X-Syncing features (account syncing, network syncing, notification syncing, asset syncing).

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.

@Prithpal-Sooriya Prithpal-Sooriya added the team-notifications Notifications team label Sep 19, 2024
Copy link
Contributor

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.

note - we need to update our library to support generic paths and mock X-Sync data features
@Prithpal-Sooriya Prithpal-Sooriya force-pushed the test/fix-failing-user-storage-snapshot branch from ac2fef4 to fcec986 Compare September 19, 2024 18:36
@@ -58,5 +58,6 @@
"oidc.api.cx.metamask.io",
"price.api.cx.metamask.io",
"token.api.cx.metamask.io",
"client-side-detection.api.cx.metamask.io"
"client-side-detection.api.cx.metamask.io",
"user-storage.api.cx.metamask.io"
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 API is now introduced in e2e tests as we have launched a new "Account Syncing" feature, which will sync your accounts you may have previously stored. This is done on wallet start/open post onboarding.

Comment on lines +37 to +38
// TODO - add better mock responses for other Profile Sync features
// (Account Sync, Network Sync, ...)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we are currently mocking these API responses to fail fast. This ensures that the syncing features won't get in the way with existing e2e tests.

@metamaskbot
Copy link
Collaborator

Builds ready [fcec986]
Page Load Metrics (1855 ± 104 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint162624521849218104
domContentLoaded16132298181219795
load164924741855216104
domInteractive138837178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [552f88e]
Page Load Metrics (1977 ± 113 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint61826091927387186
domContentLoaded158623631940229110
load164924901977235113
domInteractive17104482311
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review September 20, 2024 11:25
Copy link
Contributor

@matteoscurati matteoscurati left a comment

Choose a reason for hiding this comment

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

LGTM!

@danjm
Copy link
Contributor

danjm commented Sep 20, 2024

will this API call not occur if the basic functionalities toggle is off?

@danjm
Copy link
Contributor

danjm commented Sep 20, 2024

and when/how did the related test become flaky? how does it pass sometimes and fail other times?

@Prithpal-Sooriya
Copy link
Contributor Author

@danjm

will this API call not occur if the basic functionalities toggle is off?

Yes profile sync/this API is disabled when basic functionality is turned off.

and when/how did the related test become flaky? how does it pass sometimes and fail other times?

We started seeing this appear sometime yesterday.
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/101355/workflows/37ecc4fa-a568-4932-97b1-d90c35dfc55f/jobs/3773505
We have just released the "Account-Syncing" feature, which also is turned off if basic functionality is turned off.
The mock endpoint update was to "wildcard" all calls to this endpoint, and not just notifications.

Happy to sync or async on this.

Copy link

sonarcloud bot commented Sep 20, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [1897fad]
Page Load Metrics (1925 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint49522011861354170
domContentLoaded15502191190816479
load15932202192516278
domInteractive148436178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-notifications Notifications team
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

6 participants