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 --onlyfailures flag to work in non-watch mode #10678

Merged
merged 1 commit into from
Oct 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
### Fixes

- `[expect]` Fix `objectContaining` to work recursively into sub-objects ([#10508](https://github.com/facebook/jest/pull/10508))
- `[jest-cli, jest-core, jest-config, jest-types]` Fix `--onlyFailures` flag to work in non-watch mode ([#10678](https://github.com/facebook/jest/pull/10678/files))
- `[jest-config]` Fix for the `jest.config.ts` compiler to not interfere with `tsconfig.json` files ([#10675](https://github.com/facebook/jest/pull/10675))
- `[jest-message-util]` Update to work properly with Node 15 ([#10660](https://github.com/facebook/jest/pull/10660))
- `[jest-mock]` Allow to mock methods in getters (TypeScript 3.9 export) ([#10156](https://github.com/facebook/jest/pull/10156))
Expand Down
61 changes: 61 additions & 0 deletions e2e/__tests__/onlyFailuresNonWatch.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* 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 {tmpdir} from 'os';
import * as path from 'path';
import * as fs from 'graceful-fs';
import {cleanup, writeFiles} from '../Utils';
import runJest from '../runJest';

const DIR = path.resolve(tmpdir(), 'non-watch-mode-onlyFailures');

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

test('onlyFailures flag works in non-watch mode', () => {
writeFiles(DIR, {
'__tests__/a.js': `
test('bar', () => { expect('bar').toBe('foo'); });
`,
'__tests__/b.js': `
test('foo', () => { expect('foo').toBe('foo'); });
`,
'package.json': JSON.stringify({
jest: {
testEnvironment: 'node',
},
}),
});

let stdout, stderr;

({stdout, stderr} = runJest(DIR));
expect(stdout).toBe('');
expect(stderr).toMatch('FAIL __tests__/a.js');
expect(stderr).toMatch('PASS __tests__/b.js');

// only the failed test should run and it should fail
({stdout, stderr} = runJest(DIR, ['--onlyFailures']));
expect(stdout).toBe('');
expect(stderr).toMatch('FAIL __tests__/a.js');
expect(stderr).not.toMatch('__tests__/b.js');

// fix the failing test
const data = "test('bar 1', () => { expect('bar').toBe('bar'); })";
fs.writeFileSync(path.join(DIR, '__tests__/a.js'), data);

// only the failed test should run and it should pass
({stdout, stderr} = runJest(DIR, ['--onlyFailures']));
expect(stdout).toBe('');
expect(stderr).toMatch('PASS __tests__/a.js');
expect(stderr).not.toMatch('__tests__/b.js');

// No test should run
({stdout, stderr} = runJest(DIR, ['--onlyFailures']));
expect(stdout).toBe('No failed test found.');
expect(stderr).toBe('');
});
7 changes: 7 additions & 0 deletions packages/jest-cli/src/__tests__/cli/args.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ describe('check', () => {
);
});

it('raises an exception if onlyFailures and watchAll are both specified', () => {
const argv = {onlyFailures: true, watchAll: true} as Config.Argv;
expect(() => check(argv)).toThrow(
'Both --onlyFailures and --watchAll were specified',
);
});

it('raises an exception when lastCommit and watchAll are both specified', () => {
const argv = {lastCommit: true, watchAll: true} as Config.Argv;
expect(() => check(argv)).toThrow(
Expand Down
7 changes: 7 additions & 0 deletions packages/jest-cli/src/cli/args.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ export function check(argv: Config.Argv): true {
}
}

if (argv.onlyFailures && argv.watchAll) {
throw new Error(
`Both --onlyFailures and --watchAll were specified, but these two ` +
'options do not make sense together.',
);
}

if (argv.findRelatedTests && argv._.length === 0) {
throw new Error(
'The --findRelatedTests option requires file paths to be specified.\n' +
Expand Down
1 change: 1 addition & 0 deletions packages/jest-config/src/ValidConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ const initialOptions: Config.InitialOptions = {
notify: false,
notifyMode: 'failure-change',
onlyChanged: false,
onlyFailures: false,
preset: 'react-native',
prettierPath: '<rootDir>/node_modules/prettier',
projects: ['project-a', 'project-b/'],
Expand Down
5 changes: 4 additions & 1 deletion packages/jest-config/src/normalize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,7 @@ export default function normalize(
case 'notify':
case 'notifyMode':
case 'onlyChanged':
case 'onlyFailures':
case 'outputFile':
case 'passWithNoTests':
case 'replname':
Expand Down Expand Up @@ -985,10 +986,12 @@ export default function normalize(

if (argv.all) {
newOptions.onlyChanged = false;
newOptions.onlyFailures = false;
} else if (newOptions.testPathPattern) {
// When passing a test path pattern we don't want to only monitor changed
// files unless `--watch` is also passed.
// or failed files unless `--watch` is also passed.
newOptions.onlyChanged = newOptions.watch;
newOptions.onlyFailures = newOptions.watch;
Copy link
Member

Choose a reason for hiding this comment

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

this is a change in behavior for the watch flag, but I like it

Copy link
Member

Choose a reason for hiding this comment

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

hmm, coming back to this, I don't 😅 If I run in watch mode I want to run all tests I specify, not just the ones that failed last time (unless I opt in to this behavior)

Copy link
Member

Choose a reason for hiding this comment

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

When I said I liked it, what I thought at the time this did was, that in addition to running the specified files, we'll run any failing tests. But that's not what this does

Choose a reason for hiding this comment

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

There is no way to opt out of this behaviour without using the command line Press f as far as I can tell. It's broken running jest in watch mode in webstorm as you cannot interact with the CLI. This behaviour is a pain.

Copy link
Member

Choose a reason for hiding this comment

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

@domarmstrong reverted in #10692, will make a new release once #10690 is solved

Choose a reason for hiding this comment

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

Brilliant thanks for the info 👍

}

if (!newOptions.onlyChanged) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@

exports[`getNoTestsFoundMessage returns correct message when monitoring only changed 1`] = `"<bold>No tests found related to files changed since last commit.</>"`;

exports[`getNoTestsFoundMessage returns correct message when monitoring only failures 1`] = `
"<bold>No failed test found.</>
<bold></><dim>Press \`f\` to quit \\"only failed tests\\" mode.</>"
`;
exports[`getNoTestsFoundMessage returns correct message when monitoring only failures 1`] = `"<bold>No failed test found.</>"`;

exports[`getNoTestsFoundMessage returns correct message with passWithNoTests 1`] = `"<bold>No tests found, exiting with code 0</>"`;

Expand Down
20 changes: 15 additions & 5 deletions packages/jest-core/src/getNoTestFoundFailed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,20 @@
*/

import chalk = require('chalk');
import type {Config} from '@jest/types';
import {isInteractive} from 'jest-util';

export default function getNoTestFoundFailed(): string {
return (
chalk.bold('No failed test found.\n') +
chalk.dim('Press `f` to quit "only failed tests" mode.')
);
export default function getNoTestFoundFailed(
globalConfig: Config.GlobalConfig,
): string {
let msg = chalk.bold('No failed test found.');
if (isInteractive) {
msg += chalk.dim(
'\n' +
(globalConfig.watch
? 'Press `f` to quit "only failed tests" mode.'
: 'Run Jest without `--onlyFailures` or with `--all` to run all tests.'),
);
}
return msg;
}
2 changes: 1 addition & 1 deletion packages/jest-core/src/getNoTestsFoundMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export default function getNoTestsFoundMessage(
globalConfig: Config.GlobalConfig,
): string {
if (globalConfig.onlyFailures) {
return getNoTestFoundFailed();
return getNoTestFoundFailed(globalConfig);
}
if (globalConfig.onlyChanged) {
return getNoTestFoundRelatedToChangedFiles(globalConfig);
Expand Down
10 changes: 7 additions & 3 deletions packages/jest-core/src/runJest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,13 @@ export default async function runJest({
return;
}

if (globalConfig.onlyFailures && failedTestsCache) {
allTests = failedTestsCache.filterTests(allTests);
globalConfig = failedTestsCache.updateConfig(globalConfig);
if (globalConfig.onlyFailures) {
if (failedTestsCache) {
allTests = failedTestsCache.filterTests(allTests);
globalConfig = failedTestsCache.updateConfig(globalConfig);
} else {
allTests = sequencer.allFailedTests(allTests);
}
}

const hasTests = allTests.length > 0;
Expand Down
15 changes: 15 additions & 0 deletions packages/jest-test-sequencer/src/__tests__/test_sequencer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,21 @@ test('writes the cache based on results without existing cache', () => {
});
});

test('returns failed tests in sorted order', () => {
fs.readFileSync.mockImplementationOnce(() =>
JSON.stringify({
'/test-a.js': [SUCCESS, 5],
'/test-ab.js': [FAIL, 1],
'/test-c.js': [FAIL],
}),
);
const testPaths = ['/test-a.js', '/test-ab.js', '/test-c.js'];
expect(sequencer.allFailedTests(toTests(testPaths))).toEqual([
{context, duration: undefined, path: '/test-c.js'},
{context, duration: 1, path: '/test-ab.js'},
]);
});

test('writes the cache based on the results', () => {
fs.readFileSync.mockImplementationOnce(() =>
JSON.stringify({
Expand Down
8 changes: 8 additions & 0 deletions packages/jest-test-sequencer/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,14 @@ export default class TestSequencer {
});
}

allFailedTests(tests: Array<Test>): Array<Test> {
const hasFailed = (cache: Cache, test: Test) =>
cache[test.path]?.[0] === FAIL;
return this.sort(
tests.filter(test => hasFailed(this._getCache(test), test)),
);
}

cacheResults(tests: Array<Test>, results: AggregatedResult): void {
const map = Object.create(null);
tests.forEach(test => (map[test.path] = test));
Expand Down
2 changes: 2 additions & 0 deletions packages/jest-types/src/Config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ export type InitialOptions = Partial<{
notify: boolean;
notifyMode: string;
onlyChanged: boolean;
onlyFailures: boolean;
outputFile: Path;
passWithNoTests: boolean;
preprocessorIgnorePatterns: Array<Glob>;
Expand Down Expand Up @@ -418,6 +419,7 @@ export type Argv = Arguments<
notify: boolean;
notifyMode: string;
onlyChanged: boolean;
onlyFailures: boolean;
outputFile: string;
preset: string | null | undefined;
projects: Array<string>;
Expand Down