Custom formatter not respected by samsam/commons

[ajaska]

Sinon supports setting a custom formatter, but it is not used by sub-libraries.

To Reproduce

const sinon = require("sinon");
const inspect = require("util").inspect;

const obj = { a: () => {} };
const aSpy = sinon.spy(obj, "a");
obj.a({ foo: { bar: "baz" } });

sinon.setFormatter(inspect);

sinon.assert.calledWith(aSpy, sinon.match({ foo2: { bar2: "baz2" } }));

Output:

Error [AssertError]: expected a to be called with arguments
{ foo: { bar: 'baz' } } match(foo2: [object Object])

Expected behavior

Error [AssertError]: expected a to be called with arguments
{ foo: { bar: 'baz' } } match(foo2: { bar2: 'baz2' })

Context (please complete the following information):

• Library version: sinon@latest -- 13.0.1
• Environment: node
• Example URL:
• Other libraries you are using:

Additional context I believe this is because common's valueToString takes a simple approach and does not allow one to pass a custom formatter in. I believe a fix would involve updating samsam's createMatcher to accept a formatter, and using it (if specified) instead of common's valueToString.

[mroderick]

Thank you for taking the time to create an easy to follow issue 👍

Would you like to contribute a PR to samsam to improve the situation?

[mroderick]

This is an interesting one.

The setFomatter method was originally added in #1503, in order to allow Cypress to override the formatter.

We have a few options for making things consistent here.

1. Add a setFormatter option to @sinonjs/samsam
2. Remove setFormatter from sinon and make all @sinonjs/ libraries use node's inspect from the util package for formatting values
3. Use a different formatter?

Option 1 - Add a setFormatter option to @sinonjs/samsam

In order to solve the inconsistency and have some nicer output, we could expand @sinonjs/samsam with it's own setFomatter method, that could be called by sinon.setFormatter.

However, this would make @sinonjs/samsam stateful, which would have the potential to break people's tests in all sorts of interesting ways.

Ultimately, I don't think that libraries should be stateful.

Option 2 - Use util.inspect

Instead of making more libraries stateful, we remove setFormatter and use util.inspect for value formatting everywhere.

I tried this in a branch last night and got it working pretty quick 👍

In @sinonjs/referee we switched to using util.inspect in https://github.com/sinonjs/referee/pull/183. This has worked quite well, once we adapted the tests to account for util.inspect improving between the node versions.

However, this option will break Cypress ... or at least, it should break the tests in Cypress, if they're testing for specific strings in their error messages.

But ... Cypress is currently on [email protected] and in the npm registry we're at [email protected] ... I guess that means that it will be awhile before this becomes relevant for Cypress.

Option 3 - Use a different formatter?

This is a wild idea ... maybe we can get Cypress to share their formatter in a separate package and we can use that one?

---

Personally, I'd much prefer option 2, removing statefulness from libraries. However, I don't really like the idea that it has the option to cause a lot of failed tests for people using Sinon and Cypress.

---

@mantoni @fatso83 @chrisbreiding What do you think?

I'd love to hear your thoughts and ideas. Maybe there are options I haven't considered, or maybe there are options within the options that make them better or more viable.

[fatso83]

Hmm ... the easiest is certainly #2 . I do not think the method is used by a lot of people directly, but it might be relied upon implicitly by a lot people through libraries. Wrapping a tiny library such as samsam in a closure that holds a default formatter that is overridable should be pretty straight forward in a any case, so I would not mind option one either. Option 3 I do not really get anyway; it's fine to use another formatter, but we would still need to change our libs to use it.

[mantoni]

Thanks for looking into this @mroderick. This would be a breaking change anyway, right? I'd go with option 2. We already did this change in the other libraries and Sinon is now the only project that doesn't use util.inspect.

[mroderick]

This has been fixed with #2480 and has been released to the npm registry as v15.0.0