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

Revert sync timeouts #7259

Merged
merged 1 commit into from
Oct 24, 2018
Merged

Revert sync timeouts #7259

merged 1 commit into from
Oct 24, 2018

Conversation

rubennorte
Copy link
Contributor

Summary

#7074 made sync tests fail if they finished after the defined timeout. That's a breaking change whose benefits don't compensate the upgrade cost and there's a valid workaround to implement it in userland. See the comments in #6947.

Test plan

Unit and e2e tests.

@rubennorte rubennorte requested review from SimenB, mjesun and rafeca and removed request for mjesun October 24, 2018 11:26
@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

Since GitHub is being silly and not allowing me to comment inline:

image


Will this still report slow async tests as slow in the UI? (red in parens after the test name)

@rubennorte
Copy link
Contributor Author

@SimenB I can do that in a different PR, as a fix. I didn't know that was part of the change and it should be properly isolated and documented in the CHANGELOG.

It's just a revert of the change, so it'll preseve the behaviour about slow tests.

@codecov-io
Copy link

Codecov Report

Merging #7259 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7259      +/-   ##
==========================================
+ Coverage   66.55%   66.56%   +<.01%     
==========================================
  Files         237      237              
  Lines        9317     9307      -10     
  Branches        4        3       -1     
==========================================
- Hits         6201     6195       -6     
+ Misses       3115     3111       -4     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/queue_runner.js 100% <ø> (ø) ⬆️
packages/jest-circus/src/event_handler.js 12% <0%> (ø) ⬆️
packages/jest-jasmine2/src/p_timeout.js 100% <100%> (+9.09%) ⬆️
packages/jest-circus/src/utils.js 21.13% <50%> (-0.13%) ⬇️

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 319329f...b08efbf. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

it should be properly isolated and documented in the CHANGELOG.

Good call, probably worth an e2e test

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

As long as we still mark the test as slow/able to track the time spent correctly, I'm not against this.

@rubennorte rubennorte merged commit 561b767 into jestjs:master Oct 24, 2018
@rubennorte rubennorte deleted the revert-7074 branch October 24, 2018 12:40
@rubennorte
Copy link
Contributor Author

@SimenB do you know in what case would timeouts fail if the user mocked the Date? I'm not able to repro.

@rubennorte rubennorte restored the revert-7074 branch October 24, 2018 12:48
@rubennorte rubennorte deleted the revert-7074 branch October 24, 2018 12:48
@rubennorte rubennorte restored the revert-7074 branch October 24, 2018 12:48
@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

global.Date = {now: () => 0};

test('async', async () => {
  await new Promise(resolve => {
    setTimeout(resolve, 50);
  });
});

test('sync', () => {
  for (let i = 0; i < 10000; i++) {}
});

Master:
image

With binding now:
image

Removing the fake Date:
image

The sync one doesn't work as you removed the eager "test started now" code. It should display at all times

See: https://github.com/facebook/jest/blob/561b767b2db1e4306fefb310e3a7cc5e2e42d874/packages/jest-cli/src/reporters/verbose_reporter.js#L109


So not the timeouts actually, just the timing information we store on the test result

@SimenB
Copy link
Member

SimenB commented Oct 24, 2018

That said, we should inject it similar to how we do Promise, since if you fake Date in a setupFile, it's mocked before Circus is loaded at all, meaning we bind the fake Date.now.

More likely than setupFiles - people have timers: 'fake' in their config after we land Lolex, which will mockout Date for now.

@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 12, 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.

4 participants