search

sinonjs / sinon

Test spies, stubs and mocks for JavaScript.
https://sinonjs.org/
Other
9.63k stars 769 forks source link

Sinon 15.0.2 update seems to break `callThrough` functionality #2501

Closed migg24 closed 1 year ago

migg24 commented 1 year ago

Describe the bug Updating sinon from 15.0.1 to 15.0.2 breaks callThrough functionality.

To Reproduce Steps to reproduce the behavior:

Check 15.0.1 behaviour in min reproduction example: https://stackblitz.com/edit/node-cg7mbz?file=index.spec.js&view=editor Execute npm test in terminal.

Check 15.0.2 behaviour in min reproduction example: https://stackblitz.com/edit/node-k36ehn?file=index.spec.js&view=editor Execute npm test in terminal.

Expected behavior callThrough should work and in 15.0.2 example the same line should be logged.

Screenshots Working in 15.0.1: image

Not working in 15.0.2: image

Context (please complete the following information):

Additional context

fatso83 commented 1 year ago

Both of these fail with the same maximum call stack error image image

fatso83 commented 1 year ago

Just deleted my previous comment about not being able to reproduce

I first failed to reproduce the issue, as I copied the verbatim code, but they used non-absolute version numbers and so resultet in 15.0.2 being used in both variations. "^15.0.1" will include patch releases, whereas "15.0.1" will never move.

So verified!

fatso83 commented 1 year ago

Works in version 14, 15.0.0, 15.0.1, but broken in 15.0.2. Probably by my doing, if I am not mistaken 😢 Means we have holes in our test coverage!

Verification testcase that is runnable based on yours, but simplified

https://stackblitz.com/edit/node-3cmnrm (not runnable for the same reasons as above, getting that stack issue)

https://runkit.com/fatso83/sinonjs-sinon-2501 (runnable, and breaking on 15.0.2)

``` $ npm i sinon@"15.0.0" && npx jest added 2 packages, removed 1 package, changed 1 package, and audited 291 packages in 892ms 32 packages are looking for funding run `npm fund` for details found 0 vulnerabilities console.log val1: TestService:doSomething:hei at Object.log (index.spec.js:17:13) console.log val2: undefined at Object.log (index.spec.js:18:13) PASS ./index.spec.js sinon breaking change ✓ should call through (22 ms) Test Suites: 1 passed, 1 total Tests: 1 passed, 1 total Snapshots: 0 total Time: 0.309 s, estimated 1 s Ran all test suites. carlerik at FAT-WORKSTATION in ~/dev/repro $ npm i sinon@"15.0.1" && npx jest removed 2 packages, changed 1 package, and audited 289 packages in 732ms 32 packages are looking for funding run `npm fund` for details found 0 vulnerabilities console.log val1: TestService:doSomething:hei at Object.log (index.spec.js:17:13) console.log val2: undefined at Object.log (index.spec.js:18:13) PASS ./index.spec.js sinon breaking change ✓ should call through (19 ms) Test Suites: 1 passed, 1 total Tests: 1 passed, 1 total Snapshots: 0 total Time: 0.267 s, estimated 1 s Ran all test suites. carlerik at FAT-WORKSTATION in ~/dev/repro $ npm i sinon@"15.0.2" && npx jest added 1 package, changed 1 package, and audited 290 packages in 1s 32 packages are looking for funding run `npm fund` for details found 0 vulnerabilities console.log val1: undefined at Object.log (index.spec.js:17:13) console.log val2: undefined at Object.log (index.spec.js:18:13) FAIL ./index.spec.js sinon breaking change ✕ should call through (21 ms) ● sinon breaking change › should call through expect(received).toEqual(expected) // deep equality Expected: "TestService:doSomething:test1" Received: undefined 17 | console.log("val1: ", stubTestService.doSomething("hei")); 18 | console.log("val2: ", stubTestService.doSomethingElse("hei")); > 19 | expect(stubTestService.doSomething("test1")).toEqual( | ^ 20 | "TestService:doSomething:test1" 21 | ); 22 | expect(stubTestService.doSomethingElse("test2")).toEqual(undefined); at Object.toEqual (index.spec.js:19:50) Test Suites: 1 failed, 1 total Tests: 1 failed, 1 total Snapshots: 0 total Time: 0.289 s, estimated 1 s Ran all test suites. ```

image

fatso83 commented 1 year ago

verified issue with git bisect to be commit 19bd99f364ab44f0e2715571e5deab580d9aa7fd

``` $ git bisect bad You need to start by "git bisect start" Do you want me to do it for you [Y/n]? y status: waiting for both good and bad commits status: waiting for good commit(s), bad commit known carlerik at diffia-kraftkar in ~/code/sinon (main|BISECTING) $ git checkout v15.0.0 Note: switching to 'v15.0.0'. You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by switching back to a branch. If you want to create a new branch to retain commits you create, you may do so (now or later) by using -c with the switch command. Example: git switch -c Or undo this operation with: git switch - Turn off this advice by setting config variable advice.detachedHead to false HEAD is now at 35652f37 15.0.0 carlerik at diffia-kraftkar in ~/code/sinon ((v15.0.0)|BISECTING) $ npx mocha repro.test.cjs sinon breaking change val1: TestService:doSomething:hei val2: undefined ✔ should call through 1 passing (21ms) carlerik at diffia-kraftkar in ~/code/sinon ((v15.0.0)|BISECTING) $ git bisect good Bisecting: 5 revisions left to test after this (roughly 3 steps) [45be60f3c6afc350eacbceed77539f437a9bbbce] Replace probot/stale with official stale action carlerik at diffia-kraftkar in ~/code/sinon ((45be60f...)|BISECTING) $ git bisect run npx mocha repro.test.cjs running 'npx' 'mocha' 'repro.test.cjs' sinon breaking change val1: TestService:doSomething:hei val2: undefined ✔ should call through 1 passing (22ms) Bisecting: 2 revisions left to test after this (roughly 2 steps) [8663ffa056d3c58e82fa203801d58d3fce3c14a7] Upgrade deps (#2498) running 'npx' 'mocha' 'repro.test.cjs' sinon breaking change val1: TestService:doSomething:hei val2: undefined ✔ should call through 1 passing (20ms) Bisecting: 0 revisions left to test after this (roughly 1 step) [7838b57aef902dc3cd020caeb6162be7f5d043ac] 15.0.2 running 'npx' 'mocha' 'repro.test.cjs' sinon breaking change val1: undefined val2: undefined 1) should call through 0 passing (24ms) 1 failing 1) sinon breaking change should call through: AssertionError: [assert.equals] undefined expected to be equal to 'TestService:doSomething:test1' at Object.fail (node_modules/@sinonjs/referee/lib/create-fail.js:5:21) at Object.fail (node_modules/@sinonjs/referee/lib/define-assertion/index.js:47:17) at assertion (node_modules/@sinonjs/referee/lib/define-assertion/index.js:65:11) at Function.referee.. [as equals] (node_modules/@sinonjs/referee/lib/define-assertion/index.js:92:22) at Context. (repro.test.cjs:21:16) at processImmediate (node:internal/timers:466:21) Bisecting: 0 revisions left to test after this (roughly 0 steps) [19bd99f364ab44f0e2715571e5deab580d9aa7fd] Use no-op for every function when restoring instances (#2499) running 'npx' 'mocha' 'repro.test.cjs' sinon breaking change val1: undefined val2: undefined 1) should call through 0 passing (22ms) 1 failing 1) sinon breaking change should call through: AssertionError: [assert.equals] undefined expected to be equal to 'TestService:doSomething:test1' at Object.fail (node_modules/@sinonjs/referee/lib/create-fail.js:5:21) at Object.fail (node_modules/@sinonjs/referee/lib/define-assertion/index.js:47:17) at assertion (node_modules/@sinonjs/referee/lib/define-assertion/index.js:65:11) at Function.referee.. [as equals] (node_modules/@sinonjs/referee/lib/define-assertion/index.js:92:22) at Context. (repro.test.cjs:21:16) at processImmediate (node:internal/timers:466:21) 19bd99f364ab44f0e2715571e5deab580d9aa7fd is the first bad commit commit 19bd99f364ab44f0e2715571e5deab580d9aa7fd Author: Carl-Erik Kopseng Date: Sun Mar 12 22:08:29 2023 +0100 Use no-op for every function when restoring instances (#2499) * issue-2477 Use no-op for every function when restoring instances * Change wording to not depend on mutator type in tests lib/sinon/stub.js | 7 ++++++- lib/sinon/util/core/walk-object.js | 20 +++++++++++++++----- test/restore-object-test.js | 5 ++++- test/sandbox-test.js | 19 +++++++++++++++++++ test/stub-test.js | 5 ++++- test/util/core/walk-object-test.js | 4 ++-- 6 files changed, 50 insertions(+), 10 deletions(-) bisect found first bad commit ```
migg24 commented 1 year ago

Thank you very much! Can confirm that the issue is resolved in 15.0.3.