Skip to content

Fix #1741: record calls on stubbed getters/setters - #2733

Open
youdie006 wants to merge 1 commit into
sinonjs:mainfrom
youdie006:feat/accessor-call-recording
Open

Fix #1741: record calls on stubbed getters/setters#2733
youdie006 wants to merge 1 commit into
sinonjs:mainfrom
youdie006:feat/accessor-call-recording

Conversation

@youdie006

Copy link
Copy Markdown

Purpose (TL;DR)

stub.get(fn) / stub.set(fn) installed the raw replacement function directly as the property accessor, so invoking a stubbed getter/setter ran untracked — callCount never incremented (#1741). This wraps the replacement in a spy and exposes it as stub.getter / stub.setter, so accessor invocations are recorded (mirroring the existing spy.get / spy.set surface). This is the scoped "tiny PR" @fatso83 green-lit in the issue thread.

How to verify

const obj = { get prop() { return 1; } };
const s = sinon.stub(obj, "prop").get(() => 42);
obj.prop; obj.prop;
s.getter.callCount; // 2  (previously: not recorded)

npx mocha test/src/stub-test.js --grep "records calls to the stubbed" → 2 passing.

Checklist

  • Lint passes (eslint --max-warnings 0; prettier clean)
  • Scoped + non-breaking (+19 lines in default-behaviors.js, no API removed); full suite 1554 passing / 0 failing locally.

Fixes #1741

stub.get(fn)/stub.set(fn) installed the raw replacement function as the property
accessor, so invoking the stubbed getter/setter ran untracked and never
incremented callCount. Wrap the replacement in a spy and expose it as
stub.getter/stub.setter so accessor invocations are recorded, mirroring the
spy.get/spy.set surface. Scoped and non-breaking.
@mroderick

Copy link
Copy Markdown
Member

This is a really clean fix. Thank you for picking it up, and for keeping it scoped — the diff is easy to follow and exactly addresses the issue.

A couple of nice things I noticed 👏

  • Wrapping in spy() is the right approach — routes through the existing call-recording machinery without reinventing anything.
  • this context is properly preserved through the spy proxy.
  • No circular dependency introduced (spy.js doesn't import default-behaviors.js).
  • The naming collision with stub.get/stub.set makes .getter/.setter the right call.

One small suggestion: resetBehavior in stub.js doesn't clean up .getter/.setter, so after a resetBehavior() the stub still holds stale spy references. It's a narrow window (calling .get() again overwrites it), but adding delete this.getter; delete this.setter; there would keep it clean.

Nice work on this!

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.

Cannot spy on property getters, and stubbed getters do not record calls

2 participants