Retry - Add stack trace to code recursively scheduling timers #325

[itayperry]

Re-adding the reverted merge - Add stack trace to code recursively scheduling timers #325

I've cherry-picked commits made by @alistairjcbrown to re-file the original PR that was merged and then reverted due to some compatibility issues.

[codecov[bot]]

Codecov Report

Merging #375 (ee3e901) into master (843e307) will increase coverage by 0.37%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   93.46%   93.84%   +0.37%     
==========================================
  Files           1        1              
  Lines         551      585      +34     
==========================================
+ Hits          515      549      +34     
  Misses         36       36              

Flag	Coverage Δ
unit	93.84% <100.00%> (+0.37%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/fake-timers-src.js	93.84% <100.00%> (+0.37%)	: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 843e307...ee3e901. Read the comment docs.

[fatso83]

So this fails in IE 11, Safari 9, Firefox. This ties in with https://github.com/sinonjs/sinon/issues/2359 which is about which browsers we officially support and that our docs is out-of-sync. We have dropped IE 11 and Safari 9 in favor of evergreen browsers, so we could ignore them here as well, but it fails in Firefox, which we do support, and we need to handle that somehow.

@itayperry You could get our shared sauce labs credentials if you want to run the test suite locally, but it's probably just as fast to just fire up webdriver and modify the .min-wd config to use a local Selenium webdriver instance.

P.S. I will add a PR to remove the unsupported browsers:

diff --git a/.min-wd b/.min-wd
index ddeba38..22abced 100644
--- a/.min-wd
+++ b/.min-wd
@@ -1,13 +1,5 @@
 {
   "browsers": [{
     "name": "firefox"
-  }, {
-    "name": "internet explorer",
-    "version": "11"
-  }, {
-    "name": "safari",
-    "platform": "OS X 10.11",
-    "version": "9.0"
   }]
 }

[fatso83]

IE11 and Safari has been dropped. Any chance of seeing if you can get this running in Firefox as well?

[itayperry]

Yeah sure @fatso83, I don't know how to do it, but I would like to try! :) I'm on it.

[fatso83]

No worries - you don't have to fly alone, you know. Call out if you need a hand :)

I have not done it personally, but min-wd is a minimal WebDriver, so you can probably fire up Selenium and run the tests in Firefox like that. But ... on the other hand, just copy-pasting the entire script into the Console in Firefox dev tools works too ;)

[itayperry]

Thank you @fatso83 for helping out and explaining everything!

just copy-pasting the entire script

Do you mean this script: "test-cloud": "mochify --wd --no-detect-globals --timeout=10000" that appears in package.json?

I'm just guessing because this script shows Unexpected HTTP status: 401 Unauthorized when running it, and because it's the only one that I could see using the .min-wd config file.

It sounds great that I can edit the .min-wd to use my own local environment - I'm checking it out.

[fatso83]

I was simply thinking that if you needed to test certain things in Firefox that failed in the Sauce Labs run you could just copy-paste the entire fake-timers.js script and just manually execute whatever failed in the test runs. This was just in case you had a hard time firing up Selenium locally and debug it, of course.

The test-cloud script returns 401 since you do not have credentials setup. ~I contacted you on Twitter to send you the credentials. You need $SAUCE_ACCESS_KEY and $SAUCE_USERNAME in your environment. If you pass me an email (on my profile) you can get it right away.~ EDIT: I just saw you responded. Check your inbox :)

[itayperry]

I didn't understand the script way at first, so I decided to try the other way with the local wd. I'm just a little slow on the take right now :) If I wanna use the script in the console should I build it first?

And thank you for the email!! 🌼 and for all the help.

[fatso83]

If I wanna use the script in the console should I build it first?

As the docs say, you can either build it or use Skypack. Since the latter assumes it's already on NPM, you need to build it for the browser:

npx browserify src/fake-timers-src.js -o lolex.js

If you want to run all tests in the browser (without going through mochify):

npx browserify test/fake-timers-test.js -o tests.js

[itayperry]

Hi @fatso83, thanks for explaining - I tried both:

npx browserify src/fake-timers-src.js -o lolex.js worked when I pasted it in the Firefox console, the second one threw an error (about describe being undefined).

I'm gonna try to manually execute the code, do you know how do I access createClock() and basically all the other methods in the console? I've looked it up on the 'window' object (thought it might be there) after running the script, but I couldn't find it. I don't know the naming in which it was saved.

Another different thing I tried to do is to add --debug to the code that runs the script.. and I got the message: --debug does not work with --wd

[itayperry]

https://github.com/sinonjs/fake-timers/pull/375/files#diff-5a5796b4730f7629082606dc9407d4b8a084ab5ec38803e6141743b4bbb9f7efR4744-R4751

Update: Eventually I used npm link with react (and react-app-rewire-alias) to run the code (after seeing what fails using the sauce labs credentials). I've used some of the testing code to recreate the errors - luckily, both Chrome and Firefox throw basically a similar error string that begins with: "Aborting after running 5 timers, assuming an infinite loop!" but the string continues in a different manner for each.

I'll soon edit this comment to add pictures. I'll continue working on it this evening :)

BTW, something is wrong with GitHub right now.. I tried commenting the code itself but I keep getting a lot of 500 and 502 error messages, that's why I just put a link, I'm sorry for the mess :)

[itayperry]

Hi @fatso83 (and @benjamingr ) :) All the tests now pass, I just pushed the fix, when you're available please update me if everything is really ok.

Tons of RegExp lol my head is spiraling!

Edit: this needs a rebase, I'll do it soon. Edit: I rebased and squashed all my commits together (I don't know if that was a smart thing to do...) and now I'm debugging the code to see why it fails in Node.

[itayperry]

BTW, I'm trying to run npm run prettier:write and I don't know why it fails..

I used prettier --write src/fake-timers-src.js and it worked

[fatso83]

@itayperry I see why it fails! It's because we are kind of a unix shop, and I see the "C:\Projects\fake-timers" string indicating you are not in that camp 😄 So NPM will run its script in the default shell on your machine. The default shell on your machine is CMD.exe, which does not know the {js,css,ms} syntax at all. It thinks it is a verbatim string to match.

So ... either accept that you need to do a manual prettier --write */*.js (or whatever syntax works on Windows) OR check out WSL2, which works great (I am developing on Windows these days using mainly WSL2 to get all my Unix fixes covered). If you just check out the project on WSL2 it will "just work".

[fatso83]

P.S. I have not checked out your code yet, but just a general thing: instead of trying to create One Regex To Rule Them All ™️ , you can try to split it up into different "branches" and test something depending on what matches some substring. Just to get the complexity down.

[itayperry]

The default shell on your machine is CMD.exe, which does not know the {js,css,ms} syntax at all. It thinks it is a verbatim string to match.

ohhh now I get it! thank you :) I've used the WSL2 once, I'll try and use it again after I finish this PR.

test something depending on what matches some substring. Just to get the complexity down.

I didn't think about it actually... RegExp can be quite intimidating sometimes for me. For now, what I did was to add an if to check whether the env is Node.js

[itayperry]

Update: now all the tests pass, which is wonderful :) [test-node, test-headless, test-cloud]

eslint yelled at me lol - it didn't work at all and I think it was updated or something so I tried npm install and now all hell broke loose.. I'm getting many warnings - even about code that's not in the PR.

I fixed the errors but there are still approximately 100 warnings.

EDIT: I can now see that code coverage is failing, I can sort of understand why (after googling about it), do you have any idea what I can do about it? Should I maybe add more tests?

And one more question :) Does the coverage run only these tests - [test-node, test-headless, test-cloud]?

EDIT: I debugged some of the tests, the lines that the coverage warns about are actually being used sometimes and they're not always skipped. This is weird.

[itayperry]

Perhaps I should use something like this: /* istanbul ignore if */ ? I'll try it :)

[fatso83]

Does this mean it actually runs in Firefox?

[itayperry]

@fatso83 Yes, it runs in Firefox, and from what I remember also in Chrome :)

[fatso83]

To me this sound like it's ready to merge :)

[SimenB]

@fatso83 can you release this? 🙂 (I guess major after #385 etc)

[fatso83]

Hi, @SimenB . Welcome back :smiley_cat: I actually thought that both you and Benjamin both had NPM release rights. Could arrange for that, rather than having you wait for me.

Not totally sure if there is something else breaking worth thinking about ... not quite sure why #385 would be breaking though. We have targetted Evergreen browsers for some time, so if ES2015+ features have snuck in, that's to be expected. My memory is old and faded, though :shrug:

[SimenB]

Ah, even better with no major 😀

[fatso83]

@SimenB I released it as v8.0.0 now, since I was a bit unsure about how the removal of TS bindings would affect current installs. Someone might see some things break, so better safe than sorry.

[SimenB]

Thanks!

[fatso83]

Bare hyggelig :norway:

[SimenB]

Funka nesten 😀

There's a bug here, I'm seeing Cannot read property 'stack' of undefined - will dig into this and send a PR

[fatso83]

amagad, should we mark v8 as deprecated and move the latest tag to v7.x?

[SimenB]

Yeah, probably

[fatso83]

Ah, you do have rights already.

$ npm show @sinonjs/fake-timers maintainers

```json
[
  'simenb <sbekkhus91@gmail.com>',
  'mrgnrdrck <morgan@roderick.dk>',
  'fatso83 <carlerik@gmail.com>',
  'mantoni <mail@maxantoni.de>',
  'cjohansen <christian@cjohansen.no>',
  'benjamin.gruenbaum <benjamingr@gmail.com>'
]