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.

Sign up for GitHub

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

Jump to bottom

Filter API pre-filter setup hook. #8142

Merged
merged 11 commits into from Mar 19, 2019
Merged

Filter API pre-filter setup hook. #8142

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1bd8311
Filter API pre-filter setup hook.
scotthovestadt Mar 18, 2019
0dfb5c5
Ensure filter API setup Promise resolves before filtering.
scotthovestadt Mar 18, 2019
1464230
Pass through wrapped filter to avoid including it twice.
scotthovestadt Mar 18, 2019
784b77b
Resolve lint error.
scotthovestadt Mar 18, 2019
f052897
Fix unit test for Node v6.
scotthovestadt Mar 18, 2019
6d89fa3
Update changelog.
scotthovestadt Mar 19, 2019
dd507f6
Add tests and improve behavior for errors in filter and setup filter.
scotthovestadt Mar 19, 2019
7fa3230
Merge branch 'master' into filter-setup
scotthovestadt Mar 19, 2019
8b34e39
Resolve lint error.
scotthovestadt Mar 19, 2019
fe25250
Update CHANGELOG.md
SimenB Mar 19, 2019
4d392ce
Fix broken filter test in Node 6.
scotthovestadt Mar 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions e2e/__tests__/filter.test.ts
View file
Open in desktop
Expand Up @@ -43,4 +43,11 @@ describe('Dynamic test filtering', () => {
expect(result.stderr).toContain('did not return a valid test list');
expect(result.stderr).toContain('my-clowny-filter');
});

it('will call setup on filter before filtering', () => {
const result = runJest('filter', ['--filter=<rootDir>/my-setup-filter.js']);

expect(result.status).toBe(0);
expect(result.stderr).toContain('1 total');
});
});
22 changes: 22 additions & 0 deletions e2e/filter/my-setup-filter.js
View file
Open in desktop
@@ -0,0 +1,22 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.

'use strict';

const setupData = {
filterText: 'this will return no tests',
};

module.exports = function(tests) {
return {
filtered: tests
.filter(t => t.indexOf(setupData.filterText) !== -1)
.map(test => ({test})),
};
};

module.exports.setup = async function() {
await new Promise(resolve => {
setTimeout(() => resolve(), 1000);
});
setupData.filterText = 'foo';
};
4 changes: 4 additions & 0 deletions packages/jest-core/src/SearchSource.ts
View file
Open in desktop
Expand Up @@ -290,6 +290,7 @@ export default class SearchSource {
async getTestPaths(
globalConfig: Config.GlobalConfig,
changedFiles: ChangedFiles | undefined,
filterSetupPromise?: Promise<void>,
): Promise<SearchResult> {
const searchResult = this._getTestPaths(globalConfig, changedFiles);

Expand All @@ -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

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

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!

}
const filterResult: {filtered: Array<FilterResult>} = await filter(
tests.map(test => test.path),
);
Expand Down
15 changes: 15 additions & 0 deletions packages/jest-core/src/cli/index.ts
View file
Open in desktop
Expand Up @@ -145,6 +145,16 @@ const _run = async (
// as soon as possible, so by the time we need the result it's already there.
const changedFilesPromise = getChangedFilesPromise(globalConfig, configs);

// Filter may need to do an HTTP call or something similar to setup.
// We will not wait on an async response from this before using the filter.
let filterSetupPromise: Promise<void> | undefined;
if (globalConfig.filter && !globalConfig.skipFilter) {
const filter = require(globalConfig.filter);
if (filter.setup) {
filterSetupPromise = filter.setup();
}
}

const {contexts, hasteMapInstances} = await buildContextsAndHasteMaps(
configs,
globalConfig,
Expand All @@ -159,13 +169,15 @@ const _run = async (
globalConfig,
outputStream,
hasteMapInstances,
filterSetupPromise,
)
: await runWithoutWatch(
globalConfig,
contexts,
outputStream,
onComplete,
changedFilesPromise,
filterSetupPromise,
);
};

Expand All @@ -176,6 +188,7 @@ const runWatch = async (
globalConfig: Config.GlobalConfig,
outputStream: NodeJS.WriteStream,
hasteMapInstances: Array<HasteMap>,
filterSetupPromise?: Promise<void>,
) => {
if (hasDeprecationWarnings) {
try {
Expand All @@ -195,6 +208,7 @@ const runWithoutWatch = async (
outputStream: NodeJS.WritableStream,
onComplete: OnCompleteCallback,
changedFilesPromise?: ChangedFilesPromise,
filterSetupPromise?: Promise<void>,
) => {
const startRun = async (): Promise<void | null> => {
if (!globalConfig.listTests) {
Expand All @@ -209,6 +223,7 @@ const runWithoutWatch = async (
outputStream,
startRun,
testWatcher: new TestWatcher({isWatchMode: false}),
filterSetupPromise,
});
};
return startRun();
Expand Down
10 changes: 9 additions & 1 deletion packages/jest-core/src/runJest.ts
View file
Open in desktop
Expand Up @@ -37,9 +37,14 @@ const getTestPaths = async (
outputStream: NodeJS.WritableStream,
changedFiles: ChangedFiles | undefined,
jestHooks: JestHookEmitter,
filterSetupPromise?: Promise<void>,
) => {
const source = new SearchSource(context);
const data = await source.getTestPaths(globalConfig, changedFiles);
const data = await source.getTestPaths(
globalConfig,
changedFiles,
filterSetupPromise,
);

if (!data.tests.length && globalConfig.onlyChanged && data.noSCM) {
new CustomConsole(outputStream, outputStream).log(
Expand Down Expand Up @@ -129,6 +134,7 @@ export default (async function runJest({
changedFilesPromise,
onComplete,
failedTestsCache,
filterSetupPromise,
}: {
globalConfig: Config.GlobalConfig;
contexts: Array<Context>;
Expand All @@ -139,6 +145,7 @@ export default (async function runJest({
changedFilesPromise?: ChangedFilesPromise;
onComplete: (testResults: AggregatedResult) => void;
failedTestsCache?: FailedTestsCache;
filterSetupPromise?: Promise<void>;
}) {
const sequencer = new TestSequencer();
let allTests: Array<Test> = [];
Expand Down Expand Up @@ -168,6 +175,7 @@ export default (async function runJest({
outputStream,
changedFilesPromise && (await changedFilesPromise),
jestHooks,
filterSetupPromise,
);
allTests = allTests.concat(matches.tests);

Expand Down
2 changes: 2 additions & 0 deletions packages/jest-core/src/watch.ts
View file
Open in desktop
Expand Up @@ -83,6 +83,7 @@ export default function watch(
hasteMapInstances: Array<HasteMap>,
stdin: NodeJS.ReadStream = process.stdin,
hooks: JestHook = new JestHook(),
filterSetupPromise?: Promise<void>,
): Promise<void> {
// `globalConfig` will be constantly updated and reassigned as a result of
// watch mode interactions.
Expand Down Expand Up @@ -293,6 +294,7 @@ export default function watch(
outputStream,
startRun,
testWatcher,
filterSetupPromise,
}).catch(error =>
// Errors thrown inside `runJest`, e.g. by resolvers, are caught here for
// continuous watch mode execution. We need to reprint them to the
Expand Down