-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Remove enableTimeout api + allow overriding a disabled timeout #4260
Conversation
Dont get how Coverage is down. Rebased. |
9a09ef4
to
85d1818
Compare
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.
LGTM, I think. might want to document the expectations around async tests and calling done()
.
@boneskull thanks for approval, happy to add docs, but seems we have couple of options for this:
|
A little confused on the wording here. You mean that you want to address the silent exit issue in this PR (by exiting after 1e9ms)?
Does this mean closing this PR, or just taking the documentation? If the situation is no "worse" than it already is--meaning we still have a potential silent exit problem--I don't see any reason not to merge this PR, unless I'm missing something. |
Ah sorry badly written, i meant keep PR mostly as is. Removing code which might be useful for fixing the silent exit issue.
Meant reverting all changes relating to removing
The worry is we remove Im actually now thinking if the fix for silent exits is a while away, it is fine to merge this PR as it improves the area for right now, and if we need a new state mechanism it can be added as part of the silent exit fix. Thoughts? |
I think we could fix it with an interval which ran infinitely until the test completes or we exit via other means. essentially put a dummy timer into the event loop. this wouldn’t need another state variable. would have to play with it though |
do whatever you think is best. |
03fa835
to
d01b4b7
Compare
Description of the Change
Removed
enableTimeout
stuff. Use the latesttimeout
value to determine if enabled/disabled + its value. (disabled is value of 0, else is enabled).Also fixes issue of tests not timing out when timeout is re-enabled. See associating issue #4254.
Fixes #4254
Alternate Designs
Could remove
enableTimeouts
from public-facing and use it internally, however it is easiest to have a single source of truth (the value) and mirror what the user uses. Having 2 different states which are very similar gets confusing.Why should this be in core?
Simplify the codebase.
There are use-cases where it is useful to be able to re-enable and make use of a previously disabled timeout. Also seems pretty important that the feature behaves as a user would expect.
Possible Drawbacks
You can no longer restore a previously set timeout if disabled (
enableTimeout(true)
did) and must always explicitly set it again.The default timeout value will now never be used as the user has to set a value to re-enable it anyway. That could be a good thing.
Anyone relying on
enableTimeout
to disable or re-enable timeouts will have to swap to using the explicittimeout
. However if it is the correct behaviour and goes into the next major release, then should be fine.Applicable issues
Fixes #4254