-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Fix invalid re-run of tests in watch mode #7347
Changes from all commits
c835176
d2812bc
54f4418
24c4b54
2858ff4
f4b9b46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
*/ | ||
|
||
import * as os from 'os'; | ||
import * as path from 'path'; | ||
import * as fs from 'graceful-fs'; | ||
import {cleanup, writeFiles} from '../Utils'; | ||
import {runContinuous} from '../runJest'; | ||
|
||
const DIR = path.resolve(os.tmpdir(), 'watch_mode_no_access'); | ||
|
||
const sleep = (time: number) => | ||
new Promise(resolve => setTimeout(resolve, time)); | ||
|
||
beforeEach(() => cleanup(DIR)); | ||
afterAll(() => cleanup(DIR)); | ||
|
||
const setupFiles = () => { | ||
writeFiles(DIR, { | ||
'__tests__/foo.test.js': ` | ||
const foo = require('../foo'); | ||
test('foo', () => { expect(typeof foo).toBe('number'); }); | ||
`, | ||
'foo.js': ` | ||
module.exports = 0; | ||
`, | ||
'package.json': JSON.stringify({ | ||
jest: {}, | ||
}), | ||
}); | ||
}; | ||
|
||
let testRun: ReturnType<typeof runContinuous>; | ||
|
||
afterEach(async () => { | ||
if (testRun) { | ||
await testRun.end(); | ||
} | ||
}); | ||
|
||
test('does not re-run tests when only access time is modified', async () => { | ||
setupFiles(); | ||
|
||
testRun = runContinuous(DIR, ['--watchAll', '--no-watchman']); | ||
|
||
const testCompletedRE = /Ran all test suites./g; | ||
const numberOfTestRuns = (stderr: string): number => { | ||
const matches = stderr.match(testCompletedRE); | ||
return matches ? matches.length : 0; | ||
}; | ||
|
||
// First run | ||
await testRun.waitUntil(({stderr}) => numberOfTestRuns(stderr) === 1); | ||
|
||
// Should re-run the test | ||
const modulePath = path.join(DIR, 'foo.js'); | ||
const stat = fs.lstatSync(modulePath); | ||
fs.utimesSync(modulePath, stat.atime.getTime(), stat.mtime.getTime()); | ||
|
||
await testRun.waitUntil(({stderr}) => numberOfTestRuns(stderr) === 2); | ||
|
||
// Should NOT re-run the test | ||
const fakeATime = 1541723621; | ||
fs.utimesSync(modulePath, fakeATime, stat.mtime.getTime()); | ||
await sleep(3000); | ||
expect(numberOfTestRuns(testRun.getCurrentOutput().stderr)).toBe(2); | ||
|
||
// Should re-run the test | ||
fs.writeFileSync(modulePath, 'module.exports = 1;', {encoding: 'utf-8'}); | ||
await testRun.waitUntil(({stderr}) => numberOfTestRuns(stderr) === 3); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -846,6 +846,19 @@ class HasteMap extends EventEmitter { | |
return; | ||
} | ||
|
||
const relativeFilePath = fastPath.relative(rootDir, filePath); | ||
const fileMetadata = hasteMap.files.get(relativeFilePath); | ||
|
||
// The file has been accessed, not modified | ||
if ( | ||
type === 'change' && | ||
fileMetadata && | ||
stat && | ||
fileMetadata[H.MTIME] === stat.mtime.getTime() | ||
) { | ||
return; | ||
} | ||
|
||
changeQueue = changeQueue | ||
.then(() => { | ||
// If we get duplicate events for the same file, ignore them. | ||
|
@@ -879,7 +892,6 @@ class HasteMap extends EventEmitter { | |
return null; | ||
}; | ||
|
||
const relativeFilePath = fastPath.relative(rootDir, filePath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it safe to extract from here? this is async, in theory its values could have changes since before waiting for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const fileMetadata = hasteMap.files.get(relativeFilePath); | ||
|
||
// If it's not an addition, delete the file and all its metadata | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we export the return type of
runContinuous
? we shouldThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, which is weird right? I thought we had an eslint rule or TS config to guard against public exports without explicit typing