Test unexpectedly failing after v7.2.0 release

[archcorsair]

Describe the bug
Upgrading Sinon to latest v7 release causes some unexpected failures. The only relevant change I was able to identify was this PR: #1955 - Replacing deep-equal.js with samsam.deepEqual.
The tests pass on all versions up until 7.2.0, reverting to 7.1.1 (one release back) does not cause any issues and all tests pass.

Error Observed:

AssertionError: expected addTracks to have been called with arguments [{ language: "en", trackContentType: "CEA608" }]
[[functionStub] { language: "en", trackContentType: "CEA608" }] [{ language: "en", trackContentType: "CEA608" }]

Test Producing Error:

    it('should call getTextTracksManager, Track, and setCaptionLang', () => {
      sinonSandbox.stub(chromecastReceiver, 'setCaptionLang');
      chromecastReceiver.player.getTextTracksManager = sinonSandbox.stub()
        .returns({ addTracks: sinonSandbox.spy() });
      sinonSandbox.stub(cast.framework.messages, 'Track').returns(() => ({}));

      chromecastReceiver.loadCaptions('off');

      expect(chromecastReceiver.player.getTextTracksManager).to.have.callCount(1);
      expect(cast.framework.messages.Track).to.have.been
        .calledWith(3, cast.framework.messages.TrackType.TEXT);
      expect(chromecastReceiver.textTracksManager.addTracks).to.have.been.calledWith([{
        trackContentType: cast.framework.messages.CaptionMimeType.CEA608,
        language: 'en',
      }]);
      expect(chromecastReceiver.setCaptionLang).to.have.been.calledWith('off');
    });

To Reproduce
Steps to reproduce the behavior:

1. Use any Sinon version above 7.1.1
2. Run sample test (above)
3. See error

Expected behavior
Test should pass

Screenshots
If applicable, add screenshots to help explain your problem.

Context (please complete the following information):

• Library version: 7.2.0+
• Environment: macOS 10.14.5

Please let me know if I can supply more information about this.

[fatso83]

Thank you for a generally great bug report. It has one major deficit, though: I cannot run your example for verification or tracking down the bug! It's not runnable on its own, and without a way of reproducing, it's hard to fix this.

Could you make a minimal candidate for reproduction? You don't have to replicate the entire original example, more like a simplification like var myObject = { aMethod: function(){}, aProp: 42 }; // etc.

If we had known this was breaking, we would of course have upped the major version, so a regression test would be great!

P.S. Seems like sinon-chai is also involved. That's worth listing.

[archcorsair]

@fatso83 Let me see how I can get an isolated, runnable example for reproduction

[fatso83]

Any updates?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[fatso83]

@mantoni Didn't you report experiencing something like this happening in one of your projects?

[archcorsair]

Sorry for the lack of updates, I am no longer associated with the project that contained the issue. If anyone else can is experiencing the same issue, it would be great if they could provide an example for repro

[mantoni]

@fatso83 I don‘t recall running into the same issue. I have upgraded to latest Sinon in a few projects without any issues.

[fatso83]

Closing as cannot reproduce

[dgollub]

Sorry for reviving this issue from the dead, but I'm running into the same problem.

The code below is simplified for illustration purposes.

In my application I have this class

class MyUser {
  // ... some user properties  
}

and I use it later in a Koa controller function like this

const mqClient = require('./mqClient');

// GET /userDetails/<id> endpoint
async function getUserDetailsController(req, res) {
  const user = new MyUser();

  // populate user with properties etc.
  user.id = '1';
  user.name = 'Hans Gruber';
  user.tasks = [{ id: '42', title: 'execute Nakatomi tower heist' }];

  // ... get more details ...

  // Notify other users about this user
  mqClient.notifyOthers(user);

  res.json({ ok: true, user });
}

And in my getUserDetailsController.test.js I do something like this:

const mqClient = require('./mqClient');

const createExpectedUser = overrides => {
  id: '1',
  name: 'Hans Gruber',
  tasks: [{ id: '42', title: 'execute Nakatomi tower heist' }],
  ...overrides
};

it('does not fail', async () => {
  sinon.spy(mqClient, 'notifyOthers');

  const expectedUser = createExpectedUser();

  await presetRequest // helper object to run the controller code
    .get('/userDetails/1')
    .expect(200);

  // This works with sinon 7.1.1.
  // This fails with sinon 7.2.0 and onwards.
  sinon.assert.calledWithMatch(mqClient.notifyOthers, expectedUser);
});

I tested this with sinon 7.1.1 (which uses "2.1.3" of samsam), and that works.
Version 7.2.0 fails (uses 3.3.3 of samsam).
Most recent version 9.2.3 fails as well (uses 5.3.0 of samsam).

I'm on Node 14.5.3, but this also happens on Node 12.20.0.

I compared the user in the controller and the expectedUser in the test via JSON.stringify manually and they are 100% equal. The only difference syntactically is that the controller uses the class, while the test uses an object literal.

Does anyone have a good idea why this happens? Or has anybody a good idea on how to work around this?

Kind regards,
Daniel

[fatso83]

As long as we have no reproducible case to verify this, we cannot justify spending our free hours looking into this, I am afraid. I would have nothing to look for and nothing to verify that I was right.

I do love fixing bugs, though, so if you can spend some time in trying to make something I can run, do so! RunKit is a great service for doing so: https://runkit.com/fatso83/sinon-issue-reproducible-bug-template

[dgollub]

Hi @fatso83,

Thanks for taking your time to look into it. I did as you asked me to and created a runkit demo that shows the bug. Just comment/uncomment the different require('sinon') statements and run the code to see the bug.

https://runkit.com/danielkg/sinon-issue-reproducible-bug-template

I post the code here as well in case runkit doesn't work for whatever reason.

// Employs 'mini-mocha' to emulate running in the Mocha test runner (mochajs.org)
require("@fatso83/mini-mocha").install();
// const sinon = require('sinon@7.1.1'); // WORKS!
const sinon = require('sinon@7.2.0'); // FAILS!
// const sinon = require('sinon');       // FAILS!
const { assert } = require('@sinonjs/referee');

const SAMPLE_USER = {
    id: 1,
    name: 'Kid',
    age: 22,
    weight: 83.5,
    notes: [{ txt: 'abc' }, { txt: 'def' }, { txt: 'ghi' }],
};

class MyUser {
    constructor(id, name, age, weight, notes = []) {
        this.id = id;
        this.name = name;
        this.age = age;
        this.weight = weight;
        this.notes = notes;
    }
}

const createFakeUser = overrides => ({
    id: '007',
    name: 'Bond',
    ...overrides,
});

class Talkie {
    talk(user, topic) {
        console.log('talked to', user.name, 'about', topic);
    }
}

const action = (userId, talkie) => {
    const { name, age, weight, notes } = SAMPLE_USER;
    const user = new MyUser(userId, name, age, weight, notes);

    talkie.talk(user, 'apples');

    return user;
}


describe('stubbing', () => {
    const talkie = new Talkie();

    afterEach(() => {
        sinon.restore();
    });

    beforeEach(() => {
        sinon.spy(talkie, 'talk');
    });

    it('is the same user', () => {
        const userA = action(1, talkie);
        const userB = createFakeUser(SAMPLE_USER);

        assert.equals(JSON.stringify(userA), JSON.stringify(userB));
    });

    it('should set the return value', () => {
        const userA = action(1, talkie);
        const userB = createFakeUser(SAMPLE_USER);

        sinon.assert.calledWith(talkie.talk, userB, sinon.match.string);
    });
});

Please have another look when you have the time. Thank you.

Kind regards,
Daniel

[fatso83]

This was a hard one. Not totally sure what is going on, but it obviously do not think the two objects are the same.

The breaking changes in Samsam v3 are summarised here, I believe: sinonjs/samsam@07832fb

A good handful of potential suspects.

[fatso83]

I'll make a quick pass at this with a debugger ( mocha inspect test.js)

[fatso83]

OK, so it's narrowed down to a change in how deepEqual from @sinonjs/samsam compares the objects:

Maybe it looks at the prototype or something.

[fatso83]

OK, got it! As the console indicated, the change has to do with the prototype, or rather the class name of the object. So "MyUser" is compared to "Object":

I think the current match is as expected, even if it broke your tests, and that you should actually employ some kind of matcher that compares on fields, not deep equality. Need to split, but at least we know exactly what the change was :)