Add clearFields function to put logic clearing common fields in one place

[noisecapella]

Note: Please let me know if this solution is desirable behavior. I haven't yet added unit tests but I'm happy to add them if this PR implementation fits with the goals of the project.

Purpose (TL;DR) - mandatory

This PR adds a function clearFields to centralize the functionality resetting stub behavior if multiple behavior functions are called. This PR also uses clearFields on other behavior functions like callsFake.

Background (Problem in detail) - optional

The problem deals with the behavior of this situation:

stub.throws("TypeError");
stub.returns(3);
assert(stub() === 3);

returns resets certain fields including exception and exceptionCreator, effectively overriding the previous behavior.

I originally created #1511 which added the field exceptionCreator, and I noticed today that in #1517 it doesn't reset exceptionCreator. Also, my coworker told me that callsFake doesn't have the same behavior resetting behavior that returns does.

Solution - optional

I added the clearFields function which clears every field I could find referenced for default-behaviors.js, except for usingPromise since I wasn't sure if that should be reset or not.

There are a few extra behavior functions which clear fields which didn't clear fields before. The yield* functions were left alone because there were tests asserting that they didn't override returns, for example this one.

How to verify - mandatory

Run npm test

Checklist for author

• npm run lint passes
• References to standard library functions are cached.

[fatso83]

Seems like a good idea, but I have to admit I don't know the internals well enough to decide.

[mantoni]

This means that stub.yields(null, {}).returns(true) would not yield AND return anymore, right? Some behaviors can be combined and it makes sense. Apparently these combinations are not tested since the build here is green 🤔

[mantoni]

Oh, I didn't read carefully. You explicitly mentioned the yields + returns case already. I'm still a little unsure about the change because some combinations might happen to work and would break by this change ... For example callsArg is no different than yields and could be combined with returns.

[fatso83]

The basic combination logic here strikes me as extremely fragile and interconnected. Wouldn't this be one of the examples where refactoring to polymorphism actually makes proper sense? This is of course a bigger refactoring, but could help simplify all the conditionals we would need to handle all the combinations.

[mroderick]

Wouldn't this be one of the examples where refactoring to polymorphism actually makes proper sense? This is of course a bigger refactoring, but could help simplify all the conditionals we would need to handle all the combinations.

Do you mean refactoring Sinon to use polymorphism, or do you mean that @noisecapella's code should be refactored to use polymorphism?

[fatso83]

I just meant the internal behaviour code of Sinon. Or at least some way of splitting the logic up to make it manageable on a per-behavior basis.

[mroderick]

I don't think that the code in this PR makes it any less clear what is going on in the codebase.

However, the more interesting thing that @noisecapella shone a light on, is that some operations with stubs make absolutely no sense, yet are still supported.

stub.throws("TypeError");
stub.returns(3);

stub.resolves('some value');
stub.rejects(new Error('some error'));

These are contradictory statements, that silently change the stub's behaviour. Considering that many test scenarios have nested test setups, silently changing behaviour can lead to un-anticipated situations that might cause people's tests to pass when they really should not.

I think we have an opportunity to significantly improve Sinon by making non-sensical scenarios unsupported.

Perhaps we can go even further and also make a stub's behaviour (almost) immutable. By making it throw an Error when someone tries to change an already set behaviour, we can avoid the situations where complex test setups accidentally let tests pass.

stub.throws("TypeError");
stub.returns(3);
// => throws new Error('A stub cannot both throw and return a value');

stub.resolves('some value');
stub.rejects(new Error('some error'));
// => throws new Error('A stub cannot both resolve and reject a promise');

[mantoni]

Yes, I agree that some combinations should trigger Sinon exceptions.

How about certain orders of behaviors? For example

• stub.yields(null).throws('Error') invokes a callback, then throws
• stub.throws('Error').yields(null) should throw a Sinon exception because we cannot yield after throwing?

[noisecapella]

At my workplace we use a pattern like this:

beforeEach(() => {
  apiStub.throws(new Error("not implemented"));
});

it("populates the redux store", () => {
  apiStub.returns("some data");
  // test using that data
});

it('clicks a button', () => {
    // this test somewhere accesses apiStub but we didn't mock it out, so it will fail
});

The throws ensures that a test fails if we forgot to set the stub data in a particular test. That's my main motivation for this PR, that we could do something similar with callsFake in place of returns. But I understand this is undocumented and I'm willing to support whatever consistency is desirable here.

@mantoni That makes sense, I misunderstood callsArg here

[noisecapella]

I'm not sure if there's a consensus here so I'm going to close this and open two more specific pull requests: one for the missing exceptionCreator = undefined and one for clearing fields for callsFake which would be nice for my use case and see if that's a desired behavior