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

feat: set global timeout in command line argument #8456

Merged
merged 8 commits into from
Jun 5, 2019

Conversation

ert78gb
Copy link
Contributor

@ert78gb ert78gb commented May 12, 2019

Summary

My motivation is. When I debug the unit tests in IntelliJ and I got a timeout error.
Other's motivation: #6216

I would like to set the timeout to Infinity when jest runner in debug mode if this PR is approved. I like the step by step development

Test plan

I extended the timeout e2e tests with use cases

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@ert78gb ert78gb force-pushed the feat-timeout-in-command-line-arg branch from 335d649 to d36815c Compare May 12, 2019 15:22
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@jeysal
Copy link
Contributor

jeysal commented May 12, 2019

Hey @ert78gb, thanks a lot for tackling one of the most +1ed issues :)
As you'll notice test-jest-circus is failing. We'll need this functionality in jest-circus in addition to jest-jasmine2 - kind of annoying I know but hopefully just as simple as in jasmine2 :)
This should help explain what that's all about.

@jeysal jeysal requested review from SimenB and thymikee May 12, 2019 15:55
@ert78gb
Copy link
Contributor Author

ert78gb commented May 12, 2019

Hi @jeysal I will check soon what is in the "circus" :)

@ert78gb
Copy link
Contributor Author

ert78gb commented May 12, 2019

I solved the jest-circus timeout issue, but I don't like the solution.

Maybe would be better if I create an initState function but currently, I recognized only the timeout parameter to pass and the modified file is in the todo-rewrite folder, so when you will rewrite this part of the code I think you will find a better place to initialize the global timeout.

@codecov-io
Copy link

codecov-io commented May 12, 2019

Codecov Report

Merging #8456 into master will increase coverage by <.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8456      +/-   ##
==========================================
+ Coverage   60.56%   60.56%   +<.01%     
==========================================
  Files         269      269              
  Lines       11045    11051       +6     
  Branches     2690     2694       +4     
==========================================
+ Hits         6689     6693       +4     
- Misses       3770     3772       +2     
  Partials      586      586
Impacted Files Coverage Δ
packages/jest-jasmine2/src/index.ts 0% <ø> (ø) ⬆️
packages/jest-config/src/index.ts 11.94% <ø> (ø) ⬆️
packages/jest-config/src/ValidConfig.ts 100% <ø> (ø) ⬆️
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/jasmineLight.ts 0% <0%> (ø) ⬆️
packages/jest-config/src/normalize.ts 78.2% <100%> (+0.28%) ⬆️

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 44a13e7...bf40692. Read the comment docs.

@@ -789,6 +790,11 @@ export default function normalize(
value = oldOptions[key];
break;
}
case 'timeout': {
value =
oldOptions[key] === 0 ? MAX_32_BIT_SIGNED_INTEGER : oldOptions[key];
Copy link
Member

Choose a reason for hiding this comment

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

can we throw if it's 0? I bet 99.99% of the time, that's not what the user wanted.

Should throw if it's <= 0

Copy link
Member

Choose a reason for hiding this comment

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

Maybe even throw if it's more than, say, an hour. I don't see there being any use case for more than an hour, and again, I'd guess it was some calculation gone wrong for the user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to use 0 to represent the infinity timeout. Similar to other test frameworks. I want an infinity timeout when I debugging. I agree most of the time 10 min is more than enough, but there is a few edge case when need an hour.

jasmine always need a DEFAULT_TIMEOUT_INTERVAL value this is the reason why I set it. I use the max possible 32 bit signed value because I would like to avoid the next issue about pls increase the value x+1.

I can implement the <0 error throwing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB What is the final statement about the setTimeout = 0 and max value? I would like to implement them in 1 comment

Copy link
Member

Choose a reason for hiding this comment

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

What I'm afraid of is somebody messing up their math or something and having ci use a bajillion years timing out.

I'd rather have -1 mean infinity, 0 seems like a math error to me. I might be alone in that though :)

Choose a reason for hiding this comment

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

Why not just require a positive sign number or Infinity.
You can check on infinity and return MAX_32_BIT_SIGNED_INTEGER for jasmine.

Meaning 0 would be instant timeout?

This might be weird for users if other test frameworks default on the 0 = no timeout behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Is there intentional precedence or just what I'd consider buggy code if (timeout) {setSomeTimeout()} since 0 is falsy? Seems more natural to be explicit using the Infinity global. That doesn't really translate to json or CLI config, though...

I don't feel too strongly here if making 0 mean infinity is something mocha, jasmine, tap, ava or someone else already does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • mocha, tap use 0 to represent infinity timeout
  • jasmine need a positive integer. max 2^32-1 the possible value. Infinity is not option for jasmine
  • ava has not got default timeout

I am using mocha near 6 years and I never make mistake when I switch off the timeout.

All modern CI listen to /read from the standard output and if no message - mostly in 10 min - it stop the build process.

If everyone search for the option to switch off the timeout then he/she will read the documentation and if 0 is the no timeout option will use it.

I think the current solution 0 = no timeout is match with the industry standard. Jasmine need an exact value so we need to translate the 0 to a big value. The biggest value 2^32-1.

It's up to you what will the final solution, but pls don't overthink a simple problem :)

TestUtils.ts Outdated Show resolved Hide resolved
docs/CLI.md Outdated Show resolved Hide resolved
@SimenB SimenB requested a review from scotthovestadt May 13, 2019 09:24
@Removed-5an
Copy link

Hello, any chance of getting this PR merged, seems like the requested changes are minor?

@ert78gb
Copy link
Contributor Author

ert78gb commented May 22, 2019

I will do the changes asap if we agree in the final solution. But I have not receive any answer 9 days

@SimenB
Copy link
Member

SimenB commented May 22, 2019

@jeysal @thymikee thoughts?

@jeysal
Copy link
Contributor

jeysal commented May 23, 2019

I have all but strong feelings about the meaning of 0 here, which is why I didn't comment on this earlier. Do we have to treat it in a special way at all? What happens if we don't? In my mental model, the config option works by running the test this way:

setTimeout(() => cancelTestIfStillRunning(), config.timeout);
await runTestToCompletionOrCancellation();

So if the test is synchronous or only uses microticks, it's fine.

And a timeout of 1ms seems no less absurd than 0 to me, so why forbid one but not the other.

@ert78gb ert78gb force-pushed the feat-timeout-in-command-line-arg branch from 4488cb9 to 2ea81b9 Compare June 2, 2019 10:12
@ert78gb
Copy link
Contributor Author

ert78gb commented Jun 2, 2019

I renamed the timeout => testTimeout as you wish.
Please provide an exact requirement for the "no timeout" value (0, -1 or else).
After it, I can implement validation rule(s).

@ert78gb
Copy link
Contributor Author

ert78gb commented Jun 2, 2019

@jeysal You are right in synchronous tests but, this problem is not related to this issue cli timeout parameter. It is a totally new problem about how to measure the timeouts correctly.

@jeysal
Copy link
Contributor

jeysal commented Jun 3, 2019

I'd just not treat any numbers in a special way for reasons described in my latest comment and because it's what jest.setTimeout does.

Please provide an exact requirement for the "no timeout" value (0, -1 or else).

Do we absolutely need one? I'm with @SimenB on this, just use --testTimeout 99999999 and you should be fine.

@ert78gb
Copy link
Contributor Author

ert78gb commented Jun 4, 2019

Ok, then I will remove the special meaning of the 0 and I will add testTimeout < 0 validation

@ert78gb
Copy link
Contributor Author

ert78gb commented Jun 4, 2019

I removed the 0 special meaning.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your patience :)

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.

Just some nitpicks, then I think this is good to go. Thanks!

packages/jest-cli/src/cli/args.ts Outdated Show resolved Hide resolved
docs/CLI.md Outdated Show resolved Hide resolved
packages/jest-jasmine2/src/index.ts Outdated Show resolved Hide resolved
@ert78gb
Copy link
Contributor Author

ert78gb commented Jun 5, 2019

I think I finished.

@SimenB SimenB merged commit 46f9a97 into jestjs:master Jun 5, 2019
@ert78gb ert78gb deleted the feat-timeout-in-command-line-arg branch June 5, 2019 21:08
@roydekleijn
Copy link

when will this be released to NPM?

@SimenB
Copy link
Member

SimenB commented Jun 11, 2019

When we make a release (no timeline). We try to prioritize bug fixes, but features are bound to whenever a release happens. However, we'll probably have one last 24 release soonish (again, no timeline)

@bergos
Copy link

bergos commented Aug 2, 2019

You should fix syncing the docs on the website with the npm releases. When I go to Jest CLI Options it lists the --testTimeout option, also big on the top left version 24.8 is shown but it's not part of that minor version. That can be very confusing.

@jeysal
Copy link
Contributor

jeysal commented Aug 11, 2019

@bergos thanks for reporting this. Looks like there is nothing wrong on the technical side, just the 24.8 version of the CLI docs being created from the wrong commit. I fixed this particular instance in #8811, hope there's not much else wrong.

@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 11, 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.

8 participants