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

[jest-jasmine2] Rewrite QueueRunner. #3187

Merged
merged 8 commits into from
Mar 23, 2017
Merged

[jest-jasmine2] Rewrite QueueRunner. #3187

merged 8 commits into from
Mar 23, 2017

Conversation

wtgtybhertgeghgtwtg
Copy link
Contributor

Summary
Rewrites QueueRunner (now queueRunner) and pulls it out of jasmine.
One of the problems with the current implementation of QueueRunner is that it creates
very deep recursive stacks very quickly. It tries to mitigate that by spinning off into a timeout every 20 iterations or so, but that's an ugly and suboptimal solution. With this rewrite, queueRunner maps queueableFns to promises and resolves them sequentially, so the stack never gets very deep. Performance is not noticeably affected.

Test plan
Tests have been written for queueRunner. A test in jest-util has been changed, as it required the stack to be a certain way.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

wtgtybhertgeghgtwtg commented Mar 22, 2017

Tests are failing because

  • The snapshot for failures-test needs to be updated for the new stack trace.
  • The test for warnings in fake timers needs to be updated for the new stack trace.
  • Examples need to be tested.

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
arvidfm Arvid Fahlström Myrman
Manually.

Verified

This commit was signed with the committer’s verified signature.
arvidfm Arvid Fahlström Myrman
…tch-1

Update snapshot.

Verified

This commit was signed with the committer’s verified signature.
arvidfm Arvid Fahlström Myrman
@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Tests are failing due to stack trace differences in Circle, which tests on node@6, and Travis, which tests on node@7.
Is there a recommended solution for this?

@thymikee
Copy link
Collaborator

Oh, in this case you should discard usage of snapshot for stack trace and use regular matchers instead.

@aaronabramov aaronabramov merged commit 9ff6481 into jestjs:master Mar 23, 2017
@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg deleted the replace-queue-runner branch March 23, 2017 22:39
@quentin- quentin- mentioned this pull request Mar 25, 2017
skovhus pushed a commit to skovhus/jest that referenced this pull request Apr 29, 2017
* [jest-jasmine2] Rewrite `QueueRunner`.

* Make `FakeTimer` test stack agnostic.

* Update snapshot.

Manually.

* Fix `FakeTimer`.

* Lint.

* Do not use async.

* Revert "Merge pull request jestjs#1 from wtgtybhertgeghgtwtg/wtgtybhertgeghgtwtg-patch-1"

This reverts commit 9424111, reversing
changes made to ea0bb8a.
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* [jest-jasmine2] Rewrite `QueueRunner`.

* Make `FakeTimer` test stack agnostic.

* Update snapshot.

Manually.

* Fix `FakeTimer`.

* Lint.

* Do not use async.

* Revert "Merge pull request jestjs#1 from wtgtybhertgeghgtwtg/wtgtybhertgeghgtwtg-patch-1"

This reverts commit 9424111, reversing
changes made to ea0bb8a.
@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 13, 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.

None yet

4 participants