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

New matcher: .toHaveBeenCalledTimely(array) #112

Open
acostalima opened this issue Jan 23, 2018 · 11 comments
Open

New matcher: .toHaveBeenCalledTimely(array) #112

acostalima opened this issue Jan 23, 2018 · 11 comments

Comments

@acostalima
Copy link

acostalima commented Jan 23, 2018

Feature Request

Follow-up of: jestjs/jest#5363

.toHaveBeenCalledTimely accepts an array of temporal differences in milliseconds which specify the maximum time in between invocations to a mock. The matcher fails when at least one of the specified timing constraints is not fulfilled.

Example:

it('should retry 2 times with exponential back-off with a factor of 2', async () => {
  jest.doMock('foo-api', jest.fn(() => fooApiMocks.helloSuccessAsync()));
  const fooApi = require('foo-api');  

  myModuleWhichDependsOnFooApi.execute(); 

  expect(fooApi.hello).toHaveBeenCalledTimely([  
      2000, // 1st retry - time between 1st and 2nd call to fooApi.hello()
      4000 // 2nd retry - time between 2nd and 3rd call to fooApi.hello()
    ]);
});

Following the reasoning outlined here and issue reported in #98, would the above be possible before #98 is addressed and resolved?

If possible, I'd very much like to work on a PR for this. 😄
Thanks!

@mattphillips
Copy link
Member

Hey @acostalima #98 has now been published in v0.6.0.

This matcher is possible to make now, like #98 to work the mocks will need to be asynchronous to be ran in different contexts to generate different timestamps. That is something that will happen in user land. So we can just assume the test author is doing this and perform the comparisons on the timestamps 😄

If possible, I'd very much like to work on a PR for this. 😄

I'd be more than happy for you to send a PR for this, if you need any help just let me know 👍 as a place to start take a look at 966db3c

@acostalima
Copy link
Author

acostalima commented Feb 9, 2018

@mattphillips Great! I'll work on this ASAP.

Concerning the implementation, the behavior I'm actually looking for, at least in my specific use case, it to ensure the mock is called only after the specified milliseconds have elapsed. So, following the example I presented above:

  • The 1st retry should only be called after 2000 milliseconds have elapsed since the first call
  • The 2nd retry should only be called after 4000 milliseconds have elapsed since the 1st retry

Sounds good?

@benjaminkay93
Copy link
Contributor

hey @acostalima, did you manage to do this? if not I am happy enough to raise a PR for this as it would be helpful for myself anyways.

@acostalima
Copy link
Author

@benjaminkay93 not yet, but I will have some time tomorrow. Thanks for the reminder too! 🙏

@acostalima
Copy link
Author

acostalima commented Jun 30, 2018

I just learned Jest does not store timestamps of mock's invocations anymore 😞(jestjs/jest#5867). Without such information, I don't think we can implement this matcher anymore... do we?

@benjaminkay93
Copy link
Contributor

Ahh I hadn't seen this 😢 it explains the changes to toHaveBeenCalledBefore that have been merged in but not published here.

It might be worth asking for this to be revisited to reinstate the timestamps to enable this, I can see a legitimate use case for a matcher such as this. Without that, this wouldn't be possible from what I know of the .mock information. If we had both we could maintain support for the toHaveBeenCalledBefore matcher in jest 22 as well as jest 23 in the next release of jest-extended.

@acostalima
Copy link
Author

acostalima commented Jul 1, 2018

I guess we can ask. However, .timestamps was explicitly removed in favor of .invocationCallOrder (jestjs/jest#4402, jestjs/jest#5867) due to precision issues, but @brigonzalez was headed towards a possible solution (jestjs/jest#4402 (comment)).

@brigonzalez
Copy link

brigonzalez commented Jul 1, 2018

You can reinstate timestamps and use node’s process.hrtime() to calculate milliseconds between another process.hrtime() call. Although, not sure how process.hrtime() really looks as a “timestamp” since it’s using an arbitrary process to calculate a time difference.
Looks like convert-hrtime handles that conversion to milliseconds nicely.

@acostalima
Copy link
Author

acostalima commented Jul 1, 2018

Ok. I'll start a discussion in Jest's repo to get feedback. If we are good to go, let me know if you'd like to do the honors, @brigonzalez, and open the PR to reinstate the record of timestamps. Otherwise, I can try to do it myself. 😄

@brigonzalez
Copy link

Sure thing! Let me know if we’ve got the green light, or shoot me a link to the discussion!

@acostalima
Copy link
Author

@brigonzalez jestjs/jest#6672

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

No branches or pull requests

4 participants