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

Filter API pre-filter setup hook. #8142

Merged
merged 11 commits into from
Mar 19, 2019

Conversation

scotthovestadt
Copy link
Contributor

Summary

Filters may need to load data asynchronously to do filtering. For example, you may want to load a list of disabled tests from an endpoint so that they can be disabled.

Currently, it's only possible to do that when the filter is called. Unfortunately, this means that any required asynchronous operation is not parallelized with work that Jest does to build the initial list of tests.

This PR resolves that by introducing an optional setup hook that can be used to kick off an async operation early.

Open to any feedback on the API design. My goals were:

  • No breaking change
  • Simple as possible

Test plan

  • All existing tests pass.
  • Added new test for the setup hook.
  • Tested manually with real-world filter.

@codecov-io
Copy link

Codecov Report

Merging #8142 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8142   +/-   ##
=======================================
  Coverage   62.44%   62.44%           
=======================================
  Files         264      264           
  Lines       10371    10371           
  Branches     2515     2515           
=======================================
  Hits         6476     6476           
  Misses       3318     3318           
  Partials      577      577

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 76782b8...1bd8311. Read the comment docs.

@SimenB SimenB requested review from mjesun and thymikee March 18, 2019 07:53
@SimenB
Copy link
Member

SimenB commented Mar 18, 2019

No comment on the feature in general (I have to think about it :)), but

Filters may need to load data asynchronously to do filtering.

Then you should await the setup function call, to avoid a race condition.

@scotthovestadt
Copy link
Contributor Author

Happy to do await, but it means that the Promise would have to be passed down several layers to where the filter is actually called and then awaited there. You don’t want to wait for it until you actually need to call filter.

The alternative, which I’ve done here initially, is to assume that the filter implementation is aware of any Promise created by setup and can await it when the filter is actually called.

Let me know what you think is best.

@scotthovestadt
Copy link
Contributor Author

@SimenB Setup is now resolved before filter is called, slightly complicating the implementation here but making it easier to manage in the filter implementation. Let me know if you have any thoughts.

@@ -299,6 +300,9 @@ export default class SearchSource {
const tests = searchResult.tests;

const filter = require(filterPath);
if (filterSetupPromise) {
await filterSetupPromise;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, why do we pass the filter promise instead of taking it right from filter and just awaiting it? I get that it may be slightly faster (but really, is it actually measurable difference?) but it couples searchsource to cli too much (filter is actually required in 2 places, which is bad).

For the record, we recently got rid of changedFilesPromise for similar reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Promise was called earlier and passed down. It gets called before Haste Map generation, which takes several seconds. In FB's case, which is what I measured, if you have an HTTP call that takes longer than 200ms , anything on top of the 200ms is cut off. It's a measurable difference.

Any suggestions to avoid coupling?

Copy link
Contributor Author

@scotthovestadt scotthovestadt Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify-- ALL time spent waiting on an HTTP request is cut off, because before it wasn't parallelized against anything, so it can be a significant difference.

The reason there's 200ms-ish of overlap is because, depending on the size of the response, the HTTP request still needs to be parsed, and the haste map generation can CPU block up to that point, so the response doesn't get parsed early.

In my tests, an HTTP call of 1.2s~ had a solid second cut off due to the change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I didn't notice it's invoked before building haste map. I don't have any clever ideas around how to avoid coupling. At least it's inside the same module (jest-core).
Maybe we could just pass whole required filter module (with initialized setup call) and then await it when necessary? This would avoid requiring filter twice. If that seems cleaner, feel free to do it, but if not, I'm good with accepting this in current shape

Copy link
Contributor Author

@scotthovestadt scotthovestadt Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can get behind that. If we're passing something anyway, might as well pass the whole filter with the setup bootstrap already done. Something like (example simplified):

const setupPromise = filter.setup();
const filterToPass = async (testPaths: Array<string>) => {
    await setupPromise;
    return filter(testPaths);
};

...

// Later:
filterToPass(testPaths);

I think it's a good idea, working on it now. Thanks for the feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, makes sense this way, thanks as well!

hasteMapInstances,
undefined,
undefined,
filter,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB we need to convert this to an object finally :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree... I'm happy to handle that in a second PR if we want to do it now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! looks like this won't break public API for jest-core so happy to land that before next major

const rawFilter = require(globalConfig.filter);
let filterSetupPromise: Promise<void> | undefined;
if (rawFilter.setup) {
filterSetupPromise = rawFilter.setup();
Copy link
Member

@SimenB SimenB Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if this promise rejects before await filter in SearchSource? Will that bubble up as unhandled since no rejection handler has been attached?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add a .catch here and store the rejection in a variable and throw it manually? not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested and the filter setup hook being thrown is caught in the same place that the filter itself is called and has all the same behavior as if the filter failed. Which, currently, means the filter is ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

** I meant, not that the filter is ignored, but 0 results are returned.

If/when we add handling for the filter being thrown, the setup hook will be lumped in with it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding tests for both filter and setup filter throwing so we at least know the behavior, although when it happens Jest won't run tests (and that's expected).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for feedback; I did find a way to improve the behavior and I wrote tests for both filter and setup filter error code paths.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great!

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing changelog and I left an inline question. Other than that I think this turned out really good!

@scotthovestadt
Copy link
Contributor Author

@SimenB Changelog added and feedback resolved with additional tests to clarify behavior.

@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.

5 participants