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

Fix util.promisify(setTimeout) on jest-fake-timers #9180

Merged
merged 13 commits into from Nov 29, 2019
Merged

Fix util.promisify(setTimeout) on jest-fake-timers #9180

merged 13 commits into from Nov 29, 2019

Conversation

lourenci
Copy link
Contributor

It closes #9150.

Some points to consider and help me to reach the best implementation.

  • I've implemented setTimeout[util.promisify.custom] on jest-fake-timers like the original one implemented on node. It doesn't call the callback passed through it. Therefore this implementation differs from Lolex implementation.
  • Since the implementation doesn't call the callback, I'm not sure about the best test strategy here. I can't assert for some mock function to be called.
  • I don't know where is the right place to test this. My first attempt was to create a spec for each runTimers.... I still think this is the best strategy because it looks like it is more "black-boxed", but I did the implementation with the least effort to get some advice.

@facebook-github-bot
Copy link
Contributor

Hi lourenci! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@codecov-io
Copy link

codecov-io commented Nov 14, 2019

Codecov Report

Merging #9180 into master will decrease coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9180      +/-   ##
==========================================
- Coverage   65.06%   64.83%   -0.24%     
==========================================
  Files         278      279       +1     
  Lines       11864    11743     -121     
  Branches     2924     2884      -40     
==========================================
- Hits         7719     7613     -106     
+ Misses       3518     3511       -7     
+ Partials      627      619       -8
Impacted Files Coverage Δ
packages/jest-fake-timers/src/jestFakeTimers.ts 90.35% <100%> (+0.14%) ⬆️
packages/expect/src/jasmineUtils.ts 91.07% <0%> (-1.04%) ⬇️
packages/jest-reporters/src/coverage_reporter.ts 61.53% <0%> (-0.3%) ⬇️
packages/jest-haste-map/src/lib/FSEventsWatcher.ts 12.5% <0%> (-0.23%) ⬇️
packages/jest-runtime/src/index.ts 68.86% <0%> (-0.11%) ⬇️
packages/jest-haste-map/src/index.ts 81.21% <0%> (-0.06%) ⬇️
packages/jest-snapshot/src/utils.ts 95.29% <0%> (ø) ⬆️
...ckages/jest-reporters/src/generateEmptyCoverage.ts 88.88% <0%> (ø) ⬆️
packages/jest-config/src/index.ts 11.94% <0%> (ø) ⬆️
packages/jest-snapshot/src/index.ts 73.23% <0%> (ø) ⬆️
... and 10 more

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 429877d...b27f42b. Read the comment docs.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! I left comments where the code/test does not seem to be correct

packages/jest-fake-timers/src/jestFakeTimers.ts Outdated Show resolved Hide resolved
packages/jest-fake-timers/src/jestFakeTimers.ts Outdated Show resolved Hide resolved
@jeysal
Copy link
Contributor

jeysal commented Nov 26, 2019

Can you add a changelog entry please? :)

@jeysal jeysal requested a review from SimenB November 26, 2019 12:55
@lourenci lourenci requested a review from jeysal November 26, 2019 16:00
@jeysal
Copy link
Contributor

jeysal commented Nov 29, 2019

Made an edit that triggers Azure CI to run again, I think the macOS cancellation was just flakiness, let's see

@jeysal
Copy link
Contributor

jeysal commented Nov 29, 2019

Alright, Azure passed. Node 13 failure happens everywhere now, unrelated. Thanks a lot for the contribution! :)

@jeysal jeysal merged commit 5759a85 into jestjs:master Nov 29, 2019
@lourenci lourenci deleted the fix-jest-faker-timers branch November 29, 2019 22:16
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jest-fake-timers breaks util.promisify(setTimeout)
4 participants