Filter API pre-filter setup hook. #8142

scotthovestadt · 2019-03-18T04:40:01Z

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 · 2019-03-18T05:16:18Z

Codecov Report

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

@@           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 · 2019-03-18T07:54:43Z

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 · 2019-03-18T08:01:08Z

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 · 2019-03-18T08:16:06Z

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

thymikee · 2019-03-18T08:29:47Z

packages/jest-core/src/SearchSource.ts

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

      const filter = require(filterPath);
+      if (filterSetupPromise) {
+        await filterSetupPromise;


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.

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?

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.

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

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.

Yea, makes sense this way, thanks as well!

thymikee · 2019-03-18T16:08:10Z

packages/jest-core/src/cli/index.ts

+        hasteMapInstances,
+        undefined,
+        undefined,
+        filter,


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

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

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

SimenB · 2019-03-19T11:18:29Z

packages/jest-core/src/cli/index.ts

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


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

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

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.

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

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

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.

Sounds great!

SimenB

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

scotthovestadt · 2019-03-19T16:03:29Z

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

github-actions · 2021-05-11T21:07:24Z

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.

Filter API pre-filter setup hook.

1bd8311

facebook-github-bot added the cla signed label Mar 18, 2019

SimenB requested review from mjesun and thymikee March 18, 2019 07:53

Ensure filter API setup Promise resolves before filtering.

0dfb5c5

thymikee reviewed Mar 18, 2019

View reviewed changes

Pass through wrapped filter to avoid including it twice.

1464230

thymikee approved these changes Mar 18, 2019

View reviewed changes

scotthovestadt added 2 commits March 18, 2019 09:13

Resolve lint error.

784b77b

Fix unit test for Node v6.

f052897

SimenB reviewed Mar 19, 2019

View reviewed changes

SimenB requested changes Mar 19, 2019

View reviewed changes

scotthovestadt added 4 commits March 19, 2019 08:39

Update changelog.

6d89fa3

Add tests and improve behavior for errors in filter and setup filter.

dd507f6

Merge branch 'master' into filter-setup

7fa3230

Resolve lint error.

8b34e39

Update CHANGELOG.md

fe25250

SimenB approved these changes Mar 19, 2019

View reviewed changes

Fix broken filter test in Node 6.

4d392ce

scotthovestadt merged commit 4a57c24 into jestjs:master Mar 19, 2019

snyk-bot mentioned this pull request Dec 15, 2019

[Snyk] Upgrade babel-jest from 24.0.0 to 24.9.0 jakeherp/burger-builder#14

Open

This was referenced Mar 21, 2020

[Snyk] Upgrade: jest, jest-cli lwojcik/sc2pte-panel-frontend#88

Closed

[Snyk] Upgrade jest from 24.0.0 to 24.9.0 DavidKindler/burger-builder#47

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter API pre-filter setup hook. #8142

Filter API pre-filter setup hook. #8142

scotthovestadt commented Mar 18, 2019

codecov-io commented Mar 18, 2019

SimenB commented Mar 18, 2019

scotthovestadt commented Mar 18, 2019

scotthovestadt commented Mar 18, 2019

thymikee Mar 18, 2019

scotthovestadt Mar 18, 2019

scotthovestadt Mar 18, 2019 •

edited

thymikee Mar 18, 2019

scotthovestadt Mar 18, 2019 •

edited

thymikee Mar 18, 2019

thymikee Mar 18, 2019

scotthovestadt Mar 18, 2019

thymikee Mar 18, 2019

SimenB Mar 19, 2019 •

edited

SimenB Mar 19, 2019

scotthovestadt Mar 19, 2019

scotthovestadt Mar 19, 2019

scotthovestadt Mar 19, 2019

scotthovestadt Mar 19, 2019

SimenB Mar 19, 2019

SimenB left a comment

scotthovestadt commented Mar 19, 2019

github-actions bot commented May 11, 2021

Filter API pre-filter setup hook. #8142

Filter API pre-filter setup hook. #8142

Conversation

scotthovestadt commented Mar 18, 2019

Summary

Test plan

codecov-io commented Mar 18, 2019

Codecov Report

SimenB commented Mar 18, 2019

scotthovestadt commented Mar 18, 2019

scotthovestadt commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotthovestadt Mar 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotthovestadt Mar 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Mar 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

scotthovestadt commented Mar 19, 2019

github-actions bot commented May 11, 2021

scotthovestadt Mar 18, 2019 •

edited

scotthovestadt Mar 18, 2019 •

edited

SimenB Mar 19, 2019 •

edited