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

Adding support for verify(...).timeout(ms) #97

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

johanblumenberg
Copy link
Contributor

Adding support for waiting until an asynchronous method is invoked. This is useful when mocking an API that doesn't use promises that you can await, but uses callback functions.

Inspired by: http://static.javadoc.io/org.mockito/mockito-core/2.16.0/org/mockito/Mockito.html#verification_timeout

Example:

await verify(mockedFoo.getBar()).timeout(10000);

This code will wait until foo.getBar() was invoked, and fail if it isn't invoked within the given time.

@codecov-io
Copy link

codecov-io commented Mar 21, 2018

Codecov Report

Merging #97 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #97     +/-   ##
=========================================
+ Coverage    94.9%   95.01%   +0.1%     
=========================================
  Files          34       34             
  Lines         609      622     +13     
  Branches       70       72      +2     
=========================================
+ Hits          578      591     +13     
  Misses         22       22             
  Partials        9        9
Impacted Files Coverage Δ
src/MethodStubVerificator.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1c7f69...68e093e. Read the comment docs.

@johanblumenberg
Copy link
Contributor Author

Rebased on latest

@NagRock
Copy link
Owner

NagRock commented May 22, 2018

Hi, sorry for my late response.

Thanks for contributing but this API looks little bit strange. While verify(mockedFoo.getBar()) allows us to use once, twice, calledAfter etc. timeout function breaks this API. We cant with this implementation do something like verify(mockedFoo.getBar()).timeout(...).twice() etc.

Also I don't know if this should be part of mocking framework. I think this can be handled by external function.

@bendykowski what do you think?

@johanblumenberg
Copy link
Contributor Author

johanblumenberg commented May 22, 2018

Also I don't know if this should be part of mocking framework. I think this can be handled by external function.

An external function would look something like this?

let deferred = new DeferredPromise();
when(mock.method()).thenCall(() => deferred.resolve());

...

await promise;

This means two lines of setup, and one line of actually waiting. So it's more typing. It's also backwards to do most of the waiting related things in the setup phase, not in the verification phase, like this:

await verify(mock.method()).timeout(1000);

One thing to think about is that if you are doing it in the setup phase, like above, there is no way to set mocked return values. If your test is overriding the return value, the waiting setup will fail.

let deferred = new DeferredPromise();
when(mock.method()).thenCall(() => deferred.resolve());

...

when(mock.method()).thenReturn(3); // This clears the deferred setup

It becomes very complicated to allow tests to override the return value, but still be able to wait for the method invocation. You cannot use thenReturn on that method any more.

@johanblumenberg
Copy link
Contributor Author

We cant with this implementation do something like verify(mockedFoo.getBar()).timeout(...).twice()

The problem is the return type. timeout() must return a promise, but all the other checks returns a MethodStubVerificator. Even if the timeout() call would return a MethodStubVerificator & Promise type, the promise would be lost when adding .twice().

Besides, it looks like all methods of MethodStubVerificator returns void?

@johanblumenberg
Copy link
Contributor Author

ping

@johanblumenberg
Copy link
Contributor Author

johanblumenberg commented Nov 29, 2018

Another possible syntax could be something like:

await timeout(mockedFoo.getBar(), 1000);

The downside would be that the syntax for verifying a call has been done synchronously or asynchronously would be very different.

@johanblumenberg
Copy link
Contributor Author

Pushed another commit updating the readme

@ChristianLutz
Copy link

I would love seeing this feature added. I see the point of not using the verify syntax. So going with the suggested timeout would be fine. But I also have another idea. Why not

await for(mockedFoo.getBar()).max(1000);

@NagRock
Copy link
Owner

NagRock commented Jul 19, 2019

Maybe similar API as Java mockito? verify(mockedFoo.getBar(), timeout(1000)).twice?

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

Successfully merging this pull request may close these issues.

None yet

4 participants