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

[jest] Add mock functions for v22 #1918

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Mar 6, 2018

Add mockResolvedValue, mockResolvedValueOnce, mockRejectedValue, mockRejectedValueOnce to jest mocked functions

@julienw
Copy link
Contributor Author

julienw commented Mar 6, 2018

There are test errors:

 * jest_v22.x.x/flow_v0.39.x- (flow-v0.66.0): Unexpected Flow errors(0):
   Warning ------------------------------------------------------------------------------------ 0-test_jest-v22.x.x.js:52:1
   
   Unused suppression comment.
   
      52| // $ExpectError Mock function expected to return Promise<number>, not Promise<string>.
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   
   
   Warning ------------------------------------------------------------------------------------ 0-test_jest-v22.x.x.js:54:1
   
   Unused suppression comment.
   
      54| // $ExpectError Can't return a Promise from a non async function
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   
   
   Warning ------------------------------------------------------------------------------------ 0-test_jest-v22.x.x.js:58:1
   
   Unused suppression comment.
   
      58| // $ExpectError Mock function expected to return Promise<number>, not Promise<string>.
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   
   
   Warning ------------------------------------------------------------------------------------ 0-test_jest-v22.x.x.js:60:1
   
   Unused suppression comment.
   
      60| // $ExpectError Can't return a Promise from a non async function
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

But I don't have a good idea to fix them... Any idea ?

@villesau
Copy link
Member

villesau commented Mar 6, 2018

Test suite seems to take diff for grandparent instead of parent: #1920

@AndrewSouthpaw
Copy link
Contributor

Restarting build...

@AndrewSouthpaw
Copy link
Contributor

Derp. That won't fix the problem. @julienw can you rebase against the latest master? There's a commit that should fix your build problem, assuming @villesau is correct.

@villesau
Copy link
Member

villesau commented Mar 7, 2018

Oh, sorry i totally misread the errors. They seems to come from Jest, not from unrelated libdefs. So rebasing in this case doesn't help.

@julienw
Copy link
Contributor Author

julienw commented Mar 8, 2018

Yep totally, the error definitely comes from my typing :-) I don't understand why the typing doesn't yield these errors and were seeking for some help.

@gantoine
Copy link
Member

gantoine commented Mar 8, 2018

@julienw Could you still try rebasing this PR on the latest version of master?

@gantoine gantoine added the needs updates The PR needs updates before shipping (eg. failing build, bad file path) label Mar 8, 2018
@julienw
Copy link
Contributor Author

julienw commented Mar 9, 2018

Oh sure, I just did !

@gantoine
Copy link
Member

gantoine commented Mar 9, 2018

Awesome. I just wanted to make sure that the errors were indeed coming from the typings, and that any changes weren't going to fail because of an unrelated issue.

@gantoine gantoine added the libdef Related to a library definition label Mar 21, 2018
@AndrewSouthpaw
Copy link
Contributor

Ping @gantoine :) Looks like you had something in mind to check, so I'll let you be the one to pull the trigger on the PR.

@gantoine
Copy link
Member

@julienw It seems the specs are failing again, and I don't think it's due to any issues in the master branch. When you get a chance, could you take a look at them?

@julienw
Copy link
Contributor Author

julienw commented Apr 10, 2018

Yes, actually, that's what I said in #1918 (comment) :) I don't know how to fix these problems and I'd like some help here.
Basically, Flow doesn't report errors here while it should. I use an utility called ExtractPromiseValue to try to extract the type of a promise value, but I think I'm not doing it properly...

@gantoine gantoine added on hold and removed needs updates The PR needs updates before shipping (eg. failing build, bad file path) labels Apr 14, 2018
@gantoine gantoine self-assigned this Apr 14, 2018
@julienw
Copy link
Contributor Author

julienw commented Apr 18, 2018

From my test the ExtractPromiseValue util should work. I'll give it another shot.

@julienw
Copy link
Contributor Author

julienw commented Apr 18, 2018

I simplified the problem in try. Maybe the problem is how Flow infers the parameters to JestMockFn.

@AndrewSouthpaw
Copy link
Contributor

AndrewSouthpaw commented May 2, 2018

Looks like the errors are different at least. :) LMK if you're still needing help.

@julienw
Copy link
Contributor Author

julienw commented May 2, 2018

Yeah, if you can help to find what doesn't work in the try I mentioned above, it would help a lot. We should have errors in that try but we don't have any. I think the problem is how JestMockFn is infered. But maybe this shouldn't be done like this...

@julienw
Copy link
Contributor Author

julienw commented May 2, 2018

Otherwise I can just remove these test cases so that we can land this already.

@AndrewSouthpaw
Copy link
Contributor

Have you tested this in the wild? Does it work there?

Otherwise I'd change the type def to not extract the promise value and worry about that, just provide something like Promise<*>.

@AndrewSouthpaw AndrewSouthpaw self-requested a review May 2, 2018 19:36
@julienw
Copy link
Contributor Author

julienw commented May 3, 2018

I think we can't do this. We really want to extract the type because that's what we pass to these functions.

Eg if a mock function returns a Promise<number>, we want mockResolvedValue to take a number. But I can't make flow check it properly.
Same, if a mock function returns a number we want that Flow forbids to call mockResolvedValue. Again I can't make flow check this.

For example if we were in the Rust world, I could do something like impl AsyncMock for JestMockFn<A, Promise<R>> :-)

Maybe we can do something similar here, converting JestMockFn to a class or an interface.

I use this in the wild.

@julienw
Copy link
Contributor Author

julienw commented May 3, 2018

Maybe we can do something similar here, converting JestMockFn to a class or an interface.

I tried it and stumbled on facebook/flow#5437

@AndrewSouthpaw
Copy link
Contributor

AndrewSouthpaw commented May 4, 2018

Hm. Seems like we've been spinning our wheels here for a while. I'd be fine with flipping the table and just putting any there. Flow is far from perfect, and we shouldn't let perfect become the enemy of better.

If you're fine going that route, just make sure to link to this conversation and briefly explain how it's essentially impossible right now.

@julienw
Copy link
Contributor Author

julienw commented Dec 19, 2018

I see #2396 reused part of this PR to land the feature. I'm a bit surprised that it didn't use ExtractPromiseType though. And I find issues on my end for mockRejectedValue, I'm surprised the tests were passing at all...

@julienw
Copy link
Contributor Author

julienw commented Dec 19, 2018

OK I find that the typings are failing for things like:

foo.doAsyncStuff = jest
  .fn()
  .mockResolvedValueOnce(10)
  .mockResolvedValueOnce(42);

foo.doAsyncStuff = jest.fn();
foo.doAsyncStuff.mockRejectedValueOnce(new Error("hello world"));

Using a type closer to this PR makes these work, but fail other existing valid and important tests, especially this isn't checked anymore:

// $ExpectError Mock function expected to return Promise<number>, not Promise<string>
foo.doAsyncStuff = jest.fn().mockResolvedValue("10");

@julienw
Copy link
Contributor Author

julienw commented Dec 19, 2018

This is my current try: https://github.com/flow-typed/flow-typed/compare/master...julienw:jest-resolved-values-fix?expand=1

Various tests are failing... the 2 I was mentioning above, plus new tests failing in latest 2 flow versions :/

@Brianzchen Brianzchen changed the title Add mockResolvedValue, mockResolvedValueOnce, mockRejectedValue, mock… [jest] Add mock functions for v22 Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libdef Related to a library definition on hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants