Skip to content

test: fix flaky test Import ERC1155 NFT should be able to import an ERC1155 NFT that user owns#24709

Merged
seaona merged 1 commit intodevelopfrom
flaky-fix-import-erc1155
May 22, 2024
Merged

test: fix flaky test Import ERC1155 NFT should be able to import an ERC1155 NFT that user owns#24709
seaona merged 1 commit intodevelopfrom
flaky-fix-import-erc1155

Conversation

@seaona
Copy link
Copy Markdown
Member

@seaona seaona commented May 22, 2024

Description

This PR fixes the flaky tests related to ERC1155 token imports.
The problem is that whenever we import a token, we are making a call to IPFS, which was currently not mocked. This caused that anytime that the response took long, the test failed.

The fix is simply add a mock for the IPFS request.

However, this shows that we have a bug in the Import process, as we should be able to import the token even if the IPFS call has not ended. Now, it might take several minutes, and we can import the token once we get the timeout response 504.
I opened an issue here

Related issues

Fixes: #24604

Manual testing steps

  1. Run the test multiple times yarn test:e2e:single test/e2e/tests/tokens/nft/import-erc1155.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10
  2. Check ci

Screenshots/Recordings

Screenshot from 2024-05-22 15-42-15

ipfs-pending-req.mp4

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

@github-actions
Copy link
Copy Markdown
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.

@seaona seaona changed the title mock IPFs endpoint for NFT metadata test: fix flaky test Import ERC1155 NFT should be able to import an ERC1155 NFT that user owns May 22, 2024
@seaona seaona self-assigned this May 22, 2024
@seaona seaona added team-extension-platform Extension Platform team flaky tests labels May 22, 2024
@seaona seaona marked this pull request as ready for review May 22, 2024 13:45
@seaona seaona requested a review from a team as a code owner May 22, 2024 13:45
@seaona seaona added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 22, 2024
@DDDDDanica
Copy link
Copy Markdown
Contributor

LGTM !

@codecov
Copy link
Copy Markdown

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.40%. Comparing base (ccb80fc) to head (3ab6d95).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #24709   +/-   ##
========================================
  Coverage    67.40%   67.40%           
========================================
  Files         1293     1293           
  Lines        50358    50358           
  Branches     13043    13043           
========================================
  Hits         33940    33940           
  Misses       16418    16418           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [3ab6d95]
Page Load Metrics (1585 ± 550 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint641611072713
domContentLoaded97118178
load52322915851145550
domInteractive97118178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Copy Markdown
Contributor

@chloeYue chloeYue left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@seaona seaona merged commit 3ddb0d4 into develop May 22, 2024
@seaona seaona deleted the flaky-fix-import-erc1155 branch May 22, 2024 15:22
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2024
@metamaskbot metamaskbot added release-11.18.0 release-11.16.1 Issue or pull request that will be included in release 11.16.1 and removed release-11.18.0 labels May 22, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Missing release label release-11.16.1 on PR. Adding release label release-11.16.1 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.1.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

flaky tests release-11.16.1 Issue or pull request that will be included in release 11.16.1 team-extension-platform Extension Platform team

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Flaky Test: "Import ERC1155 NFT should be able to import an ERC1155 NFT that user owns"

4 participants