-
Notifications
You must be signed in to change notification settings - Fork 28
build(deps-dev): bump sinon from 17.0.1 to 19.0.2 #6302
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
Conversation
01f7464 to
9b77329
Compare
9ef6db9 to
eb959e0
Compare
eb959e0 to
15367c5
Compare
|
@dependabot rebase |
Bumps [sinon](https://github.com/sinonjs/sinon) from 17.0.1 to 19.0.2. - [Release notes](https://github.com/sinonjs/sinon/releases) - [Changelog](https://github.com/sinonjs/sinon/blob/main/docs/changelog.md) - [Commits](sinonjs/sinon@v17.0.1...v19.0.2) --- updated-dependencies: - dependency-name: sinon dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
15367c5 to
8ecda5d
Compare
|
This is failing because of the |
| clock = sinon.useFakeTimers({ global }) | ||
| sinon.stub(global, 'setTimeout').callsFake(() => 100) | ||
| sinon.stub(global, 'clearTimeout') |
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.
@eatyourgreens the use of sinon.useFakeTimers() was added in #3019. I'm not totally sure of the difference between .setFakeTimers() vs .stub(global) here, but reverting to .stub() allows the tests to pass. Any thoughts on this sinon method?
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.
From the release notes for 19, they’ve upgraded Fake Timers to the latest version.
Do you know what the classification queue tests are using the fake clock for?
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.
The docs for fake timers are here but, as noted, it’s just a thin wrapper for the Fake Timers package. These docs do explain config.global.
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.
With your change, the tests run using the local clock ie. the clock continues to tick during the tests. With fake timers, I think the test clock doesn’t tick unless you tell it to tick.
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.
This test now looks like it’s evergreen to me, because timeouts can’t be set by the mocks. If there were bugs in the timeout code, I think the mocks might hide them.
front-end-monorepo/packages/lib-classifier/src/store/utils/ClassificationQueue.spec.js
Lines 206 to 210 in 2de1f3e
| it('should not set a timer to retry failed classifications', async function () { | |
| expect(classificationQueue.flushTimeout).to.be.null() | |
| await classificationQueue.add(classificationData) | |
| expect(classificationQueue.flushTimeout).to.be.null() | |
| }) |
|
|
||
| before(function () { | ||
| clock = sinon.useFakeTimers({ global }) | ||
| sinon.stub(global, 'setTimeout').callsFake(() => 100) |
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.
You've changed the return type of My mistake, setTimeout here.setTimeout returns an integer, not a timeout.
This should really mock setTimeout with a function that creates a mock timeout object and returns its ID. Then clearTimeout should destroy the mock timeout for a given ID.
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.
Fake Timers is useful here because it will take care of that mocking for you.
https://github.com/sinonjs/fake-timers#api-reference
| before(function () { | ||
| clock = sinon.useFakeTimers({ global }) | ||
| sinon.stub(global, 'setTimeout').callsFake(() => 100) | ||
| sinon.stub(global, 'clearTimeout') |
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.
Similarly, this should mock clearTimeout with a function that takes a timeout and clears it.
By turning off timeouts, you've turned off retries in the classification queue. That's potentially a problem because the tests check whether or not a retry has been scheduled for a given response.
Have you tried https://github.com/sinonjs/fake-timers?tab=readme-ov-file#faking-the-native-timers |
|
Using |
|
After a bit of searching, I think it's this issue with Nock uses Node's internal timers, but fake timers turns those off. |
|
Fixed in #6544. |
This is good to know about the ClassificationQueue tests. Thank you for the help! |
Configure the fake timers for `ClassificationQueue` tests so that they only mock `setTimeout` and `clearTimeout`.
You're welcome. This made me realise that I've been lazy about mockiing in the past, and globally mocked timer functions that might be used in third-party libraries. |
Bumps sinon from 17.0.1 to 19.0.2.
Changelog
Sourced from sinon's changelog.
... (truncated)
Commits
372593f19.0.24eb4c4bUse fixed 13.0.2 version968f403Improve formatting. Add note on fix in [email protected]a5b03dbAdd links to code that is affected by the breaking changes43e9dd119.0.1037ec2dUpdate migration docs62724bb19.0.0dda95c2Update lock file3534ab4Bump samsam and nise to latest versions (#2617)c9964ebRemove referee-sinon introduced by recent mochify merge (#2616)You can trigger a rebase of this PR by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)