Arguments of mocked functions are saved by reference.

[izikl]

Description

Arguments of mocked functions are saved by reference.

const f = td.function();
p.some = "1";
f(p)
p.some = "2" 
f(p)

I expect explain to show calls to f with some="1" and some="2". However, it shows it with some="2" twice.

Issue

The code example below printed the following:
name: Salvador last: Sobral <-- first call
name: Netta last: Barzilai
Call 1. name: Netta last: Barzilai <-- first call according to expect
Call 2. name: Netta last: Barzilai

Environment

• node -v output: v8.11.2
• npm -v (or yarn --version) output: 5.6.0
• npm ls testdouble (or yarn list testdouble) version: testdouble@3.8.1

Code-fenced Examples

var td = require('testdouble')

class Person { }
const p = new Person();

const f = td.function();
td.when(f(td.matchers.anything())).thenDo((p) => { console.log("name: " + p.first + " last: " + p.last); });

// first call
p.first = "Salvador";
p.last = "Sobral";
f(p);

// second call 
p.first = "Netta";
p.last = "Barzilai";
f(p);

// explain has "Netta Barzilai" twice
td
  .explain(f)
  .calls
  .forEach((call, index) => {
  call.args.map((arg) => console.log("Call " + (index + 1) + ". name: " + arg.first + " last: " + arg.last));
});

[searls]

Thanks for raising this issue. I guess it's a testament to our users that for ~3 years nobody has actually encountered and reported an issue having to do with mutating values passed to the library.

There are a few things to unwind here:

1. If an object is passed to td.when(foo(bigThing)).thenReturn('lol') and between that stubbing configuration and the production-scoped invocation of foo(), bigThing is mutated, should the stubbing be considered satisfied?
2. Similarly, if bar(otherThing) is called and then otherThing is mutated somehow, should td.verify(bar(otherThing)) pass or fail?
3. If each fake function's call log were to deep clone each argument passed (as opposed to saving a reference), what unintended consequences might there be on either of the above? (Additionally, what would the performance hit be? And what might happen for uncloneable types like Error?)
4. If we decide to change none of the above, is it worth the performance hit to save off additional copies of all arguments and contexts on every invocation just so that td.explain will be accurate on the off-chance the user ever calls it? That may prove to be a very high price.

Taken in isolation, I don't have great answers to any of these three questions. My only stance initially on this is to approach the idea of changing this behavior with trepidation, given that we've gone several years and hundreds of projects without this ever raising an actual issue.

Would love other opinions on this.

[izikl]

@searls thanks for taking the time to reply to this issue.

I will try to answer your first 2 questions, Ignoring performance and implementation difficulty and focus on usability only:

1. I believe the stubbing should be considered satisfied. The object's properties may have changed but it is still the same object.
2. I believe the verify should pass. Same reason as in the first point.

However, if otherThing and bigThing in your examples were replaced with td.matchers.contains({ bigBang: y }) then I am expecting the matching to be based on the properties of the object at the moment of the invocation.

To avoid the performance hit and the possible implementation pitfalls, I think it is ok to keep just a copy of the matching properties (bigBang in my example above) and not the entire object and take those copies into consideration when deciding if stabbing should be satisfied or if verify should pass.

It will not solve my example above, but I believe it will solve 99% of the use cases.

[searls]

I've never seen or heard of the problem your (more narrow) proposed solution would fix, so I'm not especially inclined to try to fix it until we see data that this is a real pain point for people. For stubbing, "matching to be based on the properties of the object at the moment of the invocation" is already how it works, since arguments and matchers passed to td.when are almost always specified with literals. For verification, there's the chance for a problem but there isn't much we can do about it, as the API requires us to not know anything about how interactions will be td.verify()'d until after the subject is invoked (and these sorts of mutations occur).

Moreover, I'm struggling to think of cases when this could happen where the subject code wouldn't be exhibiting the code smell of operating at multiple levels of abstraction (if we've faked our dependencies and make sure blah is passed between them, great, but if the function also goes out of its way to mutate blah with its own implemented logic, that's diving a layer deeper relative to its other behavior). As a result, since tdjs is first and foremost a design feedback tool, that sort of knotty design are not really things we want to enable by safeguarding users.

I'm going to close until we observe more examples of problem cases, so we can find real-world pain that isn't exhibiting some other design smell. I'd like to take that step before covering additional edge cases in the library's behavior.

Please don't read this as my being dismissive, I'm just trying to be really cautious and thoughtful about changes to the library's more well-established behavior.

[jasonkarns]

related:
sinonjs/sinon#583
sinonjs/sinon#571
sinonjs/sinon#665

[searls]

If the pain point here is in td.explain, I wonder if maybe we could at least save away the stringification of any set of args at the moment of configuration, invocation, and verification. The stringify-object module we improved for this lib's logging is no slouch, and it wouldn't lead to any kind of confusion about state

[searls]

Landed in 3.12.0 via #417