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

chore(tests): Fixing weird test case in SwapScreen #4953

Closed
wants to merge 1 commit into from

Conversation

dievazqu
Copy link
Contributor

@dievazqu dievazqu commented Feb 21, 2024

Description

The test in SwapScreen "should show and hide the switched network warning" doesn't fail when it runs with the full batch of test cases.
But if it runs alone or first, it was failing.

This PR fixes the test case, however it doesn't address the fact that there's some mismanagement of the state between tests.

Test plan

Unit tests

Backwards compatibility

Y

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (abc3fe9) 85.35% compared to head (77d8745) 85.35%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4953   +/-   ##
=======================================
  Coverage   85.35%   85.35%           
=======================================
  Files         713      713           
  Lines       29168    29168           
  Branches     5074     5074           
=======================================
  Hits        24895    24895           
  Misses       4034     4034           
  Partials      239      239           
Files Coverage Δ
test/values.ts 100.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abc3fe9...77d8745. Read the comment docs.

Copy link
Collaborator

@kathaypacific kathaypacific left a comment

Choose a reason for hiding this comment

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

i think the issue comes from this mock which is not cleared and persists for all the tests that come after.

the wallet requires both both the priceUsd and a recent fetched timestamp to show the fiat price, otherwise it will show no usd price warning and the confirmation overlay. the problem is we haven't replicated it well in our test conditions - here, we should have tested also for the fetched at timestamp, or enable some way to assert that the price warning is expected.

this test shouldn't work because the price for USDC doesn't have a fetched at timestamp but has a priceUsd. your fix works because you're now using Date.now() to set the last fetched price time for USDC. i think a more holistic way to fix this issue is to:

  1. restore the date.now mock in the beforeEach step
  2. update selectSingleSwapToken to account for stale token prices

i suppose your solution does a variant of 2. but it doesn't prevent head scratching in the future when other people bump up against this...

@kathaypacific kathaypacific mentioned this pull request Mar 6, 2024
1 task
@kathaypacific
Copy link
Collaborator

@dievazqu is it still on your radar to make the changes in my comment above? lmk if you want me to do it instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants