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

changed files eager loading #3979

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/jest-changed-files/src/git.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/

import type {Path} from 'types/Config';
import type {Options, SCMAdapter} from '../types';
import type {Options, SCMAdapter} from 'types/ChangedFiles';

import path from 'path';
import childProcess from 'child_process';
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-changed-files/src/hg.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/

import type {Path} from 'types/Config';
import type {Options, SCMAdapter} from '../types';
import type {Options, SCMAdapter} from 'types/ChangedFiles';

import path from 'path';
import childProcess from 'child_process';
Expand Down
5 changes: 3 additions & 2 deletions packages/jest-changed-files/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/

import type {Path} from 'types/Config';
import type {Options, Repos} from '../types';
import type {ChangedFilesPromise, Options, Repos} from 'types/ChangedFiles';

import git from './git';
import hg from './hg';
Expand All @@ -25,8 +25,9 @@ const findHgRoot = dir => mutex(() => hg.getRoot(dir));
const getChangedFilesForRoots = async (
roots: Array<Path>,
options: Options,
): Promise<{changedFiles: Set<Path>, repos: Repos}> => {
): ChangedFilesPromise => {
const repos = await findRepos(roots);

const gitPromises = Array.from(repos.git).map(repo =>
git.findChangedFiles(repo, options),
);
Expand Down
3 changes: 3 additions & 0 deletions packages/jest-cli/src/__tests__/watch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ describe('Watch mode flows', () => {
pipe,
new TestWatcher({isWatchMode: true}),
expect.any(Function),
undefined,
expect.any(Function),
);
});
Expand All @@ -82,6 +83,7 @@ describe('Watch mode flows', () => {
pipe,
new TestWatcher({isWatchMode: true}),
expect.any(Function),
undefined,
expect.any(Function),
);
});
Expand All @@ -95,6 +97,7 @@ describe('Watch mode flows', () => {
pipe,
new TestWatcher({isWatchMode: true}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider changing the signature of run_jest to an object of all this options instead of positional arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's on my plan! bun not in this PR :)
I already started refactoring the CLI flow

expect.any(Function),
undefined,
expect.any(Function),
);
expect(pipe.write.mock.calls.reverse()[0]).toMatchSnapshot();
Expand Down
17 changes: 16 additions & 1 deletion packages/jest-cli/src/cli/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {version as VERSION} from '../../package.json';
import args from './args';
import chalk from 'chalk';
import createContext from '../lib/create_context';
import getChangedFilesPromise from '../get_changed_files_promise';
import getJest from './get_jest';
import getMaxWorkers from '../lib/get_max_workers';
import handleDeprecationWarnings from '../lib/handle_deprecation_warnings';
Expand Down Expand Up @@ -251,6 +252,9 @@ const _run = async (
argv,
onComplete,
) => {
// Queries to hg/git can take a while, so we need to start the process
// as soon as possible, so by the time we need the result it's already there.
const changedFilesPromise = getChangedFilesPromise(argv, configs);
const {contexts, hasteMapInstances} = await _buildContextsAndHasteMaps(
configs,
globalConfig,
Expand All @@ -267,8 +271,16 @@ const _run = async (
globalConfig,
outputStream,
hasteMapInstances,
changedFilesPromise,
)
: _runWithoutWatch(globalConfig, contexts, argv, outputStream, onComplete);
: _runWithoutWatch(
globalConfig,
contexts,
argv,
outputStream,
onComplete,
changedFilesPromise,
);
};

const _runWatch = async (
Expand All @@ -279,6 +291,7 @@ const _runWatch = async (
globalConfig,
outputStream,
hasteMapInstances,
changedFilesPromise,
) => {
if (hasDeprecationWarnings) {
try {
Expand All @@ -304,6 +317,7 @@ const _runWithoutWatch = async (
argv,
outputStream,
onComplete,
changedFilesPromise,
) => {
const startRun = () => {
if (!argv.listTests) {
Expand All @@ -316,6 +330,7 @@ const _runWithoutWatch = async (
outputStream,
new TestWatcher({isWatchMode: false}),
startRun,
changedFilesPromise,
onComplete,
);
};
Expand Down
31 changes: 31 additions & 0 deletions packages/jest-cli/src/get_changed_files_promise.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Copyright (c) 2014, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/

import type {Argv} from 'types/Argv';
import type {ProjectConfig} from 'types/Config';
import type {ChangedFilesPromise} from 'types/ChangedFiles';
import {getChangedFilesForRoots} from 'jest-changed-files';

module.exports = (
argv: Argv,
configs: Array<ProjectConfig>,
): ?ChangedFilesPromise => {
if (argv.onlyChanged) {
const allRootsForAllProjects = configs.reduce(
(roots, config) => roots.concat(config.roots || []),
[],
);
return getChangedFilesForRoots(allRootsForAllProjects, {
lastCommit: argv.lastCommit,
});
}

return undefined;
};
1 change: 0 additions & 1 deletion packages/jest-cli/src/lib/get_test_path_pattern.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ module.exports = (argv: Argv): TestSelectionConfig => {
if (argv.onlyChanged) {
return {
input: '',
lastCommit: argv.lastCommit,
onlyChanged: true,
watch: argv.watch,
};
Expand Down
17 changes: 12 additions & 5 deletions packages/jest-cli/src/run_jest.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import type {Argv} from 'types/Argv';
import type {Context} from 'types/Context';
import type {ChangedFilesPromise} from 'types/ChangedFiles';
import type {GlobalConfig} from 'types/Config';
import type {TestSelectionConfig} from './search_source';
import type {AggregatedResult} from 'types/TestResult';
Expand Down Expand Up @@ -102,19 +103,23 @@ const getNoTestsFoundMessage = (testRunData, pattern) => {
const getTestPaths = async (
globalConfig,
context,
pattern,
testSelectionConfig,
argv,
outputStream,
changedFilesPromise,
) => {
const source = new SearchSource(context);
let data = await source.getTestPaths(pattern);
let data = await source.getTestPaths(
testSelectionConfig,
changedFilesPromise,
);
if (!data.tests.length) {
if (pattern.onlyChanged && data.noSCM) {
if (testSelectionConfig.onlyChanged && data.noSCM) {
if (globalConfig.watch) {
// Run all the tests
updateArgv(argv, 'watchAll', {noSCM: true});
pattern = getTestPathPattern(argv);
data = await source.getTestPaths(pattern);
testSelectionConfig = getTestPathPattern(argv);
data = await source.getTestPaths(testSelectionConfig);
} else {
new Console(outputStream, outputStream).log(
'Jest can only find uncommitted changed files in a git or hg ' +
Expand Down Expand Up @@ -160,6 +165,7 @@ const runJest = async (
outputStream: stream$Writable | tty$WriteStream,
testWatcher: TestWatcher,
startRun: () => *,
changedFilesPromise: ?ChangedFilesPromise,
onComplete: (testResults: AggregatedResult) => any,
// We use this internaly at FB. Since we run multiple processes and most
// of them don't match any tests, we don't want to print 'no tests found'
Expand All @@ -184,6 +190,7 @@ const runJest = async (
testSelectionConfig,
argv,
outputStream,
changedFilesPromise,
);
allTests = allTests.concat(matches.tests);
return {context, matches};
Expand Down
29 changes: 12 additions & 17 deletions packages/jest-cli/src/search_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ import type {Context} from 'types/Context';
import type {Glob, Path} from 'types/Config';
import type {ResolveModuleConfig} from 'types/Resolve';
import type {Test} from 'types/TestRunner';
import type {ChangedFilesPromise} from 'types/ChangedFiles';

import path from 'path';
import micromatch from 'micromatch';
import DependencyResolver from 'jest-resolve-dependencies';
import {getChangedFilesForRoots} from 'jest-changed-files';
import {escapePathForRegex, replacePathSepForRegex} from 'jest-regex-util';

type SearchResult = {|
Expand All @@ -28,15 +28,9 @@ type SearchResult = {|

type StrOrRegExpPattern = RegExp | string;

type Options = {|
lastCommit?: boolean,
withAncestor?: boolean,
|};

export type TestSelectionConfig = {|
input?: string,
findRelatedTests?: boolean,
lastCommit?: boolean,
onlyChanged?: boolean,
paths?: Array<Path>,
shouldTreatInputAsPattern?: boolean,
Expand Down Expand Up @@ -183,27 +177,28 @@ class SearchSource {
return {tests: []};
}

async findChangedTests(options: Options): Promise<SearchResult> {
const {repos, changedFiles} = await getChangedFilesForRoots(
this._context.config.roots,
options,
);
async findTestRelatedToChangedFiles(
changedFilesPromise: ChangedFilesPromise,
) {
const {repos, changedFiles} = await changedFilesPromise;

// no SCM (git/hg/...) is found in any of the roots.
const noSCM = Object.keys(repos).every(scm => repos[scm].size === 0);

return noSCM
? {noSCM: true, tests: []}
: this.findRelatedTests(changedFiles);
}

getTestPaths(
async getTestPaths(
testSelectionConfig: TestSelectionConfig,
changedFilesPromise: ?ChangedFilesPromise,
): Promise<SearchResult> {
if (testSelectionConfig.onlyChanged) {
return this.findChangedTests({
lastCommit: testSelectionConfig.lastCommit,
});
if (!changedFilesPromise) {
throw new Error('This promise must be present when running with -o.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the error message to something like "'A Git or Mercurial repository must be present when running with -o."? So that it gives some info to the user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is pretty much an invariant and should never throw. I only put it in there, because the promise is generated early in the flow and argv is modified during the flow. So potentially we can cause inconsistent state (ran with -o, but promise is not passed).
This error here just for catching potential bugs early :)

}

return this.findTestRelatedToChangedFiles(changedFilesPromise);
} else if (
testSelectionConfig.findRelatedTests &&
testSelectionConfig.paths
Expand Down
4 changes: 4 additions & 0 deletions packages/jest-cli/src/watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {Context} from 'types/Context';

import ansiEscapes from 'ansi-escapes';
import chalk from 'chalk';
import getChangedFilesPromise from './get_changed_files_promise';
import {replacePathSepForRegex} from 'jest-regex-util';
import HasteMap from 'jest-haste-map';
import isCI from 'is-ci';
Expand Down Expand Up @@ -115,6 +116,8 @@ const watch = (
testPathPattern: argv.testPathPattern,
}),
);
const configs = contexts.map(context => context.config);
const changedFilesPromise = getChangedFilesPromise(argv, configs);
return runJest(
// $FlowFixMe
globalConfig,
Expand All @@ -123,6 +126,7 @@ const watch = (
pipe,
testWatcher,
startRun,
changedFilesPromise,
results => {
isRunning = false;
hasSnapshotFailure = !!results.snapshot.failure;
Expand Down
28 changes: 28 additions & 0 deletions types/ChangedFiles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/

import type {Path} from 'types/Config';

export type Options = {|
lastCommit?: boolean,
withAncestor?: boolean,
|};

export type ChangedFiles = Set<Path>;
export type Repos = {|git: Set<Path>, hg: Set<Path>|};
export type ChangedFilesPromise = Promise<{|
repos: Repos,
changedFiles: ChangedFiles,
|}>;

export type SCMAdapter = {|
findChangedFiles: (cwd: Path, options: Options) => Promise<Array<Path>>,
getRoot: (cwd: Path) => Promise<?Path>,
|};