Allow to manually destroy a spy

[tkrotoff]

Following the discussion on Google Groups: https://groups.google.com/forum/#!topic/jasmine-js/0RXvFo707EQ, I think my usecase is valid.

Jasmine does not allow to manually delete/destroy a spy and this is limiting.

function signIn(done) {
  ...
  // Fails the 2x time with "<spyOn> : bar has already been spied upon"
  const mySpy = spyOn(foo, 'bar').and.callThrough();
  ...
  expect(foo.bar).toHaveBeenCalledTimes(1);

  // Jasmine missing feature:
  //mySpy.destroy();
}

function signOut(done) {
  ...
}

it('should be able to sign in/sign out multiple times', done => {
  signIn(() => {
    signOut(() => {
      signIn(() => { // Fails with "<spyOn> : bar has already been spied upon"
        done();
      });
    });
  });
});

signIn, signOut... are bricks (i.e simple functions - not Jasmine tests) that are reused to write complex Jasmine test scenarios like:

• signIn, doSomething1, signOut
• signIn, doSomething1, doSomething2, doSomething3, signOut
• signIn, signOut, signIn, signOut, signIn, signOut

Remarks:

• Currently the second signIn call fails with <spyOn> : bar has already been spied upon
• I cannot make spyOn global otherwise error Spies must be created in a before function or a spec
• Adding beforeEach(()=> { spyOn(foo, 'bar').and.callThrough(); }) is not the solution because expect(foo.bar).toHaveBeenCalledTimes(1) will fail with the 2x signIn call (because .toHaveBeenCalledTimes(2))

[tkrotoff]

expect has spy.restore(), same for Sinon.JS. Jasmine is missing spy.restore(), this has been reported multiple times:

• Spy Restore() #236 (Jun 6, 2012)
• Added the ability to remove an individual spy #342 (Mar 16, 2013)
• Restore behavior for spy #683 (Oct 6, 2014)
• restore method for a spy #975 (Nov 17, 2015)

Here you have the simplified use-case from above working with spy.restore(): mjackson/expect#196 (comment)

Time to reconsider?

[brandonros]

Why doesn't this exist yet?

Use case 1: in a more "global" beforeEach, you set up a spy. Then, in a very specific test, you need to override that spy with another spy. Overriding with andCallThrough does not help.

[slackersoft]

@brandonros if you simply want to change the strategy for the spy to execute when called, you can always call and.callThrough again on it later without needing to remove the spy and recreate it.

@tkrotoff if your primary concern with the global spy is call counts continuously increasing, you should also be able to call calls.reset() on your spy to clear out any calls that have already been tracked.

This still doesn't sound like an issue that requires actually removing the spy.

[ljharb]

@slackersoft if things keep references to the original, or to the spy, such that actually removing the spy changes how === works, then absolutely that would require it. I'm not sure why there's resistance to something that allows truly cleaning up after oneself, as opposed to leaving garbage on spied-upon objects and just expecting other tests to guess or know that there's a spy there.

[slackersoft]

@ljharb As mentioned in the other issues, Jasmine will clean up any spies that were created with spyOn at the end of the spec or suite where they were instantiated, so your code shouldn't need to do this for itself.

If you are assigning the output of createSpy to an object, Jasmine can't clean replace it because it doesn't know where that spy lives. Even an explicit unspy type of thing is not possible to implement for this as the only thing that knows what to put back is the code that did the assignment.

If you want to check for strict equality of a function that you're spying on for some tests:

• Why is the identity important and not what the function does
  • If you just want the real thing to happen for a single spec you can change the strategy to callThrough for that spec.
• If the function is already being removed and replaced an identity check would seem to be testing the Jasmine framework itself and not your code.

We haven't implemented this feature yet, because the use cases for unspy that I have seen have all been solvable with existing Jasmine functionality. Since every feature has an ongoing maintenance cost, if we can solve peoples problems with existing code, we prefer to do that as it let's us solve a greater breadth of problems.

[UziTech]

We haven't implemented this feature yet, because the use cases for unspy that I have seen have all been solvable with existing Jasmine functionality.

Everything jasmine can do can be solved with other functionality. (e.g. expect(x).toBeGreaterThan(y) can be easily solved with expect(x > y).toBe(true))

The point is that unspy is a more direct and understandable way to reset the original method then spy.calls.reset(); spy.and.callThrough(); the same way that toBeGreaterThan is more understandable than toBe

[slackersoft]

Let me rephrase this then. The only use cases I've seen for when people want to unspy on something involves them immediately re-spyOn that same thing to change something. So the unspy version of calls.reset() or and.callThrough() is:

unspy(thing, 'method');
spyOn(thing, 'method').and.callThrough();

This is actually more complicated to use than the existing functionality, so it doesn't seem like a very good reason to add unspy.

[ljharb]

Unless those two lines live in different places, in which case separating them into two calls is much better than conflating two goals into the andCallThrough call.

[UziTech]

My point is that this is obviously a wanted feature, even if it is just for semantics.

My pull request uses the feature already in place to clear the spies therefore I believe more time and effort is spent by the jasmine team to come up with reasons not to have this feature than would be spent maintaining it.

[ChrisBellew]

I want to clarify the use case that some kind of "remove spy" function would give me.

var foo = { bar: () => {} };

describe('one', () => {
    beforeEach(() => {
        spyOn(foo, 'bar').and.returnValue(1);
    });
    it('returns 1', () => {
        expect(foo.bar()).toEqual(1);
    })
    describe('two', () => {
        beforeEach(() => {
            // Remove first spy here?
            spyOn(foo, 'bar').and.returnValue(2);
        });
        it('returns 2', () => {
            expect(foo.bar()).toEqual(2);
        })
    });
})

Is this better done another way? A nice removeSpy(foo, 'bar') would be pretty nice

[slackersoft]

@ChrisBellew based on what you have in that example, you should be able to change the strategy for the inner beforeEach, something like this.:

beforeEach(() => {
    foo.bar.and.returnValue(2);
});

[marcoturi]

@slackersoft using your example i get TypeError: Cannot read property 'returnValue' of undefined. On Jasmine 2.6.4 in an angular 4 project.

[slackersoft]

That probably means that foo.bar isn't a spy at that point, if .and is returning undefined, possibly due to some other configuration. When I run the following spec, everything works fine:

var foo = { bar: () => {} };

describe('one', () => {
    beforeEach(() => {
        spyOn(foo, 'bar').and.returnValue(1);
    });
    it('returns 1', () => {
        expect(foo.bar()).toEqual(1);
    })
    describe('two', () => {
        beforeEach(() => {
            // Remove first spy here?
            foo.bar.and.returnValue(2);
        });
        it('returns 2', () => {
            expect(foo.bar()).toEqual(2);
        })
    });
})

[sc-lewis-notestine]

for posterity:

I came across this page in a search for how to deal with something I want to both mock in a beforeEach block and use as a test outcome.

I got around it by using toHaveBeenCalledWith in addition to the spyOn block, as follows:

describe('when we expect $location to mean one thing before execution and another thing after', function() {


      beforeEach(function() {
        //mock out the first call to location search.
        spyOn($location, 'search').and.returnValue({
            uuid: 'test-current-node-uuid',
            customerId: currentCustomer
          });
      });

      it('should have the appropriate query string parameters set, after execution', function() { 
        objectUnderTest.foo();

        //Verify that the second call came through with different info.
        expect($location.search).toHaveBeenCalledWith({ 
          uuid: 'some-other-uuid', 
          customerId: 'some-other-id'
        });
      });
    }); 

[slackersoft]

@sc-lewis-notestine I'm glad you found something that works for you. Note that you might want to check calls.mostRecent().args to make sure that it is the last call to $location.search() that has the arguments you need.

[profiprog]

There is also possible to add support for Spy.restore() directly in test or in helper file:

beforeAll(function () {
	var oringSpyOn = spyOn;
	spyOn = (obj, fnName) => {
		var originalValue = obj[fnName];
		var spy = oringSpyOn(obj, fnName);
		spy.restore = function () {
			obj[fnName] = originalValue;
		};
		return spy;
	};
});

[OscarGodson]

If there's a way to do this without resetting the spy I'm happy to do it another way but I'm not sure how. I have some components that will cache state upon initialization. Here's a simplified example:

function Parent () {
  var cachedState = MyApp.getState();
  return Child({
    isVisible: [cachedState.isPaidAccount]
  });
});

Then I have a test like:

describe('Parent', function () {
    beforeEach(function () {
		spyOn(window, 'Child');
		this.args = window.Child.calls.argsFor(0)[0];
    });

  // ...
		it('sets visibility to true if isPaidAccount is true', function () {
			// ???
		});

That it is where im not sure what to do. I can go back and move that original spy to every test, I can slow down the app some by looking up state for every operation or I wish I could simply do window.Child.removeSpy() or something so that I can re-call it with a new stubbed state. I'm stubbing the state easy enough with a spyOn(MyApp, 'getState').and.returnValue but the problem is that the state was already called.

Suggestions for a work around?

[slackersoft]

@OscarGodson Maybe something got lost in the simplification process, but from what you've got there it sounds like you really just want a unit test of the Child object without necessarily creating it via Parent, here's why:

If you spyOn Child (even callThrough so the real implementation happens), you still run into the problem that the Parent call has already cached off its state. If what you want to do right now in that it is change the spy behavior of the window.Child function, you can do that with window.Child.and.returnValue() or whichever behavior your want. Unfortunately, this doesn't change the fact that MyApp.getState() has already been called and its value saved.

Hope this helps. Thanks for using Jasmine!

[jackson-sandland]

Guys, what is the deal? The community has been asking about a spy.Restore() method for like 5 years. I'd be open to writing this for you if there are sufficient resources to quickly put it together. If not, would love to see this method added.

[UziTech]

This already had a PR #1289

@slackersoft doesn't seem to want people to restore a spy

[slackersoft]

The older github issues asking to restore a spy all centered around wanting to clean up spies installed with spyOn at the end of the spec. Since Jasmine handles this for you, this doesn't seem like a compelling reason for Jasmine to add explicit restore. Other spy/expectation libraries, like expect and sinon, need this feature since they're not integrated with the test execution itself, so the user needs to tell the library when it is safe to clean up spies. If you're migrating from another spy library to Jasmine's built in spies, it should be safe to just delete these restore calls.

Another issue centers around the need to change the strategy for the spy, possibly resetting the call counts as well. Using the existing functionality already built into Jasmine seems like it more accurately expresses the intent.

spyOn(foo, 'bar').and.callThrough();
foo.bar.calls.reset();
foo.bar.and.returnValue(3);

or with jasmine.getEnv().allowRespy(true);

spyOn(foo, 'bar').and.callThrough();
spyOn(foo, 'bar').and.returnValue(3);

The original post for this issue seems like it could be solved similarly with similar clarity arguments, leaving signIn looking something like:

function signIn(done) {
  foo.bar.calls.reset();

  doStuff();

  expect(foo.bar).toHaveBeenCalledTimes(1);
}

beforeEach(() => {
  spyOn(foo, 'bar').and.callThrough();
});

In this case I might also question the assumption that it is valuable to check the call to foo.bar() in every test that needs to execute in a logged in state, as this can cause heavy coupling to the actual implementation of logging a user in in tests that don't need it.

Looking back at this conversation, it appears I might not have done as well as I had hoped at giving good examples or explaining why I'm asking these questions and suggesting current functionality to solve user's issues. The intent here was never to say that Jasmine will never have this functionality, but that we need to better understand the use case for it to make sure we come up with the right solution. During this discussion of use cases and solutions is not a good time to send in a pull request for the feature, which is why I referred back to this issue and discussion from @UziTech's pull request and left it open pending this discussion.

I'm happy to revisit this feature, and pull request, with the new understanding about the differences between people aesthetic choices in their test suite and desire for compatibility with other mocking frameworks.

Hopefully this clears up some of the confusion. Thanks for using Jasmine!

[OscarGodson]

heh... i just ran into this again so i just saw the response Nov 😂

@slackersoft regarding it being cached, it wouldnt happen because I'd be doing like this (this code just has different function names by copy pasta'd

			spyOn(Devices, 'is').and.returnValue(false);
			this.bt = new BaseToolbar();
			expect(this.locationButton.visibility).toEqual([false]);

			spyOn(Devices, 'is').and.returnValue(true);
			this.bt = new BaseToolbar();
			expect(this.locationButton.visibility).toEqual([true]);

future instances of this.bt will have a cached visibility but since im making new instances it will be cached with the stubbed data.

I ended up fixing it based on your answer. In case anyone else runs into this that code above changed to this:

			spyOn(Devices, 'is').and.returnValue(false);
			this.bt = new BaseToolbar();
			expect(this.locationButton.visibility).toEqual([false]);

			Devices.is.and.returnValue(true);
			this.bt = new BaseToolbar();
			expect(this.locationButton.visibility).toEqual([true]);

[Humberd]

It should be implemented. I don't see a reason not to have this feature. Jest already has it.

[fider]

It is implemented but usage is defferent:

spyOn(a, "method").and.callFake( behaviorA );

// Now you want behavior B instead of A
// (type casting <> is for typescript)
(<jasmine.Spy>a).method.and.callFake( behaviorB );

// And now you want to restore original behavior:
(<jasmine.Spy>a).method.and.stub();

// Behavior B again
(<jasmine.Spy>a).method.and.callFake( behaviorB );

However I propose to add Spy.reset / remove method so it will be possible to use object without "overhead" (for "some" reasons)

[argelj289]

@brandonros if you simply want to change the strategy for the spy to execute when called, you can always call and.callThrough again on it later without needing to remove the spy and recreate it.

you are wrong

I see in the log that when it gets spied in another it(), it errors out that message

[UziTech]

You can use jasmine-unspy to add a method to remove the spy.

[Antoniossss]

I have
const consoleSpy = spyOn(console, 'warn');

how to unspy console.warn now??

I don't think this is that uncommon need to spy on globals - obviously one can refactor to use service/providers everywhere and not use glboals - but is it really the case?

[slackersoft]

@argelj289 can you provide a fully contained example suite that demonstrates the behavior you are describing? From your message, I'm not sure I understand what you're seeing and what you expect to be seeing.

[slackersoft]

@Antoniossss Jasmine doesn't currently provide a way to remove a spy during the execution of a single spec. This is because Jasmine will clean up spies in between specs to help ensure you don't get test pollution. If you've created the spy in a beforeAll it won't get cleaned up until all the specs in wrapping describe are run, since that's what you told Jasmine you wanted.

[nbilyk]

Most of the time I like that you can't restore a spy because it makes it very clear that you don't have to. In Jest I see tests manually restoring mocks and there's confusion whether they need to do that or not.

However, there's an edge case --
If you use clock.install / uninstall, jasmine mocks the timer methods, which you then have no access to the spy. If you try to spy on the timer methods yourself, if you created the spy after the clock install the auto-restore (run after afterEach) will restore to the clock mocks, and if you create the spy before the clock install, the clock install will fail.

[ljharb]

@nbilyk of course you have to - if you care about the call counts or arguments.