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

Add error context if timers are infinite looped #4626

Closed

Conversation

DeFuex
Copy link

@DeFuex DeFuex commented Oct 7, 2017

Summary
related to Issue #2893. If runAllTimers() is called and timers are caught in an infinite loop the error message should contain more information like pending timers, each showing a depending stacktrace.

@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 cla@fb.com. Thanks!

@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!

import type {ProjectConfig} from 'types/Config';
import type {Global} from 'types/Global';
import type {ModuleMocker} from 'jest-mock';
import type {ProjectConfig } from 'types/Config';
Copy link
Member

Choose a reason for hiding this comment

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

you should run yarn lint --fix to remove all of these whitespace changes

Copy link
Member

Choose a reason for hiding this comment

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

I also doubt you want the yarn.lock changes

Copy link
Author

Choose a reason for hiding this comment

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

hi, @SimenB , i get a lot of Unable to resolve path to module issues when yarn lint --fix ...and i also had to add "eslint-config-fbjs": "^2.0.0" to make yarn lint work. do i do something wrong here?

Copy link
Member

Choose a reason for hiding this comment

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

remove your changes to yarn.lock, and run yarn clean-all && yarn && yarn lint --fix. Make sure you are on yarn@1.1.0 as well.

That should fix it 🙂

Copy link
Author

Choose a reason for hiding this comment

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

hehe, i'll try, thanks! just switched from atom to vs code (there was some unexpected indent behaviour which i resolved now)

Copy link
Author

Choose a reason for hiding this comment

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

$ eslint . --cache --ext js,md
Cannot read config file: /Users/timoobereder/Documents/Projects/react/jest/packages/eslint-config-fb-strict/index.js
Error: Cannot find module 'eslint-config-fbjs'
Referenced from: /Users/timoobereder/Documents/Projects/react/jest/.eslintrc.js
Error: Cannot read config file: /Users/timoobereder/Documents/Projects/react/jest/packages/eslint-config-fb-strict/index.js
Error: Cannot find module 'eslint-config-fbjs'
Referenced from: /Users/timoobereder/Documents/Projects/react/jest/.eslintrc.js
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/timoobereder/Documents/Projects/react/jest/packages/eslint-config-fb-strict/index.js:10:20)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
/Users/timoobereder/.yarn/bin/yarn: line 31: 92774 Segmentation fault: 11  node "$basedir/yarn.js" "$@"```

Copy link
Member

Choose a reason for hiding this comment

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

Segfault, interesting...

Copy link
Author

Choose a reason for hiding this comment

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

i'm sure i have the latest yarn version through homebrew

Copy link
Member

Choose a reason for hiding this comment

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

@DeFuex I pushed to your branch. Still a couple of lint errors, but those are actual issues, not style related.

So just pull, and do yarn and you should be good

@SimenB
Copy link
Member

SimenB commented Oct 7, 2017

I realize it's WIP, but some tests would be great 😃

"Assuming we've hit an infinite recursion and bailing out...",
"Assuming we've hit an infinite recursion and bailing out." +
'The following timers are still pending:' +
Object.keys(timers)
Copy link
Member

@SimenB SimenB Oct 8, 2017

Choose a reason for hiding this comment

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

timers is not defined, and the syntax below is weird.

Copy link
Author

Choose a reason for hiding this comment

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

i know, im not sure if im doing this the right way, but i want to get the timers and then want to get only pending timers and output the stacktrace information for each specifically with map const timers = Object.assign({}, this._timers);

Object.keys(timers)
.sort((left, right) => timers[left].expiry - timers[right].expiry)
.map(function(key, index) {
'\n\nsetTimeout - pending timer time ' +
Copy link
Member

Choose a reason for hiding this comment

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

missing return here?

Copy link
Author

Choose a reason for hiding this comment

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

i don't think a return is missing since Object,keys(object).map should work like an iterate.

Copy link
Member

Choose a reason for hiding this comment

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

if you just want to just iterate, you should use .forEach.

But if you don't return here, the string is never shown to the user as it's lost inside the map. So you want map and the return.

Something like this (which still fails, but it's a start):

diff --git i/packages/jest-util/src/fake_timers.js w/packages/jest-util/src/fake_timers.js
index a1870352..1e0b09e7 100644
--- i/packages/jest-util/src/fake_timers.js
+++ w/packages/jest-util/src/fake_timers.js
@@ -234,16 +234,16 @@ export default class FakeTimers {
           Object.keys(timers)
             .sort((left, right) => timers[left].expiry - timers[right].expiry)
             .map(function(key, index) {
-              '\n\nsetTimeout - pending timer time ' +
+              return '\n\nsetTimeout - pending timer time ' +
                 timers[key].expiry -
-                this._now;
+                this._now +
               '\n' +
                 formatStackTrace(new Error().stack, this._global.setTimeout, {
                   noStackTrace: true,
                 }) +
                 '\n\nsetImmediate - pending timer time ' +
                 timers[key].expiry -
-                this._now;
+                this._now +
               '\n' +
                 formatStackTrace(new Error().stack, this._global.setImmediate, {
                   noStackTrace: true,

Copy link
Author

Choose a reason for hiding this comment

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

ah yeah, does make sense. i thought using object.keys would be nicer to iterate through an object since i also can use sort to get the pending timers that are left.

@DeFuex
Copy link
Author

DeFuex commented Oct 8, 2017

still getting a segmentation fault 😢

@SimenB
Copy link
Member

SimenB commented Oct 8, 2017

What version of node are you running?

Also, have you checked out jest inside of react, or is react just the name of your directory?

This is the path in your stack strace: /Users/timoobereder/Documents/Projects/react/jest/. If jest is inside of react, that can explain some of the issues, make sure to check out jest in its own directory

@DeFuex
Copy link
Author

DeFuex commented Oct 8, 2017

i'm running node version v7.4.0 and react is just the name of my directory

@SimenB
Copy link
Member

SimenB commented Oct 8, 2017

Node 7 is not supported, you should upgrade to node 8.

That said, it shouldn't be an issue... Not sure I can help with this 😞

@DeFuex
Copy link
Author

DeFuex commented Oct 8, 2017

linked the right node version (v8.6.0) and latest npm. getting following error now after yarn clean-all && yarn && yarn lint --fix -> Cannot read config file: /Users/timoobereder/Documents/Projects/react/jest/packages/eslint-config-fb-strict/index.js Error: Cannot find module 'eslint-config-fbjs' Referenced from: /Users/timoobereder/Documents/Projects/react/jest/.eslintrc.js Error: Cannot read config file: /Users/timoobereder/Documents/Projects/react/jest/packages/eslint-config-fb-strict/index.js Error: Cannot find module 'eslint-config-fbjs' Referenced from: /Users/timoobereder/Documents/Projects/react/jest/.eslintrc.js at Function.Module._resolveFilename (module.js:527:15) at Function.Module._load (module.js:476:23) at Module.require (module.js:568:17) at require (internal/module.js:11:18) at Object.<anonymous> (/Users/timoobereder/Documents/Projects/react/jest/packages/eslint-config-fb-strict/index.js:10:20) at Module._compile (module.js:624:30) at Object.Module._extensions..js (module.js:635:10) at Module.load (module.js:545:32) at tryModuleLoad (module.js:508:12) at Function.Module._load (module.js:500:3) error Command failed with exit code 1.

@SimenB
Copy link
Member

SimenB commented Oct 8, 2017

To confirm, you are on yarn 1.1.0 as well?

@DeFuex
Copy link
Author

DeFuex commented Oct 8, 2017

i feel a little stupid, as it seems i got node, npm and yarn messed up with a normal global install and the homebrew variant. at least now i know why it wasn't working. got it now and gonna go on working on the pr, thanks @SimenB 😄

@DeFuex
Copy link
Author

DeFuex commented Oct 8, 2017

Current state: needs adjustment in 1 test case and proper testing

"Assuming we've hit an infinite recursion and bailing out...",
"Assuming we've hit an infinite recursion and bailing out." +
'The following timers are still pending:' +
String(
Copy link
Member

Choose a reason for hiding this comment

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

this is unnecessary, you should do a reduce on the array returned by map instead

Copy link
Member

Choose a reason for hiding this comment

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

Or just .join

Copy link
Author

Choose a reason for hiding this comment

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

thx, i will do that today

@@ -225,11 +225,37 @@ export default class FakeTimers {
}

if (i === this._maxLoops) {
const timers = Object.assign({}, this._timers);
Copy link
Member

Choose a reason for hiding this comment

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

you don't need to do this if you only do Object.keys on it (just don't mutate the object)

@DeFuex DeFuex changed the title [WIP] Add error context if timers are infinite looped Add error context if timers are infinite looped Oct 12, 2017
@cpojer
Copy link
Member

cpojer commented Jan 6, 2018

It seems like this PR alters snapshots on Travis?

@SimenB
Copy link
Member

SimenB commented Jan 6, 2018

This will be void if and when we merge #5171 as well (at which time this context info would be great to have in Lolex, I'd think. /cc @benjamingr).

@cpojer
Copy link
Member

cpojer commented Jan 6, 2018

Got it, will close this PR in favor of the lolex replacement then.

@cpojer cpojer closed this Jan 6, 2018
@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 12, 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.

4 participants