-
Notifications
You must be signed in to change notification settings - Fork 1
fix: stop polling #292
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: stop polling #292
Conversation
if (nextTimer) { | ||
clearTimeout(nextTimer); | ||
nextTimer = undefined; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other tests that I can write to check that stopping the poller works. We already have this.
js-sdk-common/src/poller.spec.ts
Lines 181 to 188 in 94c6f67
it('stops polling', async () => { | |
const poller = initPoller(testIntervalMs, noOpCallback, { pollAfterSuccessfulStart: true }); | |
await poller.start(); | |
td.verify(noOpCallback(), { times: 1 }); | |
poller.stop(); | |
await jest.advanceTimersByTimeAsync(testIntervalMs * 10); | |
td.verify(noOpCallback(), { times: 1 }); | |
}); |
Any tips? @aarsilv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a test where we call stopPolling() immediately after making the poller but that may be overkill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jest.getTimerCount may be an option to check how many timers are remaining.
if (nextTimer) { | ||
clearTimeout(nextTimer); | ||
nextTimer = undefined; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a test where we call stopPolling() immediately after making the poller but that may be overkill
labels: mergeable
Fixes: #issue
Motivation and Context
Description
How has this been documented?
How has this been tested?