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
@@ -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
@@ -21,6 +21,7 @@ import {
TestPathCases,
TestPathCasesWithPathPattern,
TestPathCaseWithPathPatternStats,
Filter,
} from './types';

export type SearchResult = {
@@ -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;
@@ -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(
43 changes: 41 additions & 2 deletions packages/jest-core/src/cli/index.ts
Original file line number Diff line number Diff line change
@@ -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';
@@ -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,
@@ -159,13 +177,15 @@ const _run = async (
globalConfig,
outputStream,
hasteMapInstances,
filter,
)
: await runWithoutWatch(
globalConfig,
contexts,
outputStream,
onComplete,
changedFilesPromise,
filter,
);
};

@@ -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 (
@@ -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) {
@@ -209,6 +247,7 @@ const runWithoutWatch = async (
outputStream,
startRun,
testWatcher: new TestWatcher({isWatchMode: false}),
filter,
});
};
return startRun();
8 changes: 6 additions & 2 deletions packages/jest-core/src/runJest.ts
Original file line number Diff line number Diff line change
@@ -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(
@@ -129,6 +130,7 @@ export default (async function runJest({
changedFilesPromise,
onComplete,
failedTestsCache,
filter,
}: {
globalConfig: Config.GlobalConfig;
contexts: Array<Context>;
@@ -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> = [];
@@ -168,6 +171,7 @@ export default (async function runJest({
outputStream,
changedFilesPromise && (await changedFilesPromise),
jestHooks,
filter,
);
allTests = allTests.concat(matches.tests);

11 changes: 11 additions & 0 deletions packages/jest-core/src/types.ts
Original file line number Diff line number Diff line change
@@ -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
@@ -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;
@@ -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.
@@ -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