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

perf(changed-files): limit git and hg commands to specified roots #6732

Merged
merged 5 commits into from
Jul 21, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
## master


### Performance

- `[jest-changed-files]` limit git and hg commands to specified roots ([#6732](https://github.com/facebook/jest/pull/6732))

### Fixes

- `[babel-jest]` Make `getCacheKey()` take into account `createTransformer` options ([#6699](https://github.com/facebook/jest/pull/6699))
Expand Down
49 changes: 49 additions & 0 deletions e2e/__tests__/jest_changed_files.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,27 @@ test('gets changed files for git', async () => {
).toEqual(['file5.txt']);
});

test('should only monitor root paths for git', async () => {
writeFiles(DIR, {
'file1.txt': 'file1',
'nested_dir/file2.txt': 'file2',
'nested_dir/second_nested_dir/file3.txt': 'file3',
});

run(`${GIT} init`, DIR);

const roots = ['nested_dir', 'nested_dir/second_nested_dir'].map(filename =>
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like nested_dir already includes nested_dir/second_nested_dir root

Copy link
Contributor Author

@booleanbetrayal booleanbetrayal Jul 20, 2018

Choose a reason for hiding this comment

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

Updated / removed redundant root!

path.resolve(DIR, filename),
);

const {changedFiles: files} = await getChangedFilesForRoots(roots, {});
expect(
Array.from(files)
.map(filePath => path.basename(filePath))
.sort(),
).toEqual(['file2.txt', 'file3.txt']);
});

test('gets changed files for hg', async () => {
if (process.env.CI) {
// Circle and Travis have very old version of hg (v2, and current
Expand Down Expand Up @@ -326,3 +347,31 @@ test('gets changed files for hg', async () => {
.sort(),
).toEqual(['file5.txt']);
});

test('should only monitor root paths for hg', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should avoid using should in test titles :)
use it('monitors only root paths for hg'...

if (process.env.CI) {
// Circle and Travis have very old version of hg (v2, and current
// version is v4.2) and its API changed since then and not compatible
// any more. Changing the SCM version on CIs is not trivial, so we'll just
// skip this test and run it only locally.
return;
}
writeFiles(DIR, {
'file1.txt': 'file1',
'nested_dir/file2.txt': 'file2',
'nested_dir/second_nested_dir/file3.txt': 'file3',
});

run(`${HG} init`, DIR);

const roots = ['nested_dir', 'nested_dir/second_nested_dir'].map(filename =>
path.resolve(DIR, filename),
);

const {changedFiles: files} = await getChangedFilesForRoots(roots, {});
expect(
Array.from(files)
.map(filePath => path.basename(filePath))
.sort(),
).toEqual(['file2.txt', 'file3.txt']);
});
21 changes: 16 additions & 5 deletions packages/jest-changed-files/src/git.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,33 +46,44 @@ const findChangedFilesUsingCommand = async (
const adapter: SCMAdapter = {
findChangedFiles: async (
cwd: string,
roots: Array<Path>,
options?: Options,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about moving roots to Options? This way we could keep it backwards compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the idea that we would keep roots as optional within Options but ensure that getChangedFilesForRoots always provides it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, ideally with the default behavior being the same as current one (pre this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed the fix. Let me know what you think. Thanks!

): Promise<Array<Path>> => {
const changedSince: ?string =
options && (options.withAncestor ? 'HEAD^' : options.changedSince);

if (options && options.lastCommit) {
return await findChangedFilesUsingCommand(
['show', '--name-only', '--pretty=%b', 'HEAD'],
['show', '--name-only', '--pretty=%b', 'HEAD'].concat(roots),
cwd,
);
} else if (changedSince) {
const committed = await findChangedFilesUsingCommand(
['log', '--name-only', '--pretty=%b', 'HEAD', `^${changedSince}`],
[
'log',
'--name-only',
'--pretty=%b',
'HEAD',
`^${changedSince}`,
].concat(roots),
cwd,
);
const staged = await findChangedFilesUsingCommand(
['diff', '--cached', '--name-only'],
['diff', '--cached', '--name-only'].concat(roots),
cwd,
);
const unstaged = await findChangedFilesUsingCommand(
['ls-files', '--other', '--modified', '--exclude-standard'],
['ls-files', '--other', '--modified', '--exclude-standard'].concat(
roots,
),
cwd,
);
return [...committed, ...staged, ...unstaged];
} else {
return await findChangedFilesUsingCommand(
['ls-files', '--other', '--modified', '--exclude-standard'],
['ls-files', '--other', '--modified', '--exclude-standard'].concat(
roots,
),
cwd,
);
}
Expand Down
6 changes: 4 additions & 2 deletions packages/jest-changed-files/src/hg.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,19 @@ const ANCESTORS = [
const adapter: SCMAdapter = {
findChangedFiles: async (
cwd: string,
roots: Array<Path>,
options: Options,
): Promise<Array<Path>> =>
new Promise((resolve, reject) => {
let args = ['status', '-amnu'];
const args = ['status', '-amnu'];
if (options && options.withAncestor) {
args.push('--rev', `ancestor(${ANCESTORS.join(', ')})`);
} else if (options && options.changedSince) {
args.push('--rev', `ancestor(., ${options.changedSince})`);
} else if (options && options.lastCommit === true) {
args = ['tip', '--template', '{files%"{file}\n"}'];
args.push('-A');
}
args.push(...roots);
const child = childProcess.spawn('hg', args, {cwd, env});
let stdout = '';
let stderr = '';
Expand Down
4 changes: 2 additions & 2 deletions packages/jest-changed-files/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ export const getChangedFilesForRoots = async (
const repos = await findRepos(roots);

const gitPromises = Array.from(repos.git).map(repo =>
git.findChangedFiles(repo, options),
git.findChangedFiles(repo, roots, options),
);

const hgPromises = Array.from(repos.hg).map(repo =>
hg.findChangedFiles(repo, options),
hg.findChangedFiles(repo, roots, options),
);

const changedFiles = (await Promise.all(
Expand Down
6 changes: 5 additions & 1 deletion types/ChangedFiles.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ export type ChangedFilesPromise = Promise<{|
|}>;

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