-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
refactor(jest-jasmine2, jest-runtime): use Symbol to pass jest.setTimeout
value instead of jasmine
specific logic
#12124
Conversation
set(value) { | ||
(global as any)[testTimeoutSymbol] = value; | ||
}, |
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
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 Happy to land in Jest 28, tho 👍 (or 27 if I'm mistaken 😀) |
@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. |
There was a problem hiding this 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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. |
Summary
Currently
jest-runtime
still implementsjasmine
specific logic forjest.setTimeout
function. It seems like simple refactor is enough to be able to useSymbol.for('TEST_TIMEOUT_SYMBOL')
for both test runners (currently it is used only byjest-circus
).Benefits of this change:
jest-runtime
would have nojasmine
specific code; hence, this would make possible to get rid ofjasmine
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
jasmine
s private_DEFAULT_TIMEOUT_INTERVAL
is replaced byglobal[testTimeoutSymbol]
everywhere. All worked, but seemed more invasive and perhaps breaking too.Test plan
The existing test suite should be sufficient.