Jest: mock.calls[][] holds reference instead of actual value when function called twice with object

[sbugert]

Consider the following test:

jest.dontMock('../unmocked');

describe('foo', function(){
  var unmocked = require('../unmocked');
  var mocked = require('../mocked');

  it('bar', function(){
    unmocked.func();
    var firstCall = mocked.func.mock.calls[0][0];
    var secondCall = mocked.func.mock.calls[1][0];

    expect(firstCall).toEqual({ content: 'first' });
    expect(secondCall).toEqual({ content: 'second' });

  });

});

With some arbitrary function being mocked in the test:

module.exports.func = function() {};

And a function which will be tested:

var mocked = require('./mocked');

module.exports.func = function() {
  var obj = { content: 'first' };
  mocked.func(obj);
  obj.content = 'second';
  mocked.func(obj);
};

The tested function calls the mocked function with an object, mutates the object and calls the mocked function a second time.
mocked.func.mock.calls[0][0] and mocked.func.mock.calls[1][0] will now hold the same reference to obj and the above test fails.
I'm not sure if that's intended.
If it's not, shouldn't jest make a deep copy of the argument if it's an object?

I'm using facebook/jest#0.5.x.

[browniefed]

This is because you are only doing the require once.
Move your requires into a beforeEach

[sbugert]

That does not solve the problem because I want to test if the function has been called with an object and then with the same object mutated _in the same test_. So the beforeEach will run only once.

jest.dontMock('../unmocked');

describe('getSearchResults', function(){
  var unmocked;
  var mocked;

  beforeEach(function() {
    unmocked = require('../unmocked');
    mocked = require('../mocked');
  });

  it('mock.calls should deep copy objects', function(){
    unmocked.func();
    var firstCall = mocked.func.mock.calls[0][0];
    var secondCall = mocked.func.mock.calls[1][0];

    expect(firstCall).toEqual({ content: 'first' });
    expect(secondCall).toEqual({ content: 'second' });

  });

});

[browniefed]

@sbugert apologies misunderstood, I believe you just stumbled upon issue #429 or something similar.

[cpojer]

Cloning here would be too expensive for performance. I understand this is an issue but the side-effect in your function is probably not something you'd want in your application anyway, I'll assume? While I do not know what your application is doing, shallow copying the object in your function Object.assign({}, obj) is probably reasonable.

[karolis-sh]

I'm facing the same issue. I'm constructing a nested object, validating it midway in the process, and use some final formatting in the end (date -> string). I'm mocking the validator, and have to do deep copy the object being passed to it.

It would be nice that fn.mock.calls would deep copy the values itself instead of me doing it in my code.

[Towerful]

I've just stumbled onto this as being an issue, using v21.2.1
Does anyone have a fix or a work around?

I build an object with 'old data', call a function to act on it, change 1 property, and call a function to act on it.
It seems a little ridiculous that I need to build up separate objects (or Object.assign a new one) for the sake of my test suite.

I feel like a mocked function should deep copy the parameters passed in.

[snooze92]

Stumbled upon the same thing here. Any chance this gets revisited / reopened? Right now our production code is doing unnecessary copies, just so the test framework works as expected.

[et]

Cloning here would be too expensive for performance. I understand this is an issue but the side-effect in your function is probably not something you'd want in your application anyway, I'll assume? While I do not know what your application is doing, shallow copying the object in your function Object.assign({}, obj) is probably reasonable.

@cpojer - Why would cloning be too expensive? Can't there be some kind of reasonable setting that only does it if we are asking about multiple calls? Or can we just tell jest somehow via a setting: "in this situation, I want you to clone".

As to your second question about why someone would do this without copying the object. I use typeorm and I have my code running in a loop to update a user's status from somewhere else -- this is from a 3rd party so I keep asking the 3rd party system what user's status is and save the result in my own db for caching purposes.

The code more or less looks like this:

async updateUserStatus(user) {
  const maxTries = 5;
  let tryNum = 1;

  while (tryNum <= maxTries) {
    await sleep(sleepTimeout);
    const resp = await client.getStatus(user.id);
    user.status = resp.status;

    await this.userRespository.save(user);

    if (user.status === 'success') {
      return Promise.resolve();
    }
  }
  this.log.info('Failed to get user\'s status in a sucess state');
}

I ran into this and spent some time trying to debug the situation.

I haven't used sinon in a long time but I believe they got this behavior correct. There is definitely a balance between performance and doing what the user intended. Right now, checking multiple mock calls in jest seems confusing.

I totally get the perf issues but surely there's a reasonable middle ground.

[SimenB]

I wonder if it makes sense to add the ability to opt in to cloning

[elialgranti]

I want to add my vote for an option to clone between calls.
In my case I am calling the S3 client to retrieve a list of files. S3 returns a maximum of 1000 files so it is called in a loop with the same requests parameters except for the continuation token. There is currently no way to test the continuation token is correctly copied from the result of the previous call to the next.

Deep cloning may be expensive, but it is better to have slightly slower tests than to have needless copies in production or code that cannot be tested.
Thanks!

[SimenB]

I like my own idea above, and would love to see a PR.

jest.fn().cloneArgumentsAndReturnValues() is a way too long name, but you get the idea 🙂
Code lives here: https://github.com/facebook/jest/blob/master/packages/jest-mock/src/index.js

[joshribakoff]

I understand this is an issue but the side-effect in your function is probably not something you'd want in your application anyway, I'll assume?

Lets say you agree with this statement, with the current implementation of jest there is no way to write a test that verifies you don't do this.

Can't there be some kind of reasonable setting that only does it if we are asking about multiple calls?

That does not help if you have a single call with the wrong value, then after that the value gets mutated to the correct value. The test will pass, even though the mock was called with the wrong value.

[yatki]

We recently encountered the same problem. I think instead of cloning the object in my source code, I'd prefer to waste some extra memory in the test runner.

@SimenB So having some methods like startArgumentRecording, stopArgumentRecording to opt-in argument recording sounds good to me. (names are not that good but to give you an idea)

One can even call jest.startArgumentRecording() in test setup file if they want to record everything...

So, @cpojer would you consider to add this functionality. Or is there already a solution for that?

[BreakBB]

I stumpled across this as well today and nearly lost my mind, before I found out it really is an issue of jest itself.

I do understand the argument about performance, even though it is a clear side-effect caused by jest and not a real solution to add unnecessary Object.assigns to production code for tests.

The idea of @SimenB sounds like a solution which could fulfill performance requirements where object copies are not required.