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

Fix invalid re-run of tests in watch mode #7347

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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -13,6 +13,7 @@
- `[jest-circus]` [**BREAKING**] Fail tests if a test takes a done callback and have return values ([#9129](https://github.com/facebook/jest/pull/9129))
- `[jest-circus]` [**BREAKING**] Throw a proper error if a test / hook is defined asynchronously ([#8096](https://github.com/facebook/jest/pull/8096))
- `[jest-config, jest-resolve]` [**BREAKING**] Remove support for `browser` field ([#9943](https://github.com/facebook/jest/pull/9943))
- `[jest-haste-map]` Stop reporting files as changed when they are only accessed ([#7347](https://github.com/facebook/jest/pull/7347))
- `[jest-resolve]` Show relative path from root dir for `module not found` errors ([#9963](https://github.com/facebook/jest/pull/9963))

### Chore & Maintenance
Expand Down
79 changes: 79 additions & 0 deletions e2e/__tests__/watchModeNoAccess.test.ts
@@ -0,0 +1,79 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. 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.
*
* @flow
thymikee marked this conversation as resolved.
Show resolved Hide resolved
*/

'use strict';
thymikee marked this conversation as resolved.
Show resolved Hide resolved

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>;
Copy link
Member

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 should

Copy link
Collaborator

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


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);
});
14 changes: 11 additions & 3 deletions packages/jest-haste-map/src/index.ts
Expand Up @@ -846,6 +846,17 @@ 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?.[H.MTIME] === stat?.mtime.getTime()
thymikee marked this conversation as resolved.
Show resolved Hide resolved
) {
return;
}

changeQueue = changeQueue
.then(() => {
// If we get duplicate events for the same file, ignore them.
Expand Down Expand Up @@ -879,9 +890,6 @@ class HasteMap extends EventEmitter {
return null;
};

const relativeFilePath = fastPath.relative(rootDir, filePath);
Copy link
Member

Choose a reason for hiding this comment

The 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 changeQueue, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

rootDir and filePath are closed over, so they shouldn't change, but fileMetadata may technically change in the mean time, so I'll bring it back there

const fileMetadata = hasteMap.files.get(relativeFilePath);

// If it's not an addition, delete the file and all its metadata
if (fileMetadata != null) {
const moduleName = fileMetadata[H.ID];
Expand Down