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

Git support for --changedFilesWithAncestor #5189

Merged
merged 8 commits into from
Jan 4, 2018
Merged

Git support for --changedFilesWithAncestor #5189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c574721
refactor of findChangedFiles
alsuren Dec 27, 2017
beefdbc
make --changedFilesWithAncestor work with git
alsuren Dec 27, 2017
0c20845
fixup! refactor of findChangedFiles
alsuren Dec 27, 2017
007104e
add withAncestor test to git
alsuren Dec 27, 2017
129d52d
docs: update changedFilesWithAncestor's description
alsuren Dec 27, 2017
f6f5f47
docs: add --changedFilesWithAncestor fix to changelog
alsuren Dec 27, 2017
86451ba
docs: updated docs for --changedFilesWithAncestor and --lastCommit
alsuren Dec 28, 2017
4b15f86
docs: mention --watch in --changedFilesWithAncestor docs
alsuren Dec 28, 2017
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: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

### Fixes

* `[jest-cli]` `jest --onlyChanged --changedFilesWithAncestor` now also works
with git. ([#5189](https://github.com/facebook/jest/pull/5189))
* `[jest-config]` fix unexpected condition to avoid infinite recursion in
Windows platform. ([#5161](https://github.com/facebook/jest/pull/5161))

Expand Down
8 changes: 7 additions & 1 deletion docs/CLI.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ two times slower._
If you want to inspect the cache, use `--showConfig` and look at the
`cacheDirectory` value. If you need to clear the cache, use `--clearCache`.

### `--changedFilesWithAncestor`

When used together with `--onlyChanged` or `--watch`, it runs tests related to
the current changes and the changes made in the last commit.

### `--ci`

When this option is provided, Jest will assume it is running in a CI
Expand Down Expand Up @@ -182,7 +187,8 @@ Write test results to a file when the `--json` option is also specified.

### `--lastCommit`

Will run all tests affected by file changes in the last commit made.
When used together with `--onlyChanged`, it will run all tests affected by file
changes in the last commit made.

### `--listTests`

Expand Down
16 changes: 16 additions & 0 deletions integration_tests/__tests__/jest_changed_files.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,22 @@ test('gets changed files for git', async () => {
.map(filePath => path.basename(filePath))
.sort(),
).toEqual(['file1.txt']);

run(`${GIT} commit -am "test2"`, DIR);

writeFiles(DIR, {
'file4.txt': 'file4',
});

({changedFiles: files} = await getChangedFilesForRoots(roots, {
withAncestor: true,
}));
// Returns files from current uncommitted state + the last commit
expect(
Array.from(files)
.map(filePath => path.basename(filePath))
.sort(),
).toEqual(['file1.txt', 'file4.txt']);
});

test('gets changed files for hg', async () => {
Expand Down
77 changes: 46 additions & 31 deletions packages/jest-changed-files/src/git.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,44 +13,59 @@ import type {Options, SCMAdapter} from 'types/ChangedFiles';
import path from 'path';
import childProcess from 'child_process';

const findChangedFilesUsingCommand = async (args, cwd) => {
return new Promise((resolve, reject) => {
const child = childProcess.spawn('git', args, {cwd});
let stdout = '';
let stderr = '';
child.stdout.on('data', data => (stdout += data));
child.stderr.on('data', data => (stderr += data));
child.on('error', e => reject(e));
child.on('close', code => {
if (code === 0) {
stdout = stdout.trim();
if (stdout === '') {
resolve([]);
} else {
resolve(
stdout
.split('\n')
.map(changedPath => path.resolve(cwd, changedPath)),
);
}
} else {
reject(code + ': ' + stderr);
}
});
});
};

const adapter: SCMAdapter = {
findChangedFiles: async (
cwd: string,
options?: Options,
): Promise<Array<Path>> => {
if (options && options.withAncestor) {
throw new Error(
'`changedFilesWithAncestor` is not supported in git repos.',
if (options && options.lastCommit) {
return await findChangedFilesUsingCommand(
['show', '--name-only', '--pretty=%b', 'HEAD'],
cwd,
);
} else if (options && options.withAncestor) {
const changed = await findChangedFilesUsingCommand(
['diff', '--name-only', 'HEAD^'],
cwd,
);
const untracked = await findChangedFilesUsingCommand(
['ls-files', '--other', '--exclude-standard'],
cwd,
);
return changed.concat(untracked);
} else {
return await findChangedFilesUsingCommand(
['ls-files', '--other', '--modified', '--exclude-standard'],
cwd,
);
}
return new Promise((resolve, reject) => {
const args =
options && options.lastCommit
? ['show', '--name-only', '--pretty=%b', 'HEAD']
: ['ls-files', '--other', '--modified', '--exclude-standard'];
const child = childProcess.spawn('git', args, {cwd});
let stdout = '';
let stderr = '';
child.stdout.on('data', data => (stdout += data));
child.stderr.on('data', data => (stderr += data));
child.on('error', e => reject(e));
child.on('close', code => {
if (code === 0) {
stdout = stdout.trim();
if (stdout === '') {
resolve([]);
} else {
resolve(
stdout
.split('\n')
.map(changedPath => path.resolve(cwd, changedPath)),
);
}
} else {
reject(code + ': ' + stderr);
}
});
});
},

getRoot: async (cwd: string): Promise<?string> => {
Expand Down
9 changes: 4 additions & 5 deletions packages/jest-cli/src/cli/args.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,8 @@ export const options = {
},
changedFilesWithAncestor: {
description:
'When used together with `--onlyChanged`, it runs tests ' +
'related to the current changes and the changes made in the last commit. ' +
'(NOTE: this only works for hg repos)',
'When used together with `--onlyChanged` or `--watch`, it runs tests ' +
'related to the current changes and the changes made in the last commit. ',
type: 'boolean',
},
ci: {
Expand Down Expand Up @@ -268,8 +267,8 @@ export const options = {
lastCommit: {
default: undefined,
description:
'Will run all tests affected by file changes in the last ' +
'commit made.',
'When used together with `--onlyChanged`, it will run all tests ' +
'affected by file changes in the last commit made.',
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 noticed that both --lastCommit and --changedFilesWithAncestor are ignored if you don't also pass --onlyChanged or --watch. I thought about writing something in the check() function (line 15 in this file) explode if you pass an argument that will be ignored, but then I realised that someone might set --onlyChanged or --watch in their configs, or someone might add another arg that has the same effect, so I just decided to document it a bit harder.

I also decided against talking about --watch in the docs for --lastCommit, because it sounds like a pointless thing to do.

Copy link
Member

Choose a reason for hiding this comment

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

I noticed that both --lastCommit and --changedFilesWithAncestor are ignored if you don't also pass --onlyChanged or --watch.

That doesn't seem right. @cpojer WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ yarn jest --listTests | wc -l
     213
$ yarn jest --listTests --lastCommit | wc -l
     213
$ yarn jest --listTests --lastCommit --onlyChanged | wc -l
       4

@SimenB are you suggesting that:
a) we should make --lastCommit and --changedFilesWithAncestor imply --onlyChanged (may cause previously valid configurations to test fewer things than they used to)
b) we should make --lastCommit and --changedFilesWithAncestor explode if it's not combined with --onlyChanged or --watch (may cause previously valid configurations to explode, and means that the next person to add an --onlyChanged synonym needs to update the check)

or are you just after a second reviewer?

Copy link
Member

Choose a reason for hiding this comment

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

or are you just after a second reviewer?

This 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Oh, interesting. I think the reason for that is because the default at FB is that onlyChanged is true and people must pass explicitly --all to run all tests. I guess we just never noticed the inconsistency. I think --lastCommit --all doesn't make any sense, so I'm in favor of the proposed change a).

type: 'boolean',
},
listTests: {
Expand Down