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

Spy Restore() #236

Closed
vizjerai opened this issue Jun 6, 2012 · 44 comments
Closed

Spy Restore() #236

vizjerai opened this issue Jun 6, 2012 · 44 comments
Labels

Comments

@vizjerai
Copy link

vizjerai commented Jun 6, 2012

This idea comes from sinon.js where you can setup a spy then later on restore it back to the original method in the case you no longer need or want the spy.

This works well when having a spy setup that is used in most cases but in the rare case it isn't, it's more useful to set one up then for the special case remove it.

Note: this does not clean up after itself for removeAllSpies but doesn't have any bad side effects.

jasmine.Spy.prototype.restore = function() {
  this.baseObj[this.methodName] = this.originalValue;
};
describe('Spies', function() {
  it('should restore spied function', function() {
    var originalFunctionWasCalled = false;
    var TestCase = {
      someFunction: function() {
        originalFunctionWasCalled = true;
        return 'return value from original function';
      };
    };
    this.spyOn(TestClass, 'someFunction');

    TestClass.someFunction.restore();
    expect(TestClass.someFunction()).toEqual('return value from original function');
    expect(originalFunctionWasCalled).toEqual(true);
  });
});
@ragaskar
Copy link
Contributor

ragaskar commented Jun 6, 2012

Given that you can already:

my.spy.andCallThrough();

is there any need for this function? It's true we'd have some problems in the case where spying on a method that has properties (such as a constructor), but we already have this issue.

@vizjerai
Copy link
Author

vizjerai commented Jun 6, 2012

I didn't think about using andCallThrough() and was more concerned about removing the spy all together. And as you suggest it works even though in the background jasmine is still keeping track of the number of times it is called with arguments.

@vizjerai vizjerai closed this as completed Jun 6, 2012
@bradvogel
Copy link

I think this is still valuable. In some cases we want to fully remove to spy and restore the constructor. Method andCallThrough() is insufficient for this.

@ragaskar
Copy link
Contributor

That's a reasonable point. Probably deserves reconsideration. Reopening.

@ragaskar ragaskar reopened this Oct 24, 2012
@shamansir
Copy link

I also vote for this, I am spying on several internal JS methods (like document.createElement) for one tests and I want to ensure that I've totally removed these spies to use the the actual createElement in another test case and create a spy again in very other test case.

@amiuhle
Copy link

amiuhle commented Apr 3, 2013

+1

@ukilon-okta
Copy link

+1,
This is useful for the cases you spy on prototype functions and want it to be fully restored in case another module is using the same function in another spec in the same suite.
Right not what we do is something like:

beforeEach(function () {
  this._originalMethod = obj.prototype.method;
  spyOn(obj.prototype, 'method');
});

afterEach(function () {
  obj.prototype.method = this._originalMethod;
});

having a restore method would be cleaner:

beforeEach(function () {
  spyOn(obj.prototype, 'method');
});

afterEach(function () {
  obj.prototype.method.restore();
});

@remybach
Copy link

remybach commented May 2, 2013

+1

2 similar comments
@giggio
Copy link

giggio commented May 2, 2013

+1

@kevinpauli
Copy link

+1

@infews
Copy link
Contributor

infews commented May 29, 2013

I've added this story to our backlog. This is likely going to make it into 2.0.

Closing.

@infews infews closed this as completed May 29, 2013
@trkoch
Copy link

trkoch commented Jul 17, 2013

+1

1 similar comment
@lc-nyovchev
Copy link

+1

@infews
Copy link
Contributor

infews commented Aug 6, 2013

663a58d

@tarsisazevedo
Copy link

+1

2 similar comments
@newtriks
Copy link

+1

@kilaulena
Copy link

+1

@sheelc
Copy link
Contributor

sheelc commented Nov 21, 2013

To clarify, Jasmine 1.3.1 and Jasmine 2.0 restore the original function after that spec has completed.

Thus from the above example:

beforeEach(function () {
  this._originalMethod = obj.prototype.method;
  spyOn(obj.prototype, 'method');
});

afterEach(function () {
  obj.prototype.method = this._originalMethod;
});

is not actually necessary -- the afterEach and saving off _originalMethod can be removed. After using spyOn with some object (even the object referenced by a prototype), Jasmine will restore the original method when the spec completes.

Additionally, in 2.0 we now preserve properties from the original function and move them over to the spy. This preservation + andCallThrough should hopefully make spies less obtrusive.

There might still be some use cases that need the spy completely gone in the middle of a spec, but just wanted to point out that cleanup is taken care of.

@emadridm
Copy link

emadridm commented Aug 1, 2014

+1

2 similar comments
@guy-mograbi-at-gigaspaces
Copy link
Contributor

+1

@phuong3030
Copy link

+1

@slackersoft
Copy link
Member

I'm seeing lots of +1's but I'm not entirely sure what is being asked for given @sheelc's comment that the spies get removed after each spec.

What in particular is being asked for at this point?

@guy-mograbi-at-gigaspaces
Copy link
Contributor

Does sheelc's comment also apply to jasmine_node?

@slackersoft
Copy link
Member

Jasmine has restored the spy's original value after the spec for a while now, so whatever version of jasmine has been bundled into jasmine-node should do this already.

@dts
Copy link

dts commented Jan 15, 2015

+1, I would like this in case I want to re-spy a method during the same run. I.e. the first time I want the function to do thing X, and later in that run I want it to do Y.

@slackersoft
Copy link
Member

@dts you can already do this by specifying a new plan. e.g. (warning untested code)

it('spies a lot', function() {
  var spy = jasmine.createSpy('spy).and.return('foo');
  expect(spy()).toEqual('foo');

  spy.and.callFake(function() {
    return 'bar';
  });
  expect(spy()).toEqual('bar');
});

@guy-mograbi-at-gigaspaces
Copy link
Contributor

well, I guess I can simply use sinon if I really want this feature. sinon supports frontend..

@dts
Copy link

dts commented Jan 19, 2015

@slackersoft - Yup - you're totally right! Consider my +1 rescinded!

@slackersoft
Copy link
Member

@guy-mograbi-at-gigaspaces (or anyone else) we're still not totally sure what is being asked for here. The issues I've seen are (I think):

  1. Removing spies after a spec completes. Jasmine has done this for you for a long time and this shouldn't be needed.
  2. Changing what the spy does after initial setup. See my comment earlier about how to accomplish this.
  3. Wanting to call the original method. This seems like and.callThrough() would solve this.

If there's another issue I've missed or if callThrough() isn't a good enough solution for calling the original method, let me know. Otherwise, it doesn't sound like there is an issue here.

@slackersoft
Copy link
Member

Since I haven't seen anyone say I missed anything in my previous comment, it sounds like all of the cases where people were wanting to restore a spy are handled by jasmine currently. I'm going to close this issue.

If there is another case I missed, please open an issue about that particular case.

Thanks for using jasmine!

@tiriana
Copy link

tiriana commented Apr 20, 2015

+1

@slackersoft
Copy link
Member

@tiriana as has been discussed in this issue, we don't believe it should ever be necessary for a user to manually restore a spy. If you have an actual suggestion of where this is needed, please create a new issue.

@tiriana
Copy link

tiriana commented Apr 21, 2015

@slackersoft I didn's see, sorry for too-fast '+1'. So jasmine already clears/restores spy after each spec? didn't know, but if so - that's cool, and enough for me.

@bodawei
Copy link
Contributor

bodawei commented May 25, 2015

I'd like to +1 this too. It is for, I admit, a pretty obscure case.
I've got a routine which replaces $.ajax at times. At other times, it will undo itself and return the original $.ajax to be there. I also have tests which spy on $.ajax. In some very unusual cases, my tests then fail because the routine that undoes itself says "hey, that $.ajax isn't mine. I can't undo myself".

(I can change it so that it looks at spy.originalValue and notices that THAT is its own, so it knows it can restore $.ajax to what it was before... except that it can't, because when the test finishes, jasmine helpfully restores $.ajax to the one that tried to be removed.)

Put more tersely:
$.ajax // normal one is in place
test start
replace $.ajax with custom.ajax
spyOn($ "ajax");
replace custom.ajax with $.ajax
test end // jasmine replaces $.ajax with custom.ajax

I guess, more generally, the point is: if for some reason one cares about the exact instance, and there is no way to tell jasmine 'I changed this behind your back', sad things happen

(my workaround is to replace spy.originalValue with the value that I have already set $.ajax with. ugly? yes. I'm open to other ideas)

@slackersoft
Copy link
Member

@bodawei it sounds to me like you probably want to be managing your testing around $.ajax in some other way so you don't have both a spy and a fake implementation in the same test. Have you looked at something like jasmine-ajax for your case? Jasmine-ajax actually stubs out the XHR request by replacing the XMLHttpRequest constructor and provides a full fake implementation of XHR requests. If you then still needed to spyOn jQuery's ajax function to test for some params that (for example) would cause jQuery to make a jsonp request instead of normal XHR, you wouldn't be mucking with the same function.

@bodawei
Copy link
Contributor

bodawei commented Jun 15, 2015

Thanks for the suggestion, @slackersoft. Alas, that really isn't an option. We've got a mock server ( https://github.com/delphix/dxData/tree/master/dxCoreData/mockServer ) which sits in the background behind all our tests, and automagically services all server interactions. It kinda does the same thing as jasmine-ajax but more automatically. Since it is so omnipresent in our tests, turning it on and off in some fashion to get around the jasmine situation is much too cumbersome (since spying on $.ajax is a distinct minority of our test cases).

I can see an alternative would be to have the global beforeEach install a spy at $.ajax which does a andCallFake() which then calls the "real" mock server $.ajax handler. That seems, however, like a lot of work to do thousands of times in a test run when (again, the cases where we need to spy on $.ajax and it is a problem is a tiny, tiny minority of the cases). So, as it is I've ended up modifying the spy's data structures behind its back in these cases so it replaces the right routine. Ugly, but it seems the best tradeoff given options at the moment.

@slackersoft
Copy link
Member

You might also be able to:

spyOn($, 'ajax').and.callThrough();

... in a global beforeEach to call through to your mock server and then just change the execution plan for the spy with $.ajax.and.returnValue() (or whichever) for the particular tests that don't use the mock server.

@bodawei
Copy link
Contributor

bodawei commented Jun 28, 2015

I agree, that is another possibility.

@guy-mograbi-at-gigaspaces
Copy link
Contributor

How can I check haveBeenCalled multiple times in a single test?
wouldn't the second time remember the first time?

@bodawei
Copy link
Contributor

bodawei commented Jun 28, 2015

@guy-mograbi-at-gigaspaces, I for things like that, I use the call count (.e.g mySpy.calls.count()) to see if the number of counts has changed between calls.

@That-David-Guy
Copy link

A restore() method would be useful in my use case. reset() and callThrough() don't need to do what I need.

Use Case:

  1. I have a class which calls a whole bunch of methods in the constructor
  2. I want unit tests for each of those methods
  3. In a beforeEach() I want to mock (using spyOn) each of the methods then create an instance of the class
  4. In each test I want to .reset() the particular method I'm testing then test that method.

The only way I can see to do this now is by mocking every method but the one I want and create the instance in every single test. However that means I'm testing that the constructor calls the method AND the method itself, which I don't want.

Unless there is a better way to architect this type of code?

.reset() doesn't work because it seems to only reset the number of calls
.and.callThrough() doesn't work because I can't put two spys on the one method
Destroying the spec after each method doesn't work because it is too late by then

@brandonros
Copy link

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.

@hakunin
Copy link

hakunin commented Apr 17, 2017

@brandonros exactly same case here

  1. I have a top level spy that most cases can use
  2. want to throw away and re-define the spy for one test group

Pushing the definition down to them would mean adding 3 repetitive lines per group.

@slackersoft
Copy link
Member

If you simply need to change the behavior of the spy when called, you should be able to use another .and. on the existing spy, without needing to remove the spy and recreate it.

@brandonros can you go into more depth about why existingSpy.and.callThrough() doesn't help?

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

Successfully merging a pull request may close this issue.