Option to use assignment operator in injection methods.

[sripberger]

Is your feature request related to a problem? Please describe.
Yes. Sinon's injection and cleanup features are difficult to use for truly unobtrusive unit testing of function modules-- i.e. modules that just export some functions for consumers to use. This is fundamentally a result of how CommonJS modules work, and something which can be solved in ES modules.

Describe the solution you'd like
I'd like to add an optional argument to the sinon.stub and sinon.spy methods, specifically the signatures which take an object and a method name, for example const stub = sinon.stub(object, "method"); This optional argument would cause the injection logic to use a simple assignment instead of Object.defineProperty both to put the double in place, and to restore the original function during cleanup. Something like this:

const stub = sinon.stub(object, "method", {useAssignment: true});

This feature could be used to implement a standards-compliant solution for getting test doubles in place that doesn't require major changes to how code is typically written in modules like these. Under additional context below I have a much more exhaustive explanation of why this is the case, if anybody cares to read a bunch.

Describe alternatives you've considered
I am considering writing a wrapper around sinon that duplicates some of this injection logic, but it would be easier and better if I didn't have to do that. Additional context explains several other alternatives I have used historically and why they don't really work with ES modules.

Additional context
Sinon is fantastic and one of my favorite things about working in Node.js. Not only is the API expressive and powerful, it typically makes injection of test doubles very easy and doesn't force developers to drastically restructure their code in order to accommodate it.

Want to stub some behavior in a class? Just make it into an instance method, construct an instance like normal, then call sinon.stub(instance, "method"). It doesn't really get much simpler than that. No need to say, implement this behavior in a separate class and then ask all consuming code to provide this implementation every time. You can still do this if you think it's appropriate, but you're not forced to do it all the time, which I love.

This simplicity breaks down in one case, though, and that is when dealing with function modules-- i.e. modules that just contain functions which are not meant to be used as class constructors. Historically, in CommonJS, you are forced to consider how variables defined in the top scope of a module are fully hidden and cannot be affected from outside the module. If you write your modules like this, for example (and these are all intentionally-trivial examples):

my-module.js

function doThing() {
    console.log("Wow, we did the thing!");
}
exports.doThing = doThing;

function doThingTwice() {
    for (let i = 0; i < 2, i +=1) doThing();
}
exports.doThingTwice = doThingTwice;

You cannot stub doThing for a doThingTwice unit test, because this will not work:

my-module.js

const myModule = require("./my-module");
const sinon = require("sinon");

sinon.stub(myModule, "doThing");

myModule.doThingTwice();

sinon.assert.calledTwice(myModule.doThing);

As I'm sure most readers will know, this happens because the doThing variable referenced in the doThingTwice body cannot be affected by sinon-- or anything else really. All sinon is doing in this test code is replacing the reference set on the exports object, not the reference within the module under test itself. This has always been true whether you use the pattern of assigning onto exports or the module.exports pattern:

my-module.js

function doThing() {
    console.log("Wow, we did the thing!");
}

function doThingTwice() {
    for (let i = 0; i < 2, i +=1) doThing();
}

module.exports = {doThing, doThingTwice};

The only real workaround is to consistently refer to all other exported functions through the exports object, even within the module itself:

my-module.js

exports.doThing = function() {
    console.log("Wow, we did the thing!");
}

exports.doThingTwice = function() {
    for (let i = 0; i < 2, i +=1) exports.doThing();
}

This will work, but it sucks for a few reasons. It's tedious and arguably interferes with the readability and organization of your code. It also forces you to continue doing this even in consuming modules for which you may want to write more unit tests. Let's say I have another module:

other-module.js

const {doThing} = require("./my-module");

export function doThingThrice() {
    for (let i = 0; i < 3, i +=1) doThing();
}

Unsurprisingly, this doesn't work either:

other-module.js

const myModule = require("my-module");
const otherModule = require("other-module"");
const sinon = require("sinon");

sinon.stub(myModule, "doThing");

otherModule.doThingThrice();

sinon.assert.calledThrice(myModule.doThing);

Instead, we're forced to write other-module this way instead:

other-module.js

const myModule = require("./my-module");

export function doThingThrice() {
    for (let i = 0; i < 3, i +=1) myModule.doThing();
}

In addition to being tedious, this seems to be a source of some frustration of confusion with which I'm sure maintainers of this project are familiar. It is possible to deal with it to some extent using a transpiler like Typescript-- writing named imports and exports then stubbing them like this:

my-module.ts

export function doThing() {
    console.log("Wow, we did the thing!");
}

other-module.ts

import {doThing} from "./my-module";

export function doThingThrice() {
    for (let i = 0; i < 3, i +=1) doThing();
}

test code

import * as myModule from "./my-module";
import {doThingThrice} from "./other-module";
import sinon from "sinon";

sinon.stub(myModule, "doThing");

doThingThrice();

sinon.assert.calledThrice(myModule, "doThing");

This does actually work because of how Typescript transpiles. All named export references are transpiled to references through an object returned on require. This is an attempt to emulate ES6 modules using CommonJS and I don't think it'd be wrong to think of it as an implementation detail of tsc, so while it works it's probably not a good idea to rely on it.

Additionally, all of these approaches break down in the presence of actual ES modules, which is a bit of a problem for anybody hoping to move into the future. Sinon correctly throws out an error instead of trying (and inevitably failing) to overwrite a property on a module like I've been demonstrating.

ES modules, however, are not actually immutable at run time. The lexical bindings of imports and exports are, but the variable references exported from the original module are not. This is, if I'm not mistaken, one of the central goals of the ES module standard and why it is able to solve circular dependencies, among other things.

For example, if you do this:

counter.mjs

export let counter = 0;

export function addOne() {
    counter += 1;
}

You can then do this:

import {counter, addOne} from "./counter.mjs";

console.log(counter);
// 0

addOne();

console.log(counter):
// 1

addOne();

console.log(counter);
// 2

This is of course not magic. It's standards-compliant behavior and one of the core reasons why CommonJS is not really compatible with the ES standard. In addition to many other things, it enables modules to define their own API's for swapping stuff out with test doubles:

my-module.mjs

export function doThing() {
    console.log("Wow, we did the thing!");
}

export function doThingTwice() {
    for (let i = 0; i < 2, i +=1) doThing();
}

export function stubDoThing(stub) {
    doThing = stub;
    return stub;
}

test code

import {doThingTwice, stubDoThing);
import sinon from "sinon";

const stub = stubDoThing(sinon.stub().named("doThing"));

doThingTwice();

sinon.assert.calledTwice(stub);

Of course, this very much not ideal because we can't use sinon's nice sandbox features for easy cleanup. sinon.restore will not fix this and put the original back into place, even if we define another function in the module for doing so. We'll have to call all of these functions in our test teardown hooks to make sure we don't leak stubs between tests. More tedium, more confusing stuff that's easy to mess up, etc...

There is a potential solution, though. if we have our modules export an additional object with getters, then we're close to just using sinon as-is:

my-module.mjs

export function doThing() {
    console.log("Wow, we did the thing!");
}

export function doThingTwice() {
    for (let i = 0; i < 2, i +=1) doThing();
}

export const myModule = {
    get doThing() {
        return doThing;
    },
    set doThing(value) {
        doThing = value;
    }
};

This test code will work but it still has the same problem with restoring as above:

import {doThingTwice, myModule} from "./my-module.mjs";
import sinon from "sinon";

const stub = myModule.doThing = sinon.stub().named("doThing");

doThingTwice();

sinon.assert.calledTwice(stub);

This test code, however, will not work:

import {doThingTwice, myModule} from "./my-module.mjs";
import sinon from "sinon";

const stub = sinon.stub(myModule, "doThing");

doThingTwice();

sinon.assert.calledTwice(stub);

The second bit of test code will run, but the assertion will always fail because the stub is never put where we want it to go. Instead, it ends up on the myModule object and not being referenced within my-module itself.

This happens because sinon uses Object.setProperty instead of the assignment operator when putting stubs in place. This, I believe, is an entirely reasonable thing to do in many cases, because it allows you to keep the property from being treated as an enumberable own property of the object on which it exists. Just assigning could cause problems for many reasons, especially in the case of instance methods which are typically properties on a prototype object, and don't exist directly on instances.

All that being said, though, I think it would be massively helpful to add an optional argument to the sinon.stub method, as well as any other injection methods like sinon.spy, which could cause assignment to be used instead of Object.setProperty for both the initial injection and the restoration that occurs with the restore methods. Then, we could just do something like this in our test code:

import {doThingTwice, myModule} from "./my-module.mjs";
import sinon from "sinon";

const stub = sinon.stub(myModule, "doThing", {useAssignment: true});

doThingTwice();

sinon.assert.calledTwice(stub);

This would do what we want without interfering with how any existing stubs are already being injected, and documentation of this option could include some of my examples to explain what it is for so people don't misuse it and get confused.

This would be far more ideal than any other approach I've used so far, despite the need for being explicit with this option and adding boilerplate getters and setters for all the modules. It is far superior, though, because that boilerplate is not mixed in with the functions as-written, so it doesn't interfere with readability or organization. More crucially, it is compliant with the ES standard and does not involve compromising at all on the use of lexically-scoped named exports. It doesn't force us to use proxyquire or any of the other crazy runtime stuff people have historically relied upon to do things like this.

Hopefully I've made my case, otherwise I believe I'll just end up writing some kind of a wrapper around sinon which reproduces injection, sandbox, and restore functionality backed by assignment operators, which I would prefer not to do. :O

[mantoni]

Thank you for taking the time to write down your thoughts and share details about esm that I didn't know yet about. I certainly learned something 👍❤️

Interestingly, Sinon used to stub properties by simple assignments, until users reported issues with special cases (as you mentioned), which I believe @fatso83 solved with the property descriptor based implementation. So much for the history.

Without having experimented with what you layer out for esm modules, my gut feeling tells me that we might be able to solve this without an additional option: If we detect a property descriptor that allows for swapping out the property with a simple assignment, we could do just that and use the more elaborate property descriptor implementation for the special cases only.

Is that something that would work in your case? You seem to have a lot of experience with esm and real world code under your fingers that you can experiment with. Would you be interested in working on a PR?

[sripberger]

Auto-detection for this would be ideal if it is possible without breaking stuff for other people, I agree. I'd be happy to work on a PR. I may or may not have the capacity for it in the next two weeks, but if so I'll give it a shot. 👍

[fatso83]

@sripberger If you ever get the time, feel free to have a go. I am not in a life situation that permits focused work on something like this for the foreseeable future, and this requires a clear head and some hours to spare (as your long text basically already said).

[fatso83]

@mantoni We do the reverse of this today in wrap-method.js, used by spy and stub modules: if we support object descriptors (ES5), use those, otherwise fall back to simple assignment. Wouldn't this essentially revert this functionality?

@sripberger I know this is old, but I just now re-read this and saw what a great case you made for this. That being said, I am not sure how we would be able to retro-fit this consistently into the various injection APIs. For instance, sinon.spy takes a third argument already (the types of spies, used for property accessors).

The more "modern" (newer and simpler) APIs we ship with the sandboxes to replace properties seem to do exactly what you want, replacing properties with simple assignment. Would not your example work out of the box with it today?

const stub = sinon.stub()
sinon.replace(myModule, 'doThing', stub)

doThingTwice()

sinon.assert.calledTwice(stub)

EDIT: I just tried and I got an error that basically informs me we did not think this something anyone wanted 😄

$ node my-module.test.mjs 
file:///Users/carlerik/code/nimble/frontend/node_modules/.pnpm/[email protected]/node_modules/sinon/pkg/sinon-esm.js:3064
            throw new Error("Use sandbox.replaceGetter for replacing getters");
                  ^

Error: Use sandbox.replaceGetter for replacing getters

[fatso83]

I think this is very near to being a reality and thus a super way of enabling stubbing of ESM code without using custom ESM loaders such as Quibble (which I described how to do in XXX).

There are two things stopping this from working fine today:

1. Presence of accessors will throw. It works if removing these, but I think it would be better to simply add an argument that says forceAssignment: true for these.
2. Sandboxes presently forget to clean up spies on property accessors (Spies for property accessors are not automatically cleaned up #2534)

[mantoni]

Hm. I just played with this in node 20, and my findings are different from the description in this PR – I guess things have changed in the meantime.

I have the following files in the root of the Sinon project:

esm.mjs

export function test() {
    return null;
}

esm.test.mjs

import sinon from "./lib/sinon.js";
import * as m from "./esm.mjs";

sinon.replace(m, "test", sinon.fake());

When I run node esm.test.mjs I get

TypeError: Cannot assign to read only property 'test' of object '[object Module]'
    at Sandbox.replace (/Users/max/projects/sinon/sinon/lib/sinon/sandbox.js:276:26)

This leads to the line where the sandbox is trying to define the property, and it's already using an assignment:

        object[property] = replacement;

I tried using Object.defineProperty with all kinds of variations, with that just gives me

TypeError: Cannot redefine property: test
    at Function.defineProperty (<anonymous>)
    at Sandbox.replace (/Users/max/projects/sinon/sinon/lib/sinon/sandbox.js:278:20)

I checked and the module object is sealed and the properties have the descriptor configurable: false. However, according to the docs on mdn, it should still be possible to reassign the value.

What am I missing?

[fatso83]

@mantoni You are missing the "middle man" export. See the myModule export above. The namespace is immutable, but you can still change the exported references from within the module. This is different from CJS. @sripberger achieves this by exporting accessor properties on the myModule export. Currently, trying to use replace on myModule fields will through, asking us to use replaceSetter instead.

EDIT: I clearly did not understand how this worked on first reading, either, so maybe you could also try re-reading this bit to experience the belated enlightenment I had 😄

[mantoni]

Ah, I understand now. However, the proposed solution requires a "backdoor" for testing. I personally prefer a custom module loader for use cases like this.

However, if you want to make it work with an explicit additional config on the stub for the sandbox to invoke the setter, then why not.

[fatso83]

In any case, for this to have any value, we need to fix the bug regarding missing cleanup of spies on accessors: #2534
✅

[fatso83]

@sripberger I made the necessary changes for this to work with sinon.replace. You might want to take a look at #2538 . This will allow easy stubbing and restoring of ES Modules via sandboxes as you outlined.

@mantoni Care to take a look at the PR? It's quite trivial.

[sripberger]

@fatso83 & @mantoni: So awesome to see progress here. It's very much appreciated!

Apologies for kinda going MIA here. I had actually planned on helping out, but my work situation went a bit sideways following this post, leading to a period of Sisyphean proprietary work, a months-long burnout, and a forced mental health break from the whole industry.

I've fully returned to work now, though, and I once again find myself looking to standardize ES modules and unit testing practices in an organization, so the timing of this couldn't be better.

So thanks again! 👏

[fatso83]

@sripberger do you have input for the naming issues of #2538 ? Kinda stuck on this stupid thing

[mantoni]

Did we already consider sinon.assign(obj, prop, fake)?

[fatso83]

@mantoni Ha, assign could work, but I was just wondering if people would not get a bit confused without specifying that it is supposed to be used with accessors. As in assignUsingAccessors, but we could clarify that both in the docs and with error messages.

sinon.assign(obj, foo', myStub); 
// => throws "TypeError: Sandbox#assign(...) is meant to assign values on objects that expose 
//                    accessors (getters/setters). Consider using Sandbox#replace(...)

Concerning methods and docs mentioning Sandbox, I am unsure if we want users to think about sandboxes, given that sandboxes are used implicitly using the Sinon instance itself, anyway.

[fatso83]

This went live with Sinon 16.1 some time ago. I just forgot closing this.

Bear in mind that the implementation does not add a third/fourth option to all the legacy methods. That was too cumbersome and would end up being very inconsistent in practice.

Instead we exposed the general method sandbox.replace.usingAccessor(object, property, value);

Value could be whatever you choose, not limited to functions. It will be auto cleaned up. Worth keeping in mind is that the default sinon export IS a sandbox as well, so you can just do sinon.replace.usingAccessor(object, property, value);