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

Prove wait for works using useFakeTimers #637

Closed
wants to merge 0 commits into from

Conversation

chris110408
Copy link
Contributor

@chris110408 chris110408 commented Jun 18, 2021

What:

Fixes #631

Why:
Create the unit test to prove the wait for function works using jest.useFakeTimers

How:

Created a unit test use jest.useFakeTimers and jest.advanceTimersByTime()
Checklist:

  • Documentation updated
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@chris110408 chris110408 changed the title prove wait for works when using useFakeTimers prove wait for works using useFakeTimers Jun 18, 2021
@chris110408 chris110408 changed the title prove wait for works using useFakeTimers Prove wait for works using useFakeTimers Jun 18, 2021
@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #637 (f9ed124) into main (ee0b83f) will not change coverage.
The diff coverage is n/a.

❗ Current head f9ed124 differs from pull request most recent head 0aaf07a. Consider uploading reports for the commit 0aaf07a to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main      #637   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          235       235           
  Branches        33        29    -4     
=========================================
  Hits           235       235           

Continue to review full report at Codecov.

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

@mpeyper
Copy link
Member

mpeyper commented Jun 18, 2021

thanks again @chris110408!

I'll wait for a resolution to the conversation in #631 before doing anything with this.

)

expect(complete).toBe(true)
jest.useRealTimers()
Copy link
Member

Choose a reason for hiding this comment

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

This should be in a catch block. If the test fails, we don't want to the fake timers to break other tests in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy that makes sense. I will wrap the jest.useRealTimers() in finally

Copy link
Member

Choose a reason for hiding this comment

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

lol, yes, I meant finally, not catch... putting it in catch would not be useful at all 🤦

@mpeyper
Copy link
Member

mpeyper commented Jun 20, 2021

@chris110408 I'm thinking of merging these tests but keeping #631 open until we get some clarification on what the actual issue is there, as I feel these represent valid ways of using fake timers and they will help ensure we don't accidentally break anything for the current functionality.

I know it's annoying (read my thoughts on it here, but would you mind making variations for native and server renderers? It'll mostly be copying the file into their test directory and you may need to add a hydrate call for the server renderer (see other server tests for examples).

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.

waitFor doesn't work if jest fake timers are used
2 participants