refactor(spy): removes reset method

[stalniy]

Spy should remain immutable. Instead of reseting it, it's better to create a new one

[alexmnv]

I miss reset() feature.
What is a convenient way of injection a new spy into a private function (that is not accessible via object property)?
For example:

let foo = FooFactory.create(spy)

foo.someFunc()
expect(spy).to.have.been.called.twice()
expect(spy).to.have.been.called.with('bar')
expect(spy).to.have.been.called.with('baz')

spy.reset() // doesn't work anymore

foo.someFunc2()
expect(spy).to.have.been.called.once()
expect(spy).to.have.been.called.with('bar')

[stalniy]

Just create a new instance of foo. Also I'd recommend to split your test into 2 using beforeEach
Solution #1:

let spy = chai.spy()
let foo = FooFactory.create(spy)

foo.someFunc()
expect(spy).to.have.been.called.twice()
expect(spy).to.have.been.called.with('bar')
expect(spy).to.have.been.called.with('baz')

spy = chai.spy()
foo = FooFactory.create(spy)


foo.someFunc2()
expect(spy).to.have.been.called.once()
expect(spy).to.have.been.called.with('bar')

Better way:

describe('....', function(){
   let foo, spy

  beforeEach(()=>{
     spy = chai.spy()
     foo = FooFactory.create(spy)
  })

  It('....', () => {
    foo.someFunc()

    expect(spy).to.have.been.called.twice()
    expect(spy).to.have.been.called.with('bar')
    expect(spy).to.have.been.called.with('baz')
  })

  it('....', () =>{
    foo.someFunc2()

    expect(spy).to.have.been.called.once()
    expect(spy).to.have.been.called.with('bar')
  })
})

[alexmnv]

Thanks for the response. But the thing is that someFunc and someFunc2 must be called on the same instance sequentially in order to test the behavior.
I use this approach (which seems less elegant than using spy.reset())

let spy
let spyWrapper = function(arg) {
	if (spy) spy(arg)
}

let foo = FooFactory.create(spyWrapper)

spy = chai.spy()
foo.someFunc()
expect(spy).to.have.been.called.twice()
expect(spy).to.have.been.called.with('bar')
expect(spy).to.have.been.called.with('baz')


spy = chai.spy()
foo.someFunc2()
expect(spy).to.have.been.called.once()
expect(spy).to.have.been.called.with('bar')

[stalniy]

So, probably this feature - #75 may be more useful for you. As you will be able to check expect(spy).to.have.been.third.called.with('bar')

[shellscape]

This is a pretty lowsy regression. reset was an incredibly elegant convenience method that allowed:

    before(() => {
      chai.spy.on(ga, 'trackEvent');
    });

    beforeEach(() => {
      ga.trackEvent.reset();
    });

Additionally to.have.been.third.called is neither proper English, nor is it as elegant as sinons calledThrice.

Poor choice, and it should have had more community feedback before merging (as it was created and merged on the same day). I'll be reverting that change in chai-spies-next.

[stalniy]

reset mutates spy internals. The goal was to make spy to be immutable. You can use new sandbox feature to achieve what you want (just re-spy this method for each test).

Update: please check #38 for details

Update 2: you need to change your code to this:

    beforeEach(() => {
      chai.spy.on(ga, 'trackEvent');
    });

    afterEach(() => {
      chai.spy.restore(ga, 'trackEvent');
    });

This is the more propere way to do it.

[shellscape]

I understand that, and it's still a regression. Spies don't have to be immutable in that way. There's no discernible advantage to it other than saying "hey look we're immutable". Your solution is to write more code where none was needed before. The cost-benefit ratio is terribly out of whack, and your only community feedback on this regression is negative. I'm gonna maintain my disagreement and the fork that reverts the regression.

[stalniy]

@shellscape I don't think that example above pushes you to write more code :) we received "negative" feedback only from you and @alexmnv . Both cases are solvable without spy.reset.

By the way, how do you restore ga.trackEvent to original method with spy.reset?

[stalniy]

@shellscape also your statement about immutability is not correct. It was done by purpose which I explained in #35 (comment) and it's not just hey look we're immutable

[shellscape]

Disagree again. Your perspective doesn't take into account others' perspectives. As evidenced by:

I can't imagine the proper use case for this, instead of changing spy's state you always can create new one (it's not smth that takes too much memory or time).

Immutable state is good because you have guarantee that nobody can change it and you protect yourself from writing mess like:

You're forgetting that not everyone works like you work. You're forgetting that flexibility in a testing environment is paramount. Your reasoning is entirely based on your point of view.

I'm sorry man, but while you may think the way you prefer to work is the best way to go, there are plenty of others that disagree. If we were talking webpack, react, chrome, etc in projects that are too large to accommodate insane layers of flexibility - you might have a point. But this a an effin small lib, and you're mandating your patterns onto others. I prefer flexibility and letting people work how they want to work, even if that means completely messing with a spy to do some crazy stuff that you can't imagine. The argument that it adds complexity just doesn't hold water.

[stalniy]

I see your point. Thanks :)

[AdamMcCormick]

I entirely agree with @shellscape, this change means that we are not going to be able to stay on this library. We use spies to mock out external dependencies at import, so "create [a] new one" isn't an option. It would make our tests take ~10 times as long to reinitialize every file for every individual test case where resetting makes this trivial. Spies are, by their nature, NOT immutable as every call made to them mutates them by recording interactions. This change doesn't help your users, it doesn't help the library, and it makes the library significantly less useful.

[AdamMcCormick]

@stalniy Just because you don't receive negative feedback quickly doesn't make the change a good one. We don't all have the luxury of operating on the bleeding edge for all of our dependencies. The only feedback you've gotten on the change has been negative, that should tell you something.

[stalniy]

@keithamus what do you think about this?

[stalniy]

@AdamMcCormick

I’m pretty sure that it won’t make your tests bigger :) this is a strange resilience to refactor own code and desire to use the latest version ;)

Do you know that you can register global afterEach callback which will be called for every test ?

So, probably you already do something like

beforeEach(() => /*very complex spy setup */ )

Then the only think you should do is to add globally

afterEach(()=> chai.spy.restore()) to restore all spies and for the next test beforeEach will add them again.

Don’t take me wrong. I don’t want to make your life harder. What I want is to see short, easy understanble tests in my projects, that’s why such change was suggested. I believe that if you use spy.reset you breaks best practices of writing tests. Because at the point where it’s used you either have issue in project code or in your tests.

P.S.: if other guys from the team think it make sense to add reset method back then I’m ok with that

[keithamus]

I think sandboxes negate the need to restore spies. As @stalniy points out chai.spy.restore() in an afterEach() will work for 90% of existing code. I'm also in agreeement with @stalniy about simplicity - adding reset() encourages complex test flow.

Spies should not need to be reset - if you have code that resets spies in the middle of a test it could be a sign that you need to split the tests out further.

The point of Chai and all plugins isn't just to facilitate testing how you want to write tests, it is also to guide you into a way of writing better tests. Having .reset() doesn't achieve that goal, IMO.

I'll add to that - @AdamMcCormick if you show us your test code where you're using .reset() we can probably show to you how to refactor it to have a better test that doesn't need .reset().

[shellscape]

I don't have skin in this game any longer, as I've moved towards jest and ava in the time since I was active on this thread.

chai is of course not Jest nor Sinon, but it would appear those two fairly well established projects disagree with this stance, and chai is swimming against the current (which may or may not be goal; to differentiate itself)

refs:
https://jestjs.io/docs/en/mock-function-api.html#mockfnmockreset
https://sinonjs.org/releases/v2.0.0/spies/#spyreset

[AdamMcCormick]

Look I don't need your help to know how to refactor my code around an unnecessarily opinionated framework. It's just disappointing that you aren't listening to the users of your library. If I'm going to spend the man hours to refactor my code I'm going to move to a library that makes my tests work the way I have architected them.

@stalniy I make full use of the test life cycle and I don't use beforeEach except to reset my spies. Without reset I have to move the setup that is done once in before which includes fully isolating the code being tested from its dependencies and redo it before each test. My current structure does the "complex" part once, at the beginning, then leverages it through the test suite. In order to recreate the spies every round I'd have to move that setup into a beforeEach handler and that (from our measurements) makes our tests take roughly 10 times as long to run. I've tested this, I used to write tests as you prescribe, I've learned better practices since then.

@keithamus You're providing a library for testing, I'm using your code because I like the readability of chai assertions, not because I need to be taught how to write tests. I've been doing professional development a long time, and if there's one thing I've learned it's that telling your users how they should work is the easiest way to make your product fail. I'll add that - @keithamus if you really want to see my tests I'm happy to share them, but your definition of "better" is not so universal as you seem to believe

[stalniy]

@shellscape afterwards there are quetions and issues like this jestjs/jest#5143

• mock reset
• mock clear
• mock restore

jest is a bad example because it is heavily based on jasmine they even provide jasmine global variable in jest test env. By the way, I like what jest does (e.g., jsdom support) but they have bloated api IMO

@AdamMcCormick could you please show me your tests? I'd like to see few use cases, make benchmarks and probably write an article about how to migrate from using spy.reset to spy.restore

[AdamMcCormick]

Here's a stripped-down test setup from one of our simplest tests (only 2 mocked dependencies), https://gist.github.com/AdamMcCormick/618bf8e9028c9940d7b41fccd06d9294. We have classes with as many as 10 or fifteen dependencies so it only gets more complex. Add a few hundred tests and using beforeEach for mock injection becomes untenable. I'm sure it could be mocked out using your sandboxes, I vehemently disagree that it should be. But by all means, write your blog about knowing better than the rest of the software community, I'm sure it will change everyone's minds.

[AdamMcCormick]

@stalniy You should read the link you posted, it's actually based on the same source where I originally got my pattern from. In particular, look at this comment jestjs/jest#5143 (comment)

[keithamus]

@AdamMcCormick thanks for sharing your tests. I agree with what you're saying, there is a performance hit for some code where the setup time can become untenable to repeat in each test.

The code you have is heavily dependant on externalities that you need to mock out. This is typical of a lot of JavaScript, because there is no clean pattern for dependency injection. Often when I see code like this I push for architectural changes to declare dependencies as function arguments, as it makes the testing path a lot smoother - good examples of this are Angular's $scope or CycleJS' drivers. Following TDD principles usually highlights this sooner rather than later. Of course your code is your code and you're welcome to write it how you want.

But let's get to the brass tacks: we're happy to listen to our users but also we're not going to implement every feature a user asks for. For example we've had plenty of requests in Chai core for a .or assertion so users can write expect(foo).to.be.a('string').or('array'). We don't think this kind of assertion is conducive to good testing. If you do, cool - Chai is pluggable for this exact reason!

Given your set up, and how you are writing complex mocks, I think you might be better off using Sinon + Sinon-Chai over Chai Spies. Sinon has mocks and matchers and built in faking of XHR, you can assert on complex layering of calls, all the stuff it looks like (from the brief test example you've shown us) your code might benefit from. Chai Spies will never do everything that Sinon does. To quote our readme (emphasis mine):

It provides the most basic function spy ability and tests.

Chai Spies is intentionally made to be a simple as possible. If you want something more complex, we fully encourage you to work with Sinon instead!

[AdamMcCormick]

Chai Spies is intentionally made to be a simple as possible.

@keithamus That's exactly what I like about your library (or liked before the change) you weren't opinionated about how my code and tests had to be written and since I'm working on existing enterprise systems that meant the test framework could be integrated without massive refactoring efforts. I find it interesting that you think reset is somehow more complex that sandbox/restore since it took something like 9 lines of code in your baseline to get reset (27cbb16) and hundreds for sandboxing (b11aeeb). If you want the "most basic function spy ability" reset certainly fits the bill better than sandbox/restore

[keithamus]

It's not about the lines of code we ship, it's about complexity of our API and subsequently the intuitiveness of the library and the amount of support/learning it requires.

Sandboxes are an incredibly useful and powerful piece of functionality, and are sufficiently complex enough that for our users to implement would be virtually impossible (without rewriting the whole lib). The cost of complexity in the code is worth it because of the intrinsic value it provides.

.reset() on the other hand introduces API complexity because of the afforementioned issues - people get confused between restore and reset. It's easy to implement because it does a simple thing, but the trade-off lies in the cost to the user: reset clobbers a sensitive prototype, it has an unclear/often conflated name, and it mutates instrumentation mid flow violating the principle of least surprise.

But look, if you really want reset() back, then let's strike a deal: design it with less problems. Find a better name for it, and make it static (so its not hanging off of a spy). That would assuage many of my concerns around its complexity.

[AdamMcCormick]

@keithamus I find your argument about the harm to users unconvincing as your users don't seem to agree with the assertion. The functionality is to take a spy and, in place and without reassignment so as to maintain all references, clear all recorded activity. That doesn't clobber any prototypes (this is a state mutation, no more or less), it does a very specific thing (reset state to the initial proxy https://github.com/chaijs/chai-spies/blob/master/lib/spy.js#L225), and it's far clearer than sandbox and restore to understand and write around. I also don't see value in changing the name from the name that matches all the other libraries, but whatever.

The static method is literally the five lines at https://github.com/chaijs/chai-spies/blob/master/lib/spy.js#L225 with a conditional around it to check if it's a spy first. Why is that so hard to imagine? I could write it as an external utility but that would be coupled to your internal state management which I try not to do.

chai.spy.clearCalls = proxy => {
    if(proxy && proxy.__spy) {
        proxy.__spy.calls = [];
        proxy.__spy.called = false;
    }
};

[AdamMcCormick]

The core problem here is that you seem to assume that I'm spying on something, but I'm actually making a pure mock from spies so that nothing I'm not providing can be called. So rather than mutate dependencies I have no control of (which is how your sandbox and restore work) I never touch the dependencies at all. I'm strictly verifying the parts of their interfaces I use. The dependencies can break and my tests don't, this isolates the cause of failures and makes my unit tests actual unit tests. It also allows me to model specific behaviors in external libraries without a lot of setup.

[keithamus]

@AdamMcCormick the clobbering I was referring to was that the old spy reset method was attached to the spy itself, so it clobbered the function you pass in, if you wanted to spy on something that itself had a reset method then that would be overridden.

I'm happy to look at accepting a PR with the code you've outlined. I encourage you (or anyone else) to raise a PR with tests & documentation. I'm not sure about the clearCalls name, but that is something we can bikeshed separate to this discussion.