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

Support async cacheResults in custom testSequencer #8833

Closed
wants to merge 1 commit into from

Conversation

yairhaimo
Copy link

Summary

PR #8642 makes it so that the sort method of a testSequencer can be async in order to support use cases where you fetch test data from an async endpoint. This PR aims to complement it by adding the possibility to store the cache in an async fashion as well.

The entire PR comes down to adding the await keyword to the cacheResults method call.

Test plan

I am opening this PR as a draft PR because I want to consult with the community on how I should test this feature.
At first I added more tests to the customTestSequencers.test.ts suite. Those tests are e2e tests that spawn a process which will wait for the async operation to end before exiting, making the async process work eventually, thus making the test useless.

Instead, I thought about adding unit tests but currently there are no tests for customSequencers other than the aforementioned e2e. The unit tests for the test_sequencer.test.js test the default implementation and I think adding tests for a custom one will make a mess of it all.

Where and how should I test this?
Thanks

@SimenB
Copy link
Member

SimenB commented Nov 9, 2019

I'm not sure how to best test this... Since it's fire and forget it's not clean indeed. What happens if you return a rejected promise (which was previously ignored) - that should propagate properly now which it didn't before. Maybe that's enough?

@jeysal
Copy link
Contributor

jeysal commented Nov 10, 2019

This'll look a bit weird, but if you have a "only works if a cache is there" sequencer with a cacheResults impl of

new Promise(resolve => {
  setTimeout(() => {
    writeFileSomewhere();
    resolve();
  }), 1000).unref()
})

and a sort impl of

if (fileWrittenIsNotThere()) throw new Error();

and runJest twice and assert that the second run passes, that should check that cacheResults was awaited properly 😄

@SimenB
Copy link
Member

SimenB commented Apr 23, 2020

@yairhaimo ping 🙂

@yairhaimo
Copy link
Author

Hi. Closing this for now as I am working on other stuff right now. I'll reopen if its relevant again. Thanks for all your work!

@yairhaimo yairhaimo closed this Apr 23, 2020
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants