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
7 changes: 7 additions & 0 deletions e2e/__tests__/filter.test.ts
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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';
};
14 changes: 4 additions & 10 deletions packages/jest-core/src/SearchSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
TestPathCases,
TestPathCasesWithPathPattern,
TestPathCaseWithPathPatternStats,
Filter,
} from './types';

export type SearchResult = {
Expand All @@ -41,11 +42,6 @@ export type TestSelectionConfig = {
watch?: boolean;
};

type FilterResult = {
test: string;
message: string;
};

const globsToMatcher = (globs?: Array<Config.Glob> | null) => {
if (globs == null || globs.length === 0) {
return () => true;
Expand Down Expand Up @@ -290,18 +286,16 @@ export default class SearchSource {
async getTestPaths(
globalConfig: Config.GlobalConfig,
changedFiles: ChangedFiles | undefined,
filter?: Filter,
): Promise<SearchResult> {
const searchResult = this._getTestPaths(globalConfig, changedFiles);

const filterPath = globalConfig.filter;

if (filterPath && !globalConfig.skipFilter) {
if (filter) {
const tests = searchResult.tests;

const filter = require(filterPath);
const filterResult: {filtered: Array<FilterResult>} = await filter(
tests.map(test => test.path),
);
const filterResult = await filter(tests.map(test => test.path));

if (!Array.isArray(filterResult.filtered)) {
throw new Error(
Expand Down
43 changes: 41 additions & 2 deletions packages/jest-core/src/cli/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import HasteMap from 'jest-haste-map';
import chalk from 'chalk';
import rimraf from 'rimraf';
import exit from 'exit';
import {Filter} from '../types';
import createContext from '../lib/create_context';
import getChangedFilesPromise from '../getChangedFilesPromise';
import {formatHandleErrors} from '../collectHandles';
Expand Down Expand Up @@ -145,6 +146,23 @@ 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 filter: Filter | undefined;
if (globalConfig.filter && !globalConfig.skipFilter) {
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!

}
filter = async (testPaths: Array<string>) => {
if (filterSetupPromise) {
await filterSetupPromise;
}
return rawFilter(testPaths);
};
}

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

Expand All @@ -176,17 +196,34 @@ const runWatch = async (
globalConfig: Config.GlobalConfig,
outputStream: NodeJS.WriteStream,
hasteMapInstances: Array<HasteMap>,
filter?: Filter,
) => {
if (hasDeprecationWarnings) {
try {
await handleDeprecationWarnings(outputStream, process.stdin);
return watch(globalConfig, contexts, outputStream, hasteMapInstances);
return watch(
globalConfig,
contexts,
outputStream,
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

);
} catch (e) {
exit(0);
}
}

return watch(globalConfig, contexts, outputStream, hasteMapInstances);
return watch(
globalConfig,
contexts,
outputStream,
hasteMapInstances,
undefined,
undefined,
filter,
);
};

const runWithoutWatch = async (
Expand All @@ -195,6 +232,7 @@ const runWithoutWatch = async (
outputStream: NodeJS.WritableStream,
onComplete: OnCompleteCallback,
changedFilesPromise?: ChangedFilesPromise,
filter?: Filter,
) => {
const startRun = async (): Promise<void | null> => {
if (!globalConfig.listTests) {
Expand All @@ -209,6 +247,7 @@ const runWithoutWatch = async (
outputStream,
startRun,
testWatcher: new TestWatcher({isWatchMode: false}),
filter,
});
};
return startRun();
Expand Down
8 changes: 6 additions & 2 deletions packages/jest-core/src/runJest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,18 @@ import TestSequencer from './TestSequencer';
import FailedTestsCache from './FailedTestsCache';
import collectNodeHandles from './collectHandles';
import TestWatcher from './TestWatcher';
import {TestRunData} from './types';
import {TestRunData, Filter} from './types';

const getTestPaths = async (
globalConfig: Config.GlobalConfig,
context: Context,
outputStream: NodeJS.WritableStream,
changedFiles: ChangedFiles | undefined,
jestHooks: JestHookEmitter,
filter?: Filter,
) => {
const source = new SearchSource(context);
const data = await source.getTestPaths(globalConfig, changedFiles);
const data = await source.getTestPaths(globalConfig, changedFiles, filter);

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

Expand Down
11 changes: 11 additions & 0 deletions packages/jest-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,14 @@ export type TestPathCases = {
export type TestPathCasesWithPathPattern = TestPathCases & {
testPathPattern: (path: Config.Path) => boolean;
};

export type FilterResult = {
test: string;
message: string;
};

export type Filter = (
testPaths: Array<string>,
) => Promise<{
filtered: Array<FilterResult>;
}>;
3 changes: 3 additions & 0 deletions packages/jest-core/src/watch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
filterInteractivePlugins,
} from './lib/watch_plugins_helpers';
import activeFilters from './lib/active_filters_message';
import {Filter} from './types';

type ReservedInfo = {
forbiddenOverwriteMessage?: string;
Expand Down Expand Up @@ -83,6 +84,7 @@ export default function watch(
hasteMapInstances: Array<HasteMap>,
stdin: NodeJS.ReadStream = process.stdin,
hooks: JestHook = new JestHook(),
filter?: Filter,
): Promise<void> {
// `globalConfig` will be constantly updated and reassigned as a result of
// watch mode interactions.
Expand Down Expand Up @@ -293,6 +295,7 @@ export default function watch(
outputStream,
startRun,
testWatcher,
filter,
}).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