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

Add spyOn function introduced in Jest 19 #14867

Merged
merged 5 commits into from Mar 12, 2017

Conversation

Projects
None yet
6 participants
@alexjoverm
Copy link
Contributor

alexjoverm commented Feb 25, 2017

Please fill in this template.

  • Make your PR against the master branch.
  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run tsc without errors.
  • Run npm run lint package-name if a tslint.json is present.

If changing an existing definition:

@dt-bot

This comment has been minimized.

Copy link
Member

dt-bot commented Feb 25, 2017

jest/index.d.ts

to authors (@pspeter3 @vsiao @NoHomey @jwbay @asvetliakov). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?
@@ -83,6 +83,8 @@ declare namespace jest {
function useFakeTimers(): typeof jest;
/** Instructs Jest to use the real versions of the standard timer functions. */
function useRealTimers(): typeof jest;
/** Creates a mock function similar to jest.fn but also tracks calls to object[methodName] */
function spyOn(object: any, method: string): typeof jest;

This comment has been minimized.

@asvetliakov

asvetliakov Feb 25, 2017

Contributor

I'd suggest this

function spyOn<T extends {}, M extends keyof T>(object: T, method: M): MockInstance<T[M]>;

Also return type typeof jest is wrong. jest.spyOn returns mock/spy instance not the global jest object

Better T extends object but this require TS 2.2 and PR won't be merged until 4-6 weeks passed

This comment has been minimized.

@alexjoverm

alexjoverm Feb 25, 2017

Author Contributor

@asvetliakov you're right about the return type, my mistake.

About the other thing, I see the benefit so you assure is an existing property of that object. Does that take into account custom properties? Because I assume T extends {} creates a new Object type.

Anyway, if that'll take that much time, I could always create another PR by that time, but at least if we merge this now spyOn will be usable in ts-jest.

This comment has been minimized.

@asvetliakov

asvetliakov Feb 25, 2017

Contributor

T extends {} infers type from parameter, so it will be your type that you had passed into spyOn. keyof T will filter properties for this type only.

@alexjoverm

This comment has been minimized.

Copy link
Contributor Author

alexjoverm commented Feb 25, 2017

I've also realized I didn't put it in the right alphabetical order.

What about the new change? It's not as good as what @asvetliakov but it will work right now since doesn't need the keyof operator.

function spyOn<T extends {}>(object: T, method: string): Mock<T>;

@asvetliakov

This comment has been minimized.

Copy link
Contributor

asvetliakov commented Feb 25, 2017

Why you dislike keyof operator 😄 ?

@alexjoverm

This comment has been minimized.

Copy link
Contributor Author

alexjoverm commented Feb 25, 2017

Sorry, got confused. I do like keyof operator and knew it is part of TS 2.1, but I assumed extends was there before 😂

Well, to return Mock<T> is necessary to specify the T but true it doesn't need to extend {}.
I see the fn function implementation also uses extend {}.... So couple of questions here:

Is it ok the following implementation? And will it take long to get merged?

function spyOn<T>(object: T, method: string): Mock<T>;

If not, then should we go for what you @asvetliakov?

function spyOn<T extends {}, M extends keyof T>(object: T, method: M): MockInstance<T[M]>;

And, would it matter too much the return type to be Mock or MockInstance?

Sorry for these many questions but I'm not a TS expert and is my first PR to this repo 😄
Thanks for the time :)

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Mar 10, 2017

@alexjoverm can you please refresh the PR against master.

@jwbay

This comment has been minimized.

Copy link
Contributor

jwbay commented Mar 10, 2017

@alexjoverm The second option in your last post is definitely preferable. About MockInstance vs Mock, the difference is that Mock has construct and call signatures. If spyOn returned a Mock type, you'd able to do this

const spy = jest.spyOn(obj, 'foo');
spy(); //call
new spy(); //construct

But I don't know when it makes sense to call your own spy in a test. And if you really wanted to, you could just call the method directly on the object instead.

Something this PR is missing though is mockRestore. See the example test here: http://facebook.github.io/jest/docs/jest-object.html#jestspyonobject-methodname. Although it can be called on every mock, it may be confusing since it's a no-op on everything except spies. I would suggest this for spyOn's return type.

interface SpyInstance<T> extends MockInstance<T> {
  mockRestore(): void;
}
@alexjoverm

This comment has been minimized.

Copy link
Contributor Author

alexjoverm commented Mar 11, 2017

@mhegazy @jwbay Wow, thanks for that big help.

Totally have missed the mockRestore. I'll update it with your suggestions as soon as I can, and probably will do a git rebase to clean up the history.

alexjoverm added some commits Mar 11, 2017

@alexjoverm

This comment has been minimized.

Copy link
Contributor Author

alexjoverm commented Mar 11, 2017

Ok done. Since hundreds of commits are in between because of the merge, I preferred not to rewrite history, just in case it breaks something for others.

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Mar 11, 2017

@jwbay mind taking another look.

@jwbay

jwbay approved these changes Mar 11, 2017

Copy link
Contributor

jwbay left a comment

👍

@mhegazy mhegazy merged commit 545f81a into DefinitelyTyped:master Mar 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment