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

Remove enableTimeout api + allow overriding a disabled timeout #4260

Merged
merged 14 commits into from May 6, 2020

Conversation

craigtaub
Copy link
Contributor

@craigtaub craigtaub commented Apr 30, 2020

Description of the Change

Removed enableTimeout stuff. Use the latest timeout 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 explicit timeout. However if it is the correct behaviour and goes into the next major release, then should be fine.

Applicable issues

Fixes #4254

@craigtaub craigtaub added type: bug a defect, confirmed by a maintainer semver-major implementation requires increase of "major" version number; "breaking changes" labels Apr 30, 2020
@coveralls
Copy link

coveralls commented Apr 30, 2020

Coverage Status

Coverage decreased (-0.06%) to 93.045% when pulling d01b4b7 on craigtaub/issue/4254-timeouts into 8b6a76c on master.

lib/runnable.js Outdated Show resolved Hide resolved
@craigtaub craigtaub changed the title Allow overriding a disabled timeout for a test Remove enableTimeout api + allow overriding a disabled timeout May 1, 2020
lib/runnable.js Show resolved Hide resolved
lib/runnable.js Show resolved Hide resolved
lib/runnable.js Show resolved Hide resolved
lib/suite.js Show resolved Hide resolved
test/integration/timeout.spec.js Outdated Show resolved Hide resolved
@craigtaub
Copy link
Contributor Author

craigtaub commented May 3, 2020

Dont get how Coverage is down. Rebased.

@craigtaub craigtaub force-pushed the craigtaub/issue/4254-timeouts branch from 9a09ef4 to 85d1818 Compare May 3, 2020 16:42
Copy link
Member

@boneskull boneskull left a 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().

@craigtaub
Copy link
Contributor Author

craigtaub commented May 5, 2020

@boneskull thanks for approval, happy to add docs, but seems we have couple of options for this:

  1. Add docs and then merge - potentially removing code we will need later (fixing silent exit issue)
  2. Leave it and go fix silent exit issue in alternative PR, then come back in future date - not a fan of leaving this PR open indefinitely and that fix will not be quick
  3. Revert to previous bug fix (commit), leaving silent timeout issue for another time - I am more in favour of this tbh. If turns out we can remove enableTimeout should be relatively easy to re-apply. Involves leaving most of the enableTimeout behaviour alone for now aswell, as we hopefully can remove it in future.

@boneskull
Copy link
Member

@craigtaub

Add docs and then merge - potentially removing code we will need later (fixing silent exit issue)

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)?

Revert to previous bug fix (commit), leaving silent timeout issue for another time - I am more in favour of this tbh. If turns out we can remove enableTimeout should be relatively easy to re-apply. Involves leaving most of the enableTimeout behaviour alone for now aswell, as we hopefully can remove it in future.

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.

@craigtaub
Copy link
Contributor Author

@boneskull

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)?

Ah sorry badly written, i meant keep PR mostly as is. Removing code which might be useful for fixing the silent exit issue.

Does this mean closing this PR, or just taking the documentation?

Meant reverting all changes relating to removing enableTimeout, so back to this.

If the situation is no "worse" than it already is

The worry is we remove enableTimeout and it turns out to be necessary to fix the silent exit issue, I just dont know what that will entail.

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?

@boneskull
Copy link
Member

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

@boneskull
Copy link
Member

do whatever you think is best.

@craigtaub craigtaub force-pushed the craigtaub/issue/4254-timeouts branch from 03fa835 to d01b4b7 Compare May 6, 2020 10:49
@craigtaub craigtaub added this to the v8.0.0 milestone May 6, 2020
@craigtaub craigtaub merged commit c0137eb into master May 6, 2020
@craigtaub craigtaub deleted the craigtaub/issue/4254-timeouts branch May 6, 2020 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests and suites do not timeout when a zero timeout is changed to nonzero
4 participants