Skip to content
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

RFC: Stubs & Spies #1825

Closed
jamiebuilds opened this issue Jun 1, 2018 · 13 comments
Closed

RFC: Stubs & Spies #1825

jamiebuilds opened this issue Jun 1, 2018 · 13 comments
Labels

Comments

@jamiebuilds
Copy link
Contributor

jamiebuilds commented Jun 1, 2018

I wanted to quickly write this up because I think that AVA could provide a superior/simpler developer experience for stubbing within tests than tools like Sinon.

Instead of writing a detailed proposal, I'm going to give some API examples and we can refine them from there. I will also create a library that implements as much of this as possible.

Why add stubs/spies

  • Because it's a very common need for testing larger apps
  • Because existing tools are very complex and have massive API surface
  • Because AVA can provide a better experience by integrating them into the testing framework
    • Through better error messages
    • Through automatic cleanup
    • Through smaller APIs

Why not a separate AVA-specific library

  • Because error messages could be greatly improved in AVA by recognizing assertions on stub/spy calls

Real World Examples

const test = require('ava');
const EventEmitter = require('events');

test('EventEmitter', t => {
  let e = new EventEmitter();
  let s = t.stub();
  e.on('event', s);

  e.emit('event');
  t.is(s.calls.length, 1);

  e.emit('event', 'arg');
  t.is(s.calls[1].arguments[0], 'arg');
});
const test = require('ava');
const api = require('./api');

test('api.getCurrentUser()', t => {
  let s = t.spy(api, 'request', () => {
    return Promise.resolve({ id: 42 });
  });

  await api.getCurrentUser();
  
  t.deepEqual(s.calls[0].args[0], {
    method: 'GET',
    url: '/api/v1/user',
  });
});

Detailed API Examples

test('t.stub()', t => {

  // create stub with optional inner function which can be used
  // to customize behavior. Otherwise will default to: `() => {}`

  let s = t.stub(arg => {

    // use s.calls inside optional inner func instead of complex
    // `s.onCall(0).assertArgs('s1').returns('r1');`

    if (s.calls.length === 0) { t.is(arg, 'a1'); return 'r1'; }
    if (s.calls.length === 1) { t.is(arg, 'a2'); return 'r2'; }
    if (s.calls.length === 2) { t.is(arg, 'a3'); return 'r3'; }

    throw new Error('too many calls');
  });

  // use as regular function

  s.call('t1', 'a1');
  s.call('t2', 'a2');
  s.call('t3', 'a3');

  // assert using existing AVA methods, AVA's error reporting is
  // already better than the custom stub assertion methods
  // different testing libs create.

  // AVA could also improve error messages by detecting that
  // we're writing assertions against a stub/spy.

  t.deepEqual(s.calls, [
    { this: 't1', arguments: ['a1'], return: 'r1' },
    { this: 't2', arguments: ['a2'], return: 'r2' },
    { this: 't3', arguments: ['a3'], return: 'r3' },
  ]);

  t.throws(() => {
    s.call('t4', 'a4'); // Error: too many calls
  });
});

test('t.spy()', t => {

  // create spy with optional inner function which can be used
  // to customize behavior and override the original function.
  // Otherwise will default to calling the original function.

  // object['method'] must be a defined function or t.spy() will
  // throw

  let s = t.spy(object, 'method', (...args) => {

    // use s.calls to customize this function the same way you
    // would with t.stub()

    // use s.original to access the original function if you want to
    // call it.

    return s.original(...args);
  });

  // use as regular function and write assertions the same way you
  // do with t.stub()

  // If you want to restore the original function you can simply do:
  object.method = s.original;

  // otherwise AVA will automatically restore the original function
  // at the end of a test run.
});

API type signature

// generic function (`this` refers to function's call context `this`, not a first arg)
type Func<T, A, R> = (this: T, ...arguments: A): R;

// t.spy/stub().calls array
type Calls<T, A, R> = Array<{
  this: T,
  arguments: A,
  returns: R,
}>;

// callable functions with properties
type Stub<T, A, R>   = Func<T, A, R> & { calls: Calls<T, A, R> };
type Spy<F, T, A, R> = Func<T, A, R> & { calls: Calls<T, A, R>, original: F };

// test('...', t => { t.stub/spy() });
interface t {
  stub<T, A, R>(inner?: Func<T, A, R>): Stub<T, A, R>;
  spy<F, T, A, R>(inner?: Func<T, A, R>): Spy<F, T, A, R>;
}
@jamiebuilds
Copy link
Contributor Author

I've published a library that implements (most of) this API here: https://github.com/jamiebuilds/ninos

@novemberborn
Copy link
Member

Hey this looks nice!

Ava can provide a better experience by integrating them into the testing framework

  • Through better error messages
  • Through automatic cleanup
  • Through smaller APIs

How do you see cleanup working? In my experience the spy / stub setup is impacted by test concurrency, and therefore so is cleanup. Could you elaborate?

Would we be able to return better error messages without adding assertions? Note that we're looking at adding t.assert() and removing power-assert from others (#1808). Does this become a question of improving output for t.assert() for recognized spies / stubs?

@stephenmathieson
Copy link

Just my 2 cents - my only issue with AVA+Sinon has been difficulty with concurrency. I agree that Sinon's API surface is pretty big, but their docs are sufficient (IMHO), and most folks have used it before. I find that I either have to run my tests in serial (which isn't great), or I have to use some form of dependency injection. I (almost) always go with the injection method, but in my experience, this often requires a pretty heavy refactoring of the app/service/thing I'm testing.

If the outcome of this thread resolves that issue, it'd make AVA much easier to write tests using mocks.

@jamiebuilds
Copy link
Contributor Author

How do you see cleanup working? In my experience the spy / stub setup is impacted by test concurrency, and therefore so is cleanup. Could you elaborate?

My library doesn't handle this well enough, but because t.spy() is only available within test() and friends, you can track the spies created as part of an individual test even while others are running.

This would also allow you to detect a few things:

  • Tests trying to spy on the same exact object at the same time
  • Tests calling spies/stubs that were created by other tests
  • Tests calling stubs after the test finished.

I think we'd have to play around a bit with what warnings/errors are most useful and when, but we can do a lot more by hooking into internals.

People will still likely have some amount of trouble with singletons, but I'm not sure that's ever going to be completely solved in JavaScript unless tests were completely isolated.

@jamiebuilds
Copy link
Contributor Author

Would we be able to return better error messages without adding assertions? Note that we're looking at adding t.assert() and removing power-assert from others (#1808). Does this become a question of improving output for t.assert() for recognized spies / stubs?

I have to think about that more. I wasn't aware you were thinking about removing power assert from others. Maybe you could replace it with something lighter weight in other scenarios?

Either way I think the experience is still pretty good without it being specialized (based on the errors I was getting with Niños).

@novemberborn
Copy link
Member

I'd like to explore what we'd need to support integration of libraries like Niños with AVA. We have an open issue to allow customizable assertions (#1094), which could be used to add t.spy() to the execution context. Perhaps we can have those hook into the test lifecycle as well for cleanup purposes.

@jamiebuilds
Copy link
Contributor Author

All it would take to make this work as a separate library is to have a t extension point that can be aware of the current test context. You can actually get close by using t.context

function ninos(test) {
  test.beforeEach(t => {
    t.context.spies = [];
    t.context.spy = () => {
      // ...
      t.context.spies.push({ object, method, original });
    };
  });

  test.afterEach(t => {
    t.context.spies.forEach(({ object, method, original }) => {
      object[method] = original;
    });
  });
}
const test = require('ava');
require('ninos')(test);

test('spy', t => {
  t.context.spy(object, 'method'); // reset automatically
});

This can be improved a lot with some sort of ExecutionContext.extend() method. However, I don't see a way that you can make it work with Flow/TypeScript types.

@jamiebuilds
Copy link
Contributor Author

Separately, I would argue that this sort of thing should be built into the test framework itself for a nicer out of the box experience.

I think that AVA should definitely have a ExecutionContext.extend() method, but I would reserve that for things like framework specific assertion methods. Installing a separate ava-react library makes sense, but stubbing/spying seems like it should be built-in.

I'm going to propose an API for extending the test execution context which can handle test concurrency. But I suggest we continue exploring adding stubs/spies to AVA.

@novemberborn
Copy link
Member

All it would take to make this work as a separate library is to have a t extension point that can be aware of the current test context.

If I'm reading this correctly this could be handled by my test.use() proposal discussed in #1827 (comment).

Separately, I would argue that this sort of thing should be built into the test framework itself for a nicer out of the box experience.

Currently I'd rather not add such features. We have plenty other challenges to tackle.

@jamiebuilds
Copy link
Contributor Author

We have plenty other challenges to tackle.

Is the reason just that you're too busy? I would be happy to implement this

@novemberborn
Copy link
Member

I don't think AVA's core should say how to do stubs & mocks. I'd rather support bindings for the various libraries out there. Happy to host them in the GitHub org and publish under our npm scope.

We do have to make that setup as friction free as possible.

@sindresorhus
Copy link
Member

I for one would love to have a simple stubbing solution built-in. I find libraries like Sinon, while great, a bit too much and hard to use. The downside is that it's difficult to maintain a good stubbing library. There are lots of edge-cases to handle and I don't think we have the resources to handle that. We definitely should improve the extensibility of AVA though, making it super easy to integrate an external stubbing library.

@gtarsia
Copy link

gtarsia commented Oct 7, 2019

since I loved ninos api and this went nowhere, I created dummee, which is basically ninos but standalone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants