Support for Sinon - Githubissues

skovhus commented 7 years ago

Currently we warn that usage of Sinon might be incompatible with Jest. But it would be nice just to convert it to usage Jest Mocks and Spies.

The good news: Mocking and spying should be fairly easy to map to Jest.

But I think the following cannot really be converted to Jest:

useFakeTimers (and the tick functions doesn't seem compatible with Jest)
fakeServer
useFakeXMLHttpRequest

Example of a project using Sinon: https://github.com/WordPress/gutenberg/pull/1788#issuecomment-313732854

skovhus commented 7 years ago

@gziolo you did some manual transformation of Sinon in https://github.com/WordPress/gutenberg/pull/1788#issuecomment-313732854 ... Let me know if you have anything to add to the ticket above. You have more experience with Sinon than me. Thanks.

skovhus commented 7 years ago

Taking the examples from Sinon and trying to convert them to Jest by hand:


/* eslint-env jest */
import sinon from 'sinon';

const jQuery = {
    ajax: () => {},
};

function once(fn) {
    var returnValue, called = false;
    return function() {
        if (!called) {
            called = true;
            returnValue = fn.apply(this, arguments);
        }
        return returnValue;
    };
}

function getTodos(listId, callback) {
    jQuery.ajax({
        url: '/todo/' + listId + '/items',
        success: function(data) {
            // Node-style CPS: callback(err, data)
            callback(null, data);
        },
    });
}

function throttle(callback) {
    var timer;
    return function() {
        clearTimeout(timer);
        var args = [].slice.call(arguments);
        timer = setTimeout(function() {
            callback.apply(this, args);
        }, 100);
    };
}

describe('stubbing and mocking (sinon)', () => {
    it('calls original function with right this and args', function() {
        var callback = sinon.spy();
        var proxy = once(callback);
        var obj = {};

        expect(callback.called).toBe(false);

        proxy.call(obj, 1, 2, 3);

        expect(callback.called).toBe(true);
        expect(callback.calledOn(obj)).toBe(true);
        expect(callback.calledWith(1, 2, 3)).toBe(true);
    });

    it('returns the return value from the original function', function() {
        var callback = sinon.stub().returns(42);
        var proxy = once(callback);

        expect(proxy()).toBe(42);
    });

    afterAll(() => {
        jQuery.ajax.restore();
    });

    it('makes a GET request for todo items', function() {
        sinon.stub(jQuery, 'ajax');
        getTodos(42, sinon.spy());

        expect(jQuery.ajax.calledWithMatch({ url: '/todo/42/items' })).toBe(true);
    });
});

describe('stubbing and mocking (jest)', () => {
    it('calls original function with right this and args', function() {
        var callback = jest.fn();
        var proxy = once(callback);
        var obj = {};

        expect(callback).not.toHaveBeenCalled();

        proxy.call(obj, 1, 2, 3);

        expect(callback).toHaveBeenCalled();
        expect(callback.calledOn(obj)).toBe(true);
        expect(callback).toHaveBeenCalledWith(1, 2, 3);
    });

    it('returns the return value from the original function', function() {
        var callback = sinon.stub().returns(42);
        var proxy = once(callback);

        expect(proxy()).toBe(42);
    });

    afterAll(() => {
        jQuery.ajax.restore();
    });

    it('makes a GET request for todo items', function() {
        jQuery.ajax = jest.fn();
        getTodos(42, jest.fn());

        expect(jQuery.ajax).toHaveBeenCalledWith({ url: '/todo/42/items' });
    });
});

/* ------ THINGS I DON'T THINK WE CAN FIX ------ */

describe('fake xmlHttpRequest (sinon)', () => {
    let xhr;
    let requests;

    beforeEach(function() {
        xhr = sinon.useFakeXMLHttpRequest();
        requests = [];
        xhr.onCreate = function(req) {
            requests.push(req);
        };
    });

    afterAll(function() {
        // Like before we must clean up when tampering with globals.
        xhr.restore();
    });

    it('makes a GET request for todo items', function() {
        getTodos(42, sinon.spy());

        expect(requests.length).toBe(1);
        expect(requests[0].url).toMatch('/todo/42/items');
    });
});

describe('fake server (sinon)', () => {
    let server;
    beforeEach(function() {
        server = sinon.fakeServer.create();
    });
    afterAll(function() {
        server.restore();
    });

    it('calls callback with deserialized data', function() {
        var callback = sinon.spy();
        getTodos(42, callback);

        // This is part of the FakeXMLHttpRequest API
        server.requests[0].respond(
            200,
            { 'Content-Type': 'application/json' },
            JSON.stringify([{ id: 1, text: 'Provide examples', done: true }])
        );

        expect(callback.calledOnce).toBe(true);
    });
});

describe('time bending (sinon)', () => {
    let clock;

    beforeEach(function() {
        clock = sinon.useFakeTimers();
    });
    afterAll(function() {
        clock.restore();
    });

    it('calls callback after 100ms', () => {
        var callback = sinon.spy();
        var throttled = throttle(callback);

        throttled();

        clock.tick(99);
        expect(callback.notCalled).toBe(true);

        clock.tick(1);
        expect(callback.calledOnce).toBe(true);
        expect(new Date().getTime()).toEqual(100);
    });
});

gziolo commented 7 years ago

I'm not quite sure if that is a similar thing, but it might be worth checking if clock.tick(1); could be replaced with jest.runTimersToTime(1000);. See https://facebook.github.io/jest/docs/timer-mocks.html#run-timers-to-time. Jest has built-in fake timers support, but with a slightly different API.

jordalgo commented 7 years ago

The other fun conversion will be when you have situations like this

let myStub;
beforeEach(() => {
  myStub = sinon.stub(someDependency, 'method');
});

it('should do something', () => {
  expect(myStub.calledOnce).toBe(false);
});

@skovhus - Have you already started a branch to work on the sinon codemod ? I would be happy to contribute.

skovhus commented 7 years ago

@jordalgo I haven't started on it yet. Just did some manual transformation (see above) to figure out what we can do. : )

Let me know if you start on it. Thanks for your support!

We do have some proxyquire transformations already, so some of that code can be used for inspiration.

jordalgo commented 7 years ago

@skovhus Ok cool. I may do some tinkering later this week. I'll push something up if I make any progress.

jordalgo commented 7 years ago

@skovhus I got started on a branch. I'm running with the idea that the assertions have already been transformed to jest.expect (we can add a note that a test needs to be using jest before using the sinon transformer). Also, in this first iteration, I'm not confirming that objects which call methods like calledOnce, callCount, etc... are actually sinon stubs/spies/mocks. It can be done but it's just a lot more work ATM -- just betting that most people don't have separate objects that also have methods with those names 🤞 I also haven't started to scope all that needs to be done for the codemod; just sort of tinkering at the moment.

gziolo commented 7 years ago

I have another example for you. The following file was created using Mocha + Chai + Sinon + sinon-chai before the repository was migrated to Jest. So it gives a real life example what needs to be covered:

https://github.com/WordPress/gutenberg/blob/8de49b988df2363cb468bc7547124516ea893319/editor/block-switcher/test/index.js

Let me highlight the most important pieces:

import { expect } from 'chai';
import { spy } from 'sinon';
// ...
const onTransform = spy();
// ...
expect( onTransform ).to.have.been.calledOnce();
expect( onTransform ).to.have.been.calledWith( block, destinationName );

skovhus commented 7 years ago

Thanks @gziolo!

@jordalgo let me know if you need any help : )

jordalgo commented 7 years ago

@skovhus I've made a little progress but have stalled a bit as I'm sort of considering whether Jest's spy/stub feature set is mature enough for this code mod. There are just so many sinon features that can't be converted over to Jest right now e.g. return value capturing, spy.threw, stub.withArgs, pretty much all the async stub stuff, etc... Also, TBH, I'm not so thrilled (after having started this code mod) that Jest's assertion API is blended with the spy/stub API e.g.

expect(spy.calledWith(1, 2, 3)).toBe(true);

converts to:

expect(spy).toHaveBeenCalledWith(1, 2, 3);

I'll work on cleaning up my code this week and adding a few more conversions but just wanted your thoughts on this.

skovhus commented 7 years ago

I think you could suggest improving Jest based on your finding here.

If we cannot convert all those features, I agree that a codemod might not be the best idea. : )

jordalgo commented 7 years ago

@skovhus Yeah, I was thinking about making some Jest PRs/issues to add some of these features though IMHO the sinon API is a bit too large and not all the features need to be supported in Jest.

skovhus commented 7 years ago

@jordalgo they should also be able to co-exist, Sinon and Jest. : )

gziolo commented 7 years ago

Cross-commenting from https://github.com/Automattic/wp-calypso/pull/18695:

Please let me know which cases you needed to manual fix. Would be awesome if you could create an issue here https://github.com/skovhus/jest-codemods

@skovhus it was this file: https://github.com/Automattic/wp-calypso/blob/4a6ff2c0d6c509996fe054a4c341f372683b193a/client/components/token-field/test/index.jsx - this link is to the version before I updated it manually. There are 2 root causes:

import { test } from 'sinon'; which overrides test from Jest.

test from Sinon does some trick which exposes Sinon API inside the test using this. notation, e.g. this.clock.tick( 100 );

It's rather an edge case which is easy to fix manually, but I thought it's worth sharing anyway.

augbog commented 6 years ago

Hey so just wanted to follow up is the decision that a codemod for sinon -> jest may not be the best idea? Or is this still an ongoing discussion? It does sound like, from @jordalgo point, there are a few areas where a codemod really doesn't make sense (some tests that comes out might not be the actual test people want to write with jest even though it is transformed correctly).

Even if the codemod does a partial amount of migration with some warnings, that's better than nothing for a lot of people but wanted to continue the discussion as to what might be expected in a codemod? It might be worth adding to the README although the error when doing a codemod was pretty sufficient also.

AlanFoster commented 6 years ago

We're currently running with Karma and Sinon, and I successfully migrated to using Jest's mocks and expect modules without a code shift. I've documenting the manual steps I had to follow, as most of them could be used as a basis for code shifts.

Simple case

For the most part, I was able to naively replace sinon.mock and sinon.spy with jest.fn to get a lot of tests passing:

- this.onDateChangeMock = sinon.mock();
+ this.onDateChangeMock = jest.fn();

Args

The next issue was that Sinon/Jest handle recorded calls differently:

- var [{ startDate }] = this.onDateChangeMock.getCall(0).args;
+ var [{ startDate }] = this.onDateChangeMock.mock.calls[0];

- const newWindowUrl = openUrlStub.getCall(0).args[0];
+ const newWindowUrl = openUrlStub.mock.calls[0][0];

Sinon supports the use of firstCall/lastCall, whilst jest does not. This was only used a few times:

- expect(this.callback.firstCall.args[0]).toBe(this.expected);
+ expect(this.callback.mock.calls[0][0]).toBe(this.expected);

Mocked Returns

Jest's return API is different:

- this.onChecked = sinon.stub().returns(true);
+ this.onChecked = jest.fn().mockReturnValue(true);

This probably isn't worth an MVP code shift, but there was one or two times we used the following syntax for mocking different return values upon subsequent calls:

-    this.getFooState = sinon.stub();
-    this.getFooState.onFirstCall().returns('default-foo-state')
-      .onSecondCall().returns('updated-foo-state');
+    this.getFooState = jest.fn();
+    this.getFooState.mockReturnValueOnce('default-foo-state')
+      .mockReturnValueOnce('updated-foo-state');`

Resetting

Easy to code shift:

- mock.resetHistory();
+ mock.mockReset();

toHaveBeenCalledWith

Jest already supports toHaveBeenCalledWith, but the semantics are slightly different. In Jest I had to specify all params, not just the ones I cared about. I don't think it would be possible to code shift this, for the most part I had to use expect matchers instead:

- expect(this.callbackMock).toHaveBeenCalledWith(this.email);
+ expect(this.callbackMock).toHaveBeenCalledWith(this.email, expect.any(Function));

toHaveBeenCalledOnce / Twice / Thrice

Jest does not support toHaveBeenCalledOnce, however it is possible to use toHaveBeenCalledTimes(n) instead. This would be easy to shift shift:

- expect(mock).toHaveBeenCalledOnce();
+ expect(mock).toHaveBeenCalledTimes(1);

- expect(mock).toHaveBeenCalledTwice();
+ expect(mock).toHaveBeenCalledTimes(2);

To reduce the noise however, I went with a small expect shim instead:

const toHaveBeenCalledTimes = function (mock, times) {
  try {
    expect(mock).toHaveBeenCalledTimes(times);
    return { pass: true };
  } catch (e) {
    return { pass: false, message: () => e.message };
  }
};

expect.extend({
  toHaveBeenCalledOnce(mock) {
    return toHaveBeenCalledTimes(mock, 1);
  },
  toHaveBeenCalledTwice(mock) {
    return toHaveBeenCalledTimes(mock, 2);
  },
  toHaveBeenCalledThrice(mock) {
    return toHaveBeenCalledTimes(mock, 3);
  }
});

Spying Behaviour

Sinon supports nice mocking on existing objects via:

sinon.stub(myObject, 'myProp');

Jest has much richer mocking for this functionality, but that would require a much larger rewrite / complex code shift. I instead wrote a shim for this behaviour to make the transition to jest easier:

export const jestSpy = function (object, prop) {
  const oldValue = object[prop];
  const spy = jest.fn();
  spy.restore = function () {
    object[prop] = oldValue;
  };
  object[prop] = spy;

  return spy;
};

Then updated my code as:

-    sinon.spy(retry, 'operation');
+    jestSpy(retry, 'operation');

-    sinon.stub($, 'get').returns($.Deferred().resolve({}));
+    jestSpy($, 'get').mockReturnValue($.Deferred().resolve({}));

returnsArg

This was only used once, so it's probably not worth writing a code shift for, but:

- this.mock = sinon.stub().returnsArg(0);
+ this.mock = jest.fn().mockImplementation((firstArg) => firstArg);

toHaveBeenCalledBefore

There is currently no support for this functionality, although there is an initial effort with jest-extended. It was only used twice in our codebase with 5147 tests, so this wasn't much of a deal.

useFakeTimers

Jest supports useFakeTimers, however I am currently still using sinon's fake timers still - for now.

Hopefully this is useful information to help decide what sort of MVP code shift could be beneficial for developers :+1:

augbog commented 6 years ago

@AlanFoster that is awesome! I heavily recommend adding that to the documentation or, if anything, at least a link :)

jordalgo commented 6 years ago

@augbog @AlanFoster In my local testing of converting Sinon to Jest I ran into some strange edge cases and unsupported features that made me drop development on this code-mod. However, this is what I have done so far. I'll work on cleaning this up a bit because maybe some folks would find it useful.

skovhus commented 6 years ago

@jordalgo did you ever find more time to wrap your branch up? :)

Would love to support Sinon.

alundiak commented 6 years ago

So, having this line import sinon from 'sinon'; in spec files doesn't cause to be converted to any jest.fn() or jest.spyOn() If it's still relevant feature to support, I would like to have it.

skovhus commented 6 years ago

If it's still relevant feature to support, I would like to have it.

PRs are more than welcome.

jordalgo commented 6 years ago

@skovhus Unfortunately no :( But let me take a look at it this weekend. Are we still in the situation where there are MANY Sinon features that aren't supported in Jest? I probably just have to explicitly list all the ones we don't support and spit out warnings whenever we come across them.

SimenB commented 6 years ago

We wanna revamp our mocking API in Jest, so any suggestions that come out of this are very much welcome!

Some discussion here: https://github.com/facebook/jest/issues/5969

jordalgo commented 6 years ago

So here is the test file which shows all the Sinon Methods my PR supports: https://github.com/jordalgo/jest-codemods/blob/7de97c1d0370c7915cf5e5cc2a860bc5dd96744b/src/transformers/sinon.test.js

However! Here is a (not exhaustive) list of what this code shift still needs. Many methods do not have Jest equivalents. It seems pretty daunting but if we wanted to decide on a base set of methods to support then I could wrap up the work in my PR and we'd at least have something folks could use.

Spies:

withArgs
calledBefore*
calledAfter*
calledImmediatelyBefore*
calledImmediatelyAfter*
calledOn*
alwaysCalledOn*
calledOnceWith
alwaysCalledWith*
calledWithExactly*
calledOnceWithExactly*
alwaysCalledWithExactly*
alwaysCalledWithMatch*
calledWithNew*
neverCalledWith*
neverCalledWithMatch*
threw* (not quite the same as toThrow)
alwaysThrew*
returned
alwaysReturned*
thisValues*
returnValues (use 'results')
exceptions (use 'results')
resetHistory
restore
Stubs:
withArgs*
onCall*
onFirstCall*
onSecondCall*
onThirdCall*
resetBehavior (use 'mockReturnValue')
resetHistory (use 'mockReset')
callsFake
returnsThis
resolves
resolvesArg
throws
throwsArg
rejects
callsArg
callThrough*
callsArgOn
callsArgWith
callsArgOnWith
usingPromise*^
yields
yieldsRight
yieldsOn
yieldsTo
yieldsToOn
yield*
yieldTo*
callArg
callArgWith
addBehavior*
get*
set*
value*

* No direct Jest equivalent ^ Probably a method we don't need to duplicate

Mocks

It seems like all the mock methods are set up to be expectations of what will happen once code is executed but the Jest API checks after the fact e.g. 'toHaveBeen'. Probably would involve some fun codeshift trickery to switch Sinon Mocks to Jest.

Below are all the API groups that have no support yet for any method

Fake timers
Fake XHR and server
JSON-P
Matchers
Assertions
Fakes (Probably can take a lot from stub and spy shifts)

danbeam commented 2 years ago

@skovhus is this issue due an update now that there's a dedicated Sinon->Jest codemod in this repo? cc @jordalgo (thanks for the great start!) and @catc

skovhus / jest-codemods

Support for Sinon #68

Simple case

Args

Mocked Returns

Resetting

toHaveBeenCalledWith

toHaveBeenCalledOnce / Twice / Thrice

Spying Behaviour

returnsArg

toHaveBeenCalledBefore

useFakeTimers

Spies:

Stubs:

Mocks

Below are all the API groups that have no support yet for any method