Add stack trace to code recursively scheduling timers

[alistairjcbrown]

Purpose (TL;DR) - mandatory

Fix issue #230 by using stack information for jobs and timers to provide more context when an infinite loop error is thrown.

Solution - optional

This solution works by adding a stack property to jobs and tasks. This data can then be used, if an infinite loop is detected, to set the stack of the thrown infiniteLoopError.

Example stack trace from test run

Error: Aborting after running 5 timers, assuming an infinite loop!
Timeout - recursiveCreateTimerTimeout
      at recursiveCreateTimer (test/fake-timers-test.js:4668)
      at recursiveCreateTimerTimeout (test/fake-timers-test.js:4669)
      at callTimer (src/fake-timers-src.js:441)
      at Object.next (src/fake-timers-src.js:1039)
      at src/fake-timers-src.js:1128

[codecov[bot]]

Codecov Report

Merging #325 into master will increase coverage by 0.39%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
+ Coverage   93.12%   93.52%   +0.39%     
==========================================
  Files           1        1              
  Lines         553      587      +34     
==========================================
+ Hits          515      549      +34     
  Misses         38       38              

Flag	Coverage Δ
#unit	93.52% <100.00%> (+0.39%)	:arrow_up:

Impacted Files	Coverage Δ
src/fake-timers-src.js	93.52% <100.00%> (+0.39%)	:arrow_up:

---

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 cb4c2f6...d95493e. Read the comment docs.

[SimenB]

Could we just keep track of the last scheduled timer instead of all scheduled timers? Not sure how much that matters

[SimenB]

Another thought, this is mostly meant to catch timers added while running timers. Could we add some flag internally that's set to true whenever we run timers, and only collect these at that time?

[alistairjcbrown]

@SimenB Thanks for the feedback! I've updated to set an error object on the job instead of getting the error stack. The second commit adds in a isNearInfiniteLimit flag which is used to decide whether to attach an error object to the job. Jobs from the job before we hit the limit will have an error property populated, which is then used if we need to create an InfiniteLoopError. What do you think?

[fatso83]

no time (parental leave) at the moment to look at PRs, sorry.

[benjamingr]

Also:

What do you think?

The actual implementation logic lgtm

[alistairjcbrown]

@benjamingr Awesome, thanks for the feedback! I'll take a run through it tomorrow night 👍

[alistairjcbrown]

@benjamingr

Dealing with a non-writeable stack property

Using defineProperty wrapped in try/catch as suggested - TIL about non-writeable stacks 👍

calculating the slice size automatically

Doing this by running through the stack line by line, checking the line matches at target.X (where X may be (anonymous function) or <computed>) or at Object.Y (where Y is a clock method).

Tests that check a larger chunk of the stack (so we're sure this doesn't break)

Apologies, I wasn't 100% sure on what this was asking, so I've added more tests to cover requestAnimationFrame, setImmediate, setInterval, requestIdleCallback, nextTick.

If this was referring to checking more of the stack trace itself, the tests are checking that we're getting the expected method at the top of the stack - but we can't check the filename as it'll be __puppeteer_evaluation_script__ on headless. Do you want more of the stack trace checked?

[benjamingr]

Hey, I wanted to merge and it looks like there is a merge conflict - can you please take a look?

[alistairjcbrown]

@benjamingr Rebased off master and conflict resolved 👍

[benjamingr]

Thanks!

[fatso83]

This broke the builds on Safari, IE and Firefox. It needs some patching before we can release it as part of a new release. See https://app.circleci.com/pipelines/github/sinonjs/fake-timers/117/workflows/625d68b6-ed2c-4d04-a450-c22a240599dd/jobs/670

The "cloud" tests are only run on master, as PR runs can expose the Sauce Labs credentials.

[fatso83]

@benjamingr Do you think you can get this in again, after fixing the issues that broke the build? Was some good work done here by @alistairjcbrown. Shame to let it go to waste.

[benjamingr]

Yeah I agree - I just didn't have a ton of free time recently :/

Maybe @itayperry wants to pick up readding this? (I can guide, I just have little free time to dedicate to this myself)

[itayperry]

I would be happy to take part in this @benjamingr 🎉 🪴 I could, of course, use some guidance and tips. I hope I'll be able to help 💭