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

itayperry · 2021-05-01T16:41:28Z

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 · 2021-05-01T16:43:21Z

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%)`	⬆️

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%)`	⬆️

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 · 2021-05-03T08:48:26Z

So this fails in IE 11, Safari 9, Firefox. This ties in with sinonjs/sinon#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"
   }]
 }

per discussion in sinonjs#375 and sinonjs/sinon#2359

fatso83 · 2021-05-27T11:20:55Z

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

itayperry · 2021-05-28T14:04:52Z

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

fatso83 · 2021-05-28T14:25:56Z

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 · 2021-05-29T11:14:11Z

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 · 2021-05-29T16:14:03Z

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 · 2021-05-29T19:34:48Z

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 · 2021-05-29T20:20:00Z

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 · 2021-05-31T16:24:33Z

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 · 2021-06-02T08:54:59Z

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 · 2021-06-02T10:38:13Z

test/fake-timers-test.js

+                assert.equals(
+                    new RegExp(
+                        "Error: " +
+                            expectedMessage +
+                            "\\s+Microtask - recursiveQueueMicroTask\\s+at recursiveQueueMicroTask\\s"
+                    ).test(err.stack),
+                    true
+                );


That's the code that fails, I'm working on it.

itayperry · 2021-06-03T15:54:04Z

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 · 2021-06-03T16:02:10Z

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

Use stack information for jobs and timers to provide more context when an infinite loop error is thrown. Fixes sinonjs#230 Add flag for approaching infinite loop Use a flag when approaching infinite loop detection which will enable an error object to be attached to the job, which can then be used for generating the stack output. Update for PR feedback - Safely update stack property using `defineProperty`, wrapped in try/catch - Tests for more clock functions to confirm stack trace provided - Stack trace slice automatically calculated based on clock function position Remove unnecessary durations and use globals - Use global setInterval and setImmediate to match setTimeout use - Remove unnecessary duration parameter from setImmediate and setImmediate add use of prettier Firefox and Chrome tests pass after RegExp fix Chrome shows stack messages differently, and uses the words: 'at' and 'Object', Firefox does not - that has created two problems: first, the stack trace messages that are shown to the user are a bit different (Firefox always has at least one more line at the start of the stack list - usually internal functions that are not supposed to appear). Second, the RegExp tried to match parts of the strings that only exist in Chrome, failing the Firefox tests) prettier used

fatso83 · 2021-06-04T15:40:15Z

@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 · 2021-06-04T15:42:45Z

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 · 2021-06-05T09:49:24Z

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 · 2021-06-05T09:56:47Z

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 · 2021-06-06T10:40:37Z

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

fatso83 · 2021-06-08T08:02:40Z

Does this mean it actually runs in Firefox?

itayperry · 2021-06-08T08:42:59Z

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

fatso83 · 2021-06-08T08:48:02Z

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

SimenB · 2021-09-15T09:29:23Z

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

fatso83 · 2021-09-15T09:38:03Z

Hi, @SimenB . Welcome back 😺
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 🤷

SimenB · 2021-09-15T10:00:07Z

Ah, even better with no major 😀

fatso83 · 2021-09-15T11:05:13Z

@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 · 2021-09-15T11:09:31Z

Thanks!

fatso83 · 2021-09-15T12:07:40Z

Bare hyggelig 🇳🇴

SimenB · 2021-09-15T12:30:41Z

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 · 2021-09-15T13:10:16Z

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

SimenB · 2021-09-15T13:19:35Z

Yeah, probably

fatso83 · 2021-09-15T14:20:56Z

Ah, you do have rights already.

$ npm show @sinonjs/fake-timers maintainers
 
```json
[
  'simenb <[email protected]>',
  'mrgnrdrck <[email protected]>',
  'fatso83 <[email protected]>',
  'mantoni <[email protected]>',
  'cjohansen <[email protected]>',
  'benjamin.gruenbaum <[email protected]>'
]

fatso83 added a commit to fatso83/lolex that referenced this pull request May 3, 2021

Drop IE11 and Safari 9

b44d0fa

per discussion in sinonjs#375 and sinonjs/sinon#2359

fatso83 mentioned this pull request May 3, 2021

Drop IE11 and Safari 9 from Sauce Labs test runners #376

Merged

fatso83 added the awaiting response label May 28, 2021

itayperry mentioned this pull request May 31, 2021

Safari test run breaks in Sauce Lab due to missing W3C WebDriver support #380

Closed

itayperry commented Jun 2, 2021

View reviewed changes

itayperry force-pushed the bugfix/add-stack-trace branch from b42b461 to ffe4078 Compare June 4, 2021 14:11

small RegExp fix for node env + linting

bfcd38b

improving test-coverage statistics

6bab5f3

at attempt to ignore certain condition in nyc coverage

ee3e901

fatso83 merged commit 54c2e8d into sinonjs:master Jun 8, 2021

SimenB mentioned this pull request Sep 15, 2021

fix: bind infinite loop check to clock instance #398

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

itayperry commented May 1, 2021

codecov bot commented May 1, 2021 •

edited

Loading

fatso83 commented May 3, 2021 •

edited

Loading

fatso83 commented May 27, 2021

itayperry commented May 28, 2021

fatso83 commented May 28, 2021

itayperry commented May 29, 2021 •

edited

Loading

fatso83 commented May 29, 2021 •

edited

Loading

itayperry commented May 29, 2021

fatso83 commented May 29, 2021 •

edited

Loading

itayperry commented May 31, 2021 •

edited

Loading

itayperry commented Jun 2, 2021 •

edited

Loading

itayperry Jun 2, 2021

itayperry commented Jun 3, 2021 •

edited

Loading

itayperry commented Jun 3, 2021 •

edited

Loading

fatso83 commented Jun 4, 2021

fatso83 commented Jun 4, 2021

itayperry commented Jun 5, 2021

itayperry commented Jun 5, 2021 •

edited

Loading

itayperry commented Jun 6, 2021 •

edited

Loading

fatso83 commented Jun 8, 2021

itayperry commented Jun 8, 2021 •

edited

Loading

fatso83 commented Jun 8, 2021

SimenB commented Sep 15, 2021

fatso83 commented Sep 15, 2021

SimenB commented Sep 15, 2021 •

edited

Loading

fatso83 commented Sep 15, 2021

SimenB commented Sep 15, 2021

fatso83 commented Sep 15, 2021

SimenB commented Sep 15, 2021

fatso83 commented Sep 15, 2021

SimenB commented Sep 15, 2021

fatso83 commented Sep 15, 2021

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

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

Conversation

itayperry commented May 1, 2021

codecov bot commented May 1, 2021 • edited Loading

Codecov Report

fatso83 commented May 3, 2021 • edited Loading

fatso83 commented May 27, 2021

itayperry commented May 28, 2021

fatso83 commented May 28, 2021

itayperry commented May 29, 2021 • edited Loading

fatso83 commented May 29, 2021 • edited Loading

itayperry commented May 29, 2021

fatso83 commented May 29, 2021 • edited Loading

itayperry commented May 31, 2021 • edited Loading

itayperry commented Jun 2, 2021 • edited Loading

itayperry Jun 2, 2021

Choose a reason for hiding this comment

itayperry commented Jun 3, 2021 • edited Loading

itayperry commented Jun 3, 2021 • edited Loading

fatso83 commented Jun 4, 2021

fatso83 commented Jun 4, 2021

itayperry commented Jun 5, 2021

itayperry commented Jun 5, 2021 • edited Loading

itayperry commented Jun 6, 2021 • edited Loading

fatso83 commented Jun 8, 2021

itayperry commented Jun 8, 2021 • edited Loading

fatso83 commented Jun 8, 2021

SimenB commented Sep 15, 2021

fatso83 commented Sep 15, 2021

SimenB commented Sep 15, 2021 • edited Loading

fatso83 commented Sep 15, 2021

SimenB commented Sep 15, 2021

fatso83 commented Sep 15, 2021

SimenB commented Sep 15, 2021

fatso83 commented Sep 15, 2021

SimenB commented Sep 15, 2021

fatso83 commented Sep 15, 2021

codecov bot commented May 1, 2021 •

edited

Loading

fatso83 commented May 3, 2021 •

edited

Loading

itayperry commented May 29, 2021 •

edited

Loading

fatso83 commented May 29, 2021 •

edited

Loading

fatso83 commented May 29, 2021 •

edited

Loading

itayperry commented May 31, 2021 •

edited

Loading

itayperry commented Jun 2, 2021 •

edited

Loading

itayperry commented Jun 3, 2021 •

edited

Loading

itayperry commented Jun 3, 2021 •

edited

Loading

itayperry commented Jun 5, 2021 •

edited

Loading

itayperry commented Jun 6, 2021 •

edited

Loading

itayperry commented Jun 8, 2021 •

edited

Loading

SimenB commented Sep 15, 2021 •

edited

Loading