Skip to content

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


clock.next();
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 ...

@SimenB SimenB marked this pull request as draft September 22, 2021 09:45
@SimenB SimenB force-pushed the reset-is-near-on-uninstall branch from ffcc06a to 7411b8b Compare September 22, 2021 10:06
@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.10%. Comparing base (85463d9) to head (61cfb60).
⚠️ Report is 148 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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SimenB SimenB force-pushed the reset-is-near-on-uninstall branch from 7411b8b to 72adb63 Compare September 22, 2021 22:17
});
});

// 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

@stale
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.

3 participants