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

fix: bind infinite loop check to clock instance #398

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Sep 15, 2021

Purpose (TL;DR) - mandatory

Failing test from #375 (comment).

By not resetting, the counter is not tied to a specific clock but rather then entire withGlobal.

I'll push a second commit moving the attribute from living in the withGlobal closure to living on the clock object, which I think is more correct

@SimenB
Copy link
Member Author

SimenB commented Sep 15, 2021

Pushed a commit that's almost there - interval fails since the timer itself doesn't have an error object - only the last one does. Not sure how to deal with it? A single interval will run forever with runAll without us ever registering an error

checkIsNearInfiniteLimit(clock, i);
clock.next();
Copy link
Member Author

Choose a reason for hiding this comment

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

moving this after the check fixes all but the setInterval tests

@SimenB SimenB changed the title chore: add failing test for loop limit error fix: bind infinite loop check to clock instance Sep 15, 2021
@SimenB
Copy link
Member Author

SimenB commented Sep 15, 2021

@fatso83 I don't know how to deal with setInterval. Ideas?

One quick fix is of course to handle error not being present in getInfiniteLoopError and just return the error created there without trying to get a better trace

@SimenB
Copy link
Member Author

SimenB commented Sep 15, 2021

Pushed that latest thing - not ideal, but I don't know how to solve it properly

@SimenB SimenB marked this pull request as ready for review September 15, 2021 13:42
@fatso83
Copy link
Contributor

fatso83 commented Sep 15, 2021

This looks fine, but TBH I am just too tired to grok the details of this right now, so I cannot really say something very useful about the .error prop ...

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (85463d9) 94.10% compared to head (61cfb60) 94.10%.
Report is 97 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #398   +/-   ##
=======================================
  Coverage   94.10%   94.10%           
=======================================
  Files           1        1           
  Lines         611      611           
=======================================
  Hits          575      575           
  Misses         36       36           
Flag Coverage Δ
unit 94.10% <100.00%> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -5036,6 +5036,7 @@ describe("loop limit stack trace", function () {
});
});

// This doesn't really work since we're unable to add an error to all running intervals
Copy link
Contributor

Choose a reason for hiding this comment

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

Not totally sure what "this" refers to really. The tests or the setInteval function or what.

Copy link
Member Author

Choose a reason for hiding this comment

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

tracking of callbacks triggered by setInterval

Copy link

stale bot commented Dec 27, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 27, 2023
@fatso83
Copy link
Contributor

fatso83 commented Feb 10, 2024

Is this going somewhere? Can I help? Not totally sure what to do between your last commend and Benji's approval.

@stale stale bot removed the stale label Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants