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

Add path case-insensitivity if onlyChanged option is active, fixes #4644 #4730

Merged
merged 10 commits into from
Nov 17, 2017
Merged

Add path case-insensitivity if onlyChanged option is active, fixes #4644 #4730

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
78dadae
Correct project directory path case on Windows
peterdanis Oct 20, 2017
c47755f
fixup! Add try-catch block with fallback
peterdanis Oct 20, 2017
af59143
Make only_changed test pass on Windows
peterdanis Oct 31, 2017
b1c3f48
Add test
peterdanis Oct 31, 2017
f3f4466
Update changelog
peterdanis Oct 31, 2017
ed855fc
fixup! Simplify original writeFiles change
peterdanis Oct 31, 2017
f6350d1
fixup! Ignore process.binding flow error
peterdanis Oct 31, 2017
4c82bb5
fixup! Use correct dir
peterdanis Oct 31, 2017
aa941c6
fixup! Fix lint error
peterdanis Oct 31, 2017
a0157e7
Merge branch 'master' into dev
cpojer Nov 17, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
* `[jest-config]` Include error message for `preset` json ([#4766](https://github.com/facebook/jest/pull/4766))
* `[pretty-format]` Throw `PrettyFormatPluginError` if a plugin halts with an exception ([#4787](https://github.com/facebook/jest/pull/4787))
* `[expect]` Keep the stack trace unchanged when `PrettyFormatPluginError` is thrown by pretty-format ([#4787](https://github.com/facebook/jest/pull/4787))
* `[jest-cli]` Fix `--onlyChanged` path case sensitivity on Windows platform ([#4730](https://github.com/facebook/jest/pull/4730))

### Features
* `[eslint-plugin-jest]` Add `prefer-to-have-length` lint rule. ([#4771](https://github.com/facebook/jest/pull/4771))
Expand Down
88 changes: 62 additions & 26 deletions integration_tests/__tests__/only_changed.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,10 @@ import os from 'os';
import path from 'path';
const {cleanup, run, writeFiles} = require('../utils');

const skipOnWindows = require('../../scripts/skip_on_windows');
const DIR = path.resolve(os.tmpdir(), 'jest_only_changed');
const GIT = 'git -c user.name=jest_test -c [email protected]';
const HG = 'hg --config ui.username=jest_test';

skipOnWindows.suite();

beforeEach(() => cleanup(DIR));
afterEach(() => cleanup(DIR));

Expand All @@ -45,7 +42,7 @@ test('run only changed files', () => {
expect(stdout).toMatch('No tests found related to files');

({stderr} = runJest(DIR, ['-o', '--lastCommit']));
expect(stderr).toMatch('PASS __tests__/file1.test.js');
expect(stderr).toMatch(/PASS __tests__(\/|\\)file1.test.js/);

writeFiles(DIR, {
'__tests__/file2.test.js': `require('../file2'); test('file2', () => {});`,
Expand All @@ -56,9 +53,9 @@ test('run only changed files', () => {

({stderr} = runJest(DIR, ['-o']));

expect(stderr).not.toMatch('PASS __tests__/file1.test.js');
expect(stderr).toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch('PASS __tests__/file3.test.js');
expect(stderr).not.toMatch(/PASS __tests__(\/|\\)file1.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);

run(`${GIT} add .`, DIR);
run(`${GIT} commit -m "second"`, DIR);
Expand All @@ -71,9 +68,9 @@ test('run only changed files', () => {
});

({stderr} = runJest(DIR, ['-o']));
expect(stderr).not.toMatch('PASS __tests__/file1.test.js');
expect(stderr).toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch('PASS __tests__/file3.test.js');
expect(stderr).not.toMatch(/PASS __tests__(\/|\\)file1.test.j/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);
});

test('onlyChanged in config is overwritten by --all or testPathPattern', () => {
Expand All @@ -100,7 +97,7 @@ test('onlyChanged in config is overwritten by --all or testPathPattern', () => {
);

({stderr} = runJest(DIR, ['--lastCommit']));
expect(stderr).toMatch('PASS __tests__/file1.test.js');
expect(stderr).toMatch(/PASS __tests__(\/|\\)file1.test.js/);

writeFiles(DIR, {
'__tests__/file2.test.js': `require('../file2'); test('file2', () => {});`,
Expand All @@ -111,9 +108,9 @@ test('onlyChanged in config is overwritten by --all or testPathPattern', () => {

({stderr} = runJest(DIR));

expect(stderr).not.toMatch('PASS __tests__/file1.test.js');
expect(stderr).toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch('PASS __tests__/file3.test.js');
expect(stderr).not.toMatch(/PASS __tests__(\/|\\)file1.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);

run(`${GIT} add .`, DIR);
run(`${GIT} commit -m "second"`, DIR);
Expand All @@ -123,22 +120,22 @@ test('onlyChanged in config is overwritten by --all or testPathPattern', () => {

({stderr, stdout} = runJest(DIR, ['file2.test.js']));
expect(stdout).not.toMatch('No tests found related to files');
expect(stderr).toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
expect(stderr).toMatch('1 total');

writeFiles(DIR, {
'file2.js': 'module.exports = {modified: true}',
});

({stderr} = runJest(DIR));
expect(stderr).not.toMatch('PASS __tests__/file1.test.js');
expect(stderr).toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch('PASS __tests__/file3.test.js');
expect(stderr).not.toMatch(/PASS __tests__(\/|\\)file1.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);

({stderr} = runJest(DIR, ['--all']));
expect(stderr).toMatch('PASS __tests__/file1.test.js');
expect(stderr).toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch('PASS __tests__/file3.test.js');
expect(stderr).toMatch(/PASS __tests__(\/|\\)file1.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);
});

test('gets changed files for hg', async () => {
Expand Down Expand Up @@ -173,7 +170,7 @@ test('gets changed files for hg', async () => {
});

({stdout, stderr} = runJest(DIR, ['-o']));
expect(stderr).toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);

run(`${HG} add .`, DIR);
run(`${HG} commit -m "test2"`, DIR);
Expand All @@ -183,10 +180,49 @@ test('gets changed files for hg', async () => {
});

({stdout, stderr} = runJest(DIR, ['-o']));
expect(stderr).toMatch('PASS __tests__/file3.test.js');
expect(stderr).not.toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);
expect(stderr).not.toMatch(/PASS __tests__(\/|\\)file2.test.js/);

({stdout, stderr} = runJest(DIR, ['-o', '--changedFilesWithAncestor']));
expect(stderr).toMatch('PASS __tests__/file2.test.js');
expect(stderr).toMatch('PASS __tests__/file3.test.js');
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);
});

test('path on Windows is case-insensitive', () => {
if (process.platform !== 'win32') {
// This test is Windows specific, skip it on other platforms.
return;
}

const modifiedDIR = path.resolve(DIR, 'outer_dir', 'inner_dir');
const incorrectModifiedDIR = path.resolve(DIR, 'OUTER_dir', 'inner_dir');
let stderr;
let stdout;

writeFiles(modifiedDIR, {
'.watchmanconfig': '',
'__tests__/file1.test.js': `require('../file1'); test('file1', () => {});`,
'file1.js': 'module.exports = {}',
'package.json': '{}',
});

run(`${GIT} init`, modifiedDIR);
run(`${GIT} add .`, modifiedDIR);
run(`${GIT} commit -m "first"`, modifiedDIR);

({stdout} = runJest(modifiedDIR, ['-o']));
expect(stdout).toMatch('No tests found related to files');

writeFiles(modifiedDIR, {
'__tests__/file2.test.js': `require('../file2'); test('file2', () => {});`,
'__tests__/file3.test.js': `require('../file3'); test('file3', () => {});`,
'file2.js': 'module.exports = {}',
'file3.js': `require('./file2')`,
});

({stderr} = runJest(modifiedDIR, ['-o']));

expect(stderr).not.toMatch(/PASS __tests__(\/|\\)file1.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);
});
7 changes: 6 additions & 1 deletion integration_tests/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,12 @@ const cleanup = (directory: string) => rimraf.sync(directory);
const writeFiles = (directory: string, files: {[filename: string]: string}) => {
mkdirp.sync(directory);
Object.keys(files).forEach(fileOrPath => {
const filePath = fileOrPath.split(path.sep); // ['tmp', 'a.js']
Copy link
Member

Choose a reason for hiding this comment

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

can we do const filePath = fileOrPath.split(path.posix.sep); here? (or just fileOrPath.split('/'))

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 suggestion. Changed to const filePath = fileOrPath.split('/')

let filePath;
if (process.platform === 'win32') {
filePath = fileOrPath.replace(/(\/)/g, path.sep).split(path.sep); // ['tmp', 'a.js']
} else {
filePath = fileOrPath.split(path.sep); // ['tmp', 'a.js']
}
const filename = filePath.pop(); // filepath becomes dirPath (no filename)

if (filePath.length) {
Expand Down
9 changes: 9 additions & 0 deletions packages/jest-cli/src/cli/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,15 @@ const getProjectListFromCLIArgs = (argv, project: ?Path) => {
projects.push(project);
}

if (!projects.length && process.platform === 'win32') {
try {
projects.push(process.binding('fs').realpath(process.cwd()));
Copy link
Member

Choose a reason for hiding this comment

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

why not fs.realpathSync()? process.binding seems overkill

Copy link
Contributor Author

@peterdanis peterdanis Oct 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

fs.realpathSync() does not correct the path.

They differ:

fs.realpathSync(process.cwd()):
C:\Users\pdanis\desktop\projects\my-js-learning\other\temp
(edit: corrected \\projects to \projects)

process.binding('fs').realpath(process.cwd()):
C:\Users\pdanis\Desktop\Projects\my-js-learning\other\temp

Copy link
Member

Choose a reason for hiding this comment

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

Huh, that's weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand it correctly fs.realpathSync() uses JS fs and process.binding('fs').realpath() is using GetFinalPathNameByHandle() on Windows.

There is a PR, which should expose process.binding('fs').realpath() as fs.realpathSync.native().
nodejs/node#15776

} catch (err) {
// do nothing, just catch error
// process.binding('fs').realpath can throw, e.g. on mapped drives
}
}

if (!projects.length) {
projects.push(process.cwd());
}
Expand Down