mjackson / expect

Write better assertions
MIT License
2.29k stars 117 forks source link

spy.andCall(), spy.andReturn(), spy.andThrow() should nullify other responses #138

Closed awei01 closed 8 years ago

awei01 commented 8 years ago

Currently, spy.reset() just resets the calls. Perhaps it should also clear out targetFn, thrownValue and returnValue?

Right now once the spy has a andCall() set, other tests using the spy are bound to the same andCall() even if .andReturn() or andThrow() are subsequently called. In order to workaround, I have to explicitly call andCall(null) for any of the latter to make the spy return or throw the appropriate result.

Or, the other possibility is to ensure that andCall(), andReturn() and andThrow() clear out the other targetFn, thrownValue and returnValue variables.

I can submit a PR, but wanted to get feedback first.

awei01 commented 8 years ago

After thinking about this a bit, spy.reset() should not do anything to the andCall, andReturn or andThrow values. Developer might want to set up something and let it behave the same way for all tests.

However, the and* functions should nullify the other variables.

Changed title to reflect this.

mjackson commented 8 years ago

Spies are not intended to be long-lived things. I forgot why we introduced the spy.reset API in the first place, but I never use it. It's super cheap to just make a new spy whenever you need to do some spying and then throw it away when you're done. If anything, I'd say let's remove the spy.reset API and just warn if you call two different and* methods, but I don't think it's critical.

I'd welcome a PR if you want to take this on.

awei01 commented 8 years ago

Please don't remove spy.reset. While I understand that they're cheap and easy to setup/tear down, I also find them quite useful for setting up a test suite once in a before statement. It helps keep things DRY in some cases. Pseudo code example:

  var Foo = require('./Foo');
  describe('some suite testing Foo implementation of BarDependency', () => {
    var BarDependency;
    before(() => {
      BarDependency = { 
        useBar: createSpy()
      };
      Foo.setBarDependency(BarDependency);
    });
    afterEach(() => {
      BarDependency.useBar.reset();
    });
    it('Foo.doSomething() called with [param] calls BarDependency.useBar() with [param], () => {
      Foo.doSomething('param');
      expect(BarDependency.useBar).toHaveBeenCalledWith('param');
    });
    it('Foo.doSomething() called with [null] never calls BarDependency.useBar(), () => {
      Foo.doSomething(null);
      expect(BarDependency.useBar).toNotHaveBeenCalled()
    });
  })

In the above case, I can keep my test DRY by not having to repeatedly set up Foo.setBarDependency.

I'll create a PR for the other issue.

awei01 commented 8 years ago

Created PR here: https://github.com/mjackson/expect/pull/143