-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for Sinon #68
Comments
@gziolo you did some manual transformation of Sinon in WordPress/gutenberg#1788 (comment) ... Let me know if you have anything to add to the ticket above. You have more experience with Sinon than me. Thanks. |
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);
});
}); |
I'm not quite sure if that is a similar thing, but it might be worth checking if |
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. |
@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. |
@skovhus Ok cool. I may do some tinkering later this week. I'll push something up if I make any progress. |
@skovhus I got started on a branch. I'm running with the idea that the assertions have already been transformed to |
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: 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 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,
converts to:
I'll work on cleaning up my code this week and adding a few more conversions but just wanted your thoughts on this. |
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. : ) |
@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. |
@jordalgo they should also be able to co-exist, Sinon and Jest. : ) |
Cross-commenting from Automattic/wp-calypso#18695:
It's rather an edge case which is easy to fix manually, but I thought it's worth sharing anyway. |
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. |
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 caseFor the most part, I was able to naively replace - this.onDateChangeMock = sinon.mock();
+ this.onDateChangeMock = jest.fn(); ArgsThe 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 - expect(this.callback.firstCall.args[0]).toBe(this.expected);
+ expect(this.callback.mock.calls[0][0]).toBe(this.expected); Mocked ReturnsJest'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');` ResettingEasy to code shift: - mock.resetHistory();
+ mock.mockReset(); toHaveBeenCalledWithJest already supports - expect(this.callbackMock).toHaveBeenCalledWith(this.email);
+ expect(this.callbackMock).toHaveBeenCalledWith(this.email, expect.any(Function)); toHaveBeenCalledOnce / Twice / ThriceJest does not support - 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 BehaviourSinon 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({})); returnsArgThis 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); toHaveBeenCalledBeforeThere 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. useFakeTimersJest supports Hopefully this is useful information to help decide what sort of MVP code shift could be beneficial for developers 👍 |
@AlanFoster that is awesome! I heavily recommend adding that to the documentation or, if anything, at least a link :) |
@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. |
@jordalgo did you ever find more time to wrap your branch up? :) Would love to support Sinon. |
So, having this line |
PRs are more than welcome. |
@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. |
We wanna revamp our mocking API in Jest, so any suggestions that come out of this are very much welcome! Some discussion here: jestjs/jest#5969 |
So here is the test file which shows all the Sinon Methods my PR supports: 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:
Stubs:
* No direct Jest equivalent MocksIt 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
|
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:
tick
functions doesn't seem compatible with Jest)Example of a project using Sinon: WordPress/gutenberg#1788 (comment)
The text was updated successfully, but these errors were encountered: