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

refactor(jest-jasmine2, jest-runtime): use Symbol to pass jest.setTimeout value instead of jasmine specific logic #12124

Merged
merged 5 commits into from
Feb 10, 2022
Merged

refactor(jest-jasmine2, jest-runtime): use Symbol to pass jest.setTimeout value instead of jasmine specific logic #12124

merged 5 commits into from
Feb 10, 2022

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Dec 6, 2021

Summary

Currently jest-runtime still implements jasmine specific logic for jest.setTimeout function. It seems like simple refactor is enough to be able to use Symbol.for('TEST_TIMEOUT_SYMBOL') for both test runners (currently it is used only by jest-circus).

Benefits of this change: jest-runtime would have no jasmine specific code; hence, this would make possible to get rid of jasmine specific types as well. I have a draft for types too, but left it for separate PR.

There is no need of a change log entry in this case, right?

By the way, an alternative solution can be seen in the first commit. In this case jasmines private _DEFAULT_TIMEOUT_INTERVAL is replaced by global[testTimeoutSymbol] everywhere. All worked, but seemed more invasive and perhaps breaking too.

Test plan

The existing test suite should be sufficient.

Comment on lines +56 to +58
set(value) {
(global as any)[testTimeoutSymbol] = value;
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setter is used by jasmine.DEFAULT_TIMEOUT_INTERVAL. Otherwise it is unnecessary here.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #12124 (6c1693e) into main (2f5a93d) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12124      +/-   ##
==========================================
- Coverage   67.26%   67.24%   -0.02%     
==========================================
  Files         330      330              
  Lines       17352    17355       +3     
  Branches     5071     5069       -2     
==========================================
- Hits        11672    11671       -1     
- Misses       5648     5652       +4     
  Partials       32       32              
Impacted Files Coverage Δ
packages/jest-jasmine2/src/jasmine/jasmineLight.ts 0.00% <0.00%> (ø)
packages/jest-runtime/src/index.ts 55.51% <0.00%> (+0.19%) ⬆️
packages/expect/src/utils.ts 96.09% <0.00%> (-0.49%) ⬇️

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 2f5a93d...6c1693e. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Dec 10, 2021

This is a good change, but I'm a bit worried about landing it outside of a major release. What happens if somebody gets a newer version of jest-jasmine while still having old runtime? I assume setTimeout then just breaks.

Happy to land in Jest 28, tho 👍 (or 27 if I'm mistaken 😀)

@SimenB SimenB added this to the Jest 28 milestone Dec 10, 2021
@mrazauskas
Copy link
Contributor Author

@SimenB Nice to see you are landing breaking changes. Should feel good (;

When the time comes for this PR, please take a look at the first commit. It is more breaking, but the code looks cleaner.

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.

I like it! 😀

changelog entry?

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.

👍

@SimenB SimenB merged commit ff7a751 into jestjs:main Feb 10, 2022
@mrazauskas mrazauskas deleted the refactor-use-testTimeoutSymbol branch February 10, 2022 12:30
@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 Mar 13, 2022
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