-
Notifications
You must be signed in to change notification settings - Fork 30
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
build(deps-dev): bump sinon from 17.0.1 to 19.0.2 #6302
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] <support@github.com>
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.
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
372593f
19.0.24eb4c4b
Use fixed 13.0.2 version968f403
Improve formatting. Add note on fix in fake-timers@13.0.2a5b03db
Add links to code that is affected by the breaking changes43e9dd1
19.0.1037ec2d
Update migration docs62724bb
19.0.0dda95c2
Update lock file3534ab4
Bump samsam and nise to latest versions (#2617)c9964eb
Remove 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 rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditions
will show all of the ignore conditions of the specified dependency@dependabot ignore this major version
will 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 version
will 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 dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)