Clone arguments before passing into the original function?

[b123400]

function fn(options) {
  options.foo = 1;
}
spy = chai.spy(fn);
spy({ bar : 1 });
spy.should.be.called.with({ bar : 1 }); // fail

This happens because the function fn modifies the argument, and the spy compares the modified object instead of the original version.
Do you think it makes sense if we store a shallow cloned argument instead of using the argument directly?

[keithamus]

Hey @b123400 thanks for the issue.

Firstly, currently, the way to do what you're hoping to do, is pass variables around:

function fn(options) {
  options.foo = 1;
}
spy = chai.spy(fn);
var arg1 = { bar : 1 };
spy(arg1);
spy.should.be.called.with(arg1); // passes fine

It is very important that .called.with works against references (like above) rather than deeply checking (like in your example). Take for example the following:

var p = Promise.resolve(1);
spy(p);
spy.should.be.called.with({}); // passes, but definitely should not.

Any class or instance that has no enumerable properties would resolve to any other class with no enumerable properties, or a plain object. This change would make spies much more difficult to work with than the current situation.

Having said all of that, maybe we could utilise chai's .deep flag:

function fn(options) {
  options.foo = 1;
}
spy = chai.spy(fn);
spy({ bar : 1 });
spy.should.be.deep.called.with({ bar: 1 }); // checks the arguments deeply, and so this passes

spy.should.be.called.with({ bar: 1 }); // fails

Thoughts?

[b123400]

um... I am not able to get it passed with the .deep flag.

I think it is not about how variables are compared, but which/when are variables being compared.
In the above example:

1. spy(fn) is called with { bar : 1 }
2. fn modifies the object to { bar : 1, foo : 1 }
3. spy compares { bar: 1, foo : 1 } with { bar : 1 }

and of course it fails.

Solution 1: Tell the spy the expected arguments before the function is being called, so it can compare the arguments before calling fn. Something like:

1. tell spy you expect { bar : 1 }
2. spy(fn) is called with { bar : 1 }
3. spy compares { bar : 1 } with { bar : 1 }, passed
4. call the original fn

So it will be something like:

spy.will.be.called.with({ bar : 1 })
spy({ bar : 1 })

Solution 2: Spy clones all the arguments passed in

1. spy(fn) is called with { bar : 1 }
2. spy make a copy of the arguments, let calls it argClone
3. fn modifies the object (not argClone) to { bar : 1, foo : 1 }
4. spy compares argClone with { bar : 1 }

Both of them are not perfect solutions tho.

[keithamus]

@b123400 hmm, actually you're right. All arguments are checked for deep equality. Apologies for the misinformation above.

IMO this is wrong, as I demonstrated above it becomes a problem because look-alike objects can bring up false positives. I think the problem you're experiencing is somewhat of a symptom of this.

If chai-spies only checked against references, rather than doing deep equality, this would not be an issue because you'd have to do my first code example. This is exactly how SinonJS works.

But I think I'm going to leave this one for @logicalparadox - as he I'm sure had specific design ideas when creating chai-spies, he is the best person to speak on the fundamentals of it.

[b123400]

@keithamus thanks for the quick reply.

It will involve comparing look-alike objects in solution 2, and that is a problem as well.

I am not expert in writing test, so please correct me if there is any mistake.
Your first code example requires the arg1 object to be shared between the function and the testing context, which is not the case for me. Consider this example:

function runFast(speed) {
  run({ speed: speed });
}
function run(options){
  if (!('direction' in options.direction)) {
    options.direction = "left";
  }
}
run = chai.spy(run)
runFast(10);
run.should.be.called.with({ speed: 10 }) // fail
run.should.be.called.with({ speed: 10, direction: "left" }) // pass

The actual argument passed to run in is created inside the runFast and I cannot access it from the testing context.

Maybe we can conclude this programming style is bad for testing, and I should not modify arguments directly?

[keithamus]

@b123400 if you were to ask me to review the code, outside of a test context, then yes, I'd say you're mutating variables which is, IMO, a bad practice; functions should be idempotent (unless they absolutely cannot) and so not permanently change the state they're given (rather - they should generate new state instead). If I were to personally write that code, I would do this:

function run(options){
  options = Object.assign({}, options); // if you dont have Object.assign, something like _.extend will work.
  if (!('direction' in options.direction)) {
    options.direction = "left";
  }
}

Writing code this way would also make the tests easier to write, as arguments won't be mutated from under you. However this is all personal opinion, and if you want to write it differently (e.g. the way you did), then that's fine.

With regard to your tests, you also might want to ask why you're testing run should be called from runFast. Is that something you actually care about? Testing, IMO, is about input and output. That runFast calls run is neither input, nor output - it is a side effect. I would test that runFast makes the player run fast, and run makes the character run - the desired output, given the input.

Ultimately though, how you've written code and tested it is not categorically wrong - and so chai spies should accommodate you somehow. I'm just not sure the best way for that.

[nickcarenza]

I would just like to add another use case to be considered:

function getAndSet() {
  var a = getSomething();
  if (conditional) set({prop:1});
  else set({prop:2});
}
getSomething = chai.spy.returns(1);
set = chai.spy();
getAndSet();
set.should.have.been.called.with({prop:1}) // fails

The argument to be asserted is created within the function being tested and so can't be referenced in the test.

[keithamus]

@nickcarenza this is just a case of ensuring you can set up a test environment which determines what conditional is. If you cannot influence what it is, then again - you could be relying to heavily on side effects.

[stalniy]

I'd like to add my 5 cents :)

1. chai.spy can't and shouldn't clone arguments before calls because in some cases users may pass complex arguments (e.g., instances of classes, or window object) which are hard to clone.
2. It's a bad practice to modify arguments by reference. There are special rules in eslint for that.
3. if you can't change source code of the function which you test then the only solution is to rely on that side effect and use variable:

function fn(options) {
  options.foo = 1;
}

const spy = chai.spy(fn);
const options = { bar: 1 };

spy(options);
spy.should.be.called.with(options);

The only action item which I think can be done in a backward compatible way is checking of args types. So, if we have this

var p = Promise.resolve(1);
spy(p);
spy.should.be.called.with({});

it will fail because p.constructor !== ({}).constructor. So, the check may look like this:

(p.constructor === arg.constructor || p instanceof arg.constructor) && deepEqual(p, arg)

@keithamus what your thoughts on this?

[keithamus]

It's been a while on this one, but I agree with your points. Personally I think we should wait a while for this one, but eventually do some breaking changes - so a course of action is like this:

1. called.with should work by reference using strict equality - currently this is not true, this would be a breaking change.
2. deep.called.with should modify this to use deep.equal which is what today's behavior is.
3. For other edge caes, chai 5 will introduce matchers, which will solve this. If we add matcher support to chai-spies, then in you'd be able to do stuff like: expect(spy).to.be.called.with(expect.a('promise')).

I think this gives us full flexibility without compromising on quality. I'm happy to make breaking changes if it means good progress.

[eKoopmans]

For anyone else facing a similar problem, I have found a workaround using Sinon (would have used chai-spies if it had any way to check a spy's return value).

The issue is that the input argument is getting mutated after the spy call but before chai-spies (or Sinon) checks it against the desired value. We want to make a deep-copy of the argument that we can later compare against.

I used a Sinon stub and modified it to return a deep copy of the input argument using a fakeFunction:

sinon.stub(myObj, 'method').callsFake(function fakeFn(arg) {
    // My argument was an array that was later being mutated.
    if (arg.constructor === Array) {
        return arg.slice(0);
    } else {
        return arg;
    }
});

Then instead of checking what the function was called with, check what it returns:

// Using Sinon and sinon-chai:
expect(myObj.method).to.have.returned(myArray);

(It would be great to have some option that performed the deep copy on the spy arguments directly, but this works for me for now!)