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

Add displayName to MPR #4327

Merged
merged 10 commits into from
Aug 27, 2017
Merged

Add displayName to MPR #4327

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
84d73c7
Modify flow types to support displayName
rogeliog Aug 22, 2017
372a1bb
Add project displayName
rogeliog Aug 22, 2017
417416a
Some small fixes
rogeliog Aug 22, 2017
a6ae265
Add tests
rogeliog Aug 22, 2017
1192760
Fix eslint
rogeliog Aug 22, 2017
27a07dc
Merge branch 'master' into add-display-name
rogeliog Aug 24, 2017
dc72c42
Better handling for non interactive terminals
rogeliog Aug 25, 2017
1084ae9
Update tests
rogeliog Aug 25, 2017
1f6f3df
Merge branch 'master' into add-display-name
rogeliog Aug 27, 2017
fdb0d1f
Use supportColor
rogeliog Aug 27, 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
20 changes: 11 additions & 9 deletions integration_tests/__tests__/__snapshots__/multi_project_runner.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`can pass projects or global config 1`] = `
"Test Suites: 2 failed, 2 total
"Test Suites: 3 failed, 3 total
Tests: 0 total
Snapshots: 0 total
Time: <<REPLACED>>
Expand All @@ -10,29 +10,31 @@ Ran all test suites.
`;

exports[`can pass projects or global config 2`] = `
"Test Suites: 2 passed, 2 total
Tests: 2 passed, 2 total
"Test Suites: 3 passed, 3 total
Tests: 3 passed, 3 total
Snapshots: 0 total
Time: <<REPLACED>>
Ran all test suites in 2 projects.
Ran all test suites in 3 projects.
"
`;

exports[`can pass projects or global config 3`] = `
"PASS project1/__tests__/file1.test.js
"PASS BACKEND project1/__tests__/file1.test.js
PASS UI project3/__tests__/file1.test.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronabramov There is a double space here... this happens in cases where we disable chalk... because we are doing

chalk.someOptions(` ${displayName} `)

Should we remove those extra spaces when chalk is disabled?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's remove it in that case. Both on the left and right.

Copy link
Contributor Author

@rogeliog rogeliog Aug 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I thought that only the displayName had the extra spaces but it seems that PASS/FAIL also have them already in master... so the fix might involve a bit more changes
screen shot 2017-08-24 at 11 16 33 pm

Copy link
Contributor Author

@rogeliog rogeliog Aug 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Here is the diff for removing the extra spaces in PASS and FAIL rogeliog/jest@add-display-name...rogeliog:add-display-name-full-diff (Still need some work to make sure it works with CI=true jest --color)

@cpojer @aaronabramov I'm not sure if that leading space before PASS and the two after it are intentionally there in the current version or if it is fine to remove them? I'm fine with either I just wanted to understand if any of these current behavior was intentional... I'll update this PR after I get a better understanding on the expected behavior of non interactive non MPR output :) thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to remove them when colors are disabled.

PASS project2/__tests__/file1.test.js"
`;

exports[`can pass projects or global config 4`] = `
"Test Suites: 2 passed, 2 total
Tests: 2 passed, 2 total
"Test Suites: 3 passed, 3 total
Tests: 3 passed, 3 total
Snapshots: 0 total
Time: <<REPLACED>>
Ran all test suites in 2 projects.
Ran all test suites in 3 projects.
"
`;

exports[`can pass projects or global config 5`] = `
"PASS project1/__tests__/file1.test.js
"PASS BACKEND project1/__tests__/file1.test.js
PASS UI project3/__tests__/file1.test.js
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably going to fail when they're executed in different order. Previous test had identical lines (:

Copy link
Member

Choose a reason for hiding this comment

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

Ohh, now I get this change that @thymikee was making for snapshots. Can we merge that one for the next alpha release? We can break the snapshot format for 21, and update the snapshot version. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah! i'll rerecord snapshots in www

Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a mechanism to have multiple versions of snapshots at the same time? i remember we had this idea somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to decouple snapshot format changes from jest as a framework changes.
If jest update doesn't go well, rolling it back creates a huge mess in master because of all snapshot conflicts

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 not the right place to have this conversation, I commented here: #4183 (comment) cc @thymikee

Copy link
Contributor Author

@rogeliog rogeliog Aug 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@aaronabramov Should it be fine because it is using sortLines ?https://github.com/facebook/jest/blob/fbd07088383e456fae050ae03feaebd40de7dbf8/integration_tests/__tests__/multi_project_runner.test.js#L34-L40

PASS project2/__tests__/file1.test.js"
`;
18 changes: 15 additions & 3 deletions integration_tests/__tests__/multi_project_runner.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,19 @@ test('can pass projects or global config', () => {
test('file1', () => {});
`,
'project1/file1.js': fileContentWithProvidesModule('file1'),
'project1/jest.config.js': `module.exports = {rootDir: './'}`,
'project1/jest.config.js': `module.exports = {rootDir: './', displayName: 'BACKEND'}`,
'project2/__tests__/file1.test.js': `
const file1 = require('file1');
test('file1', () => {});
`,
'project2/file1.js': fileContentWithProvidesModule('file1'),
'project2/jest.config.js': `module.exports = {rootDir: './'}`,
'project3/__tests__/file1.test.js': `
const file1 = require('file1');
test('file1', () => {});
`,
'project3/file1.js': fileContentWithProvidesModule('file1'),
'project3/jest.config.js': `module.exports = {rootDir: './', displayName: 'UI'}`,
});
let stderr;

Expand All @@ -68,12 +74,18 @@ test('can pass projects or global config', () => {
writeFiles(DIR, {
'global_config.js': `
module.exports = {
projects: ['project1/', 'project2/'],
projects: ['project1/', 'project2/', 'project3/'],
};
`,
});

({stderr} = runJest(DIR, ['-i', '--projects', 'project1', 'project2']));
({stderr} = runJest(DIR, [
'-i',
'--projects',
'project1',
'project2',
'project3',
]));

const result1 = extractSummary(stderr);
expect(result1.summary).toMatchSnapshot();
Expand Down
23 changes: 15 additions & 8 deletions packages/jest-cli/src/reporters/Status.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,13 @@ import type {ProjectConfig, Path} from 'types/Config';
import type {ReporterOnStartOptions} from 'types/Reporters';

import chalk from 'chalk';
import {getSummary, trimAndFormatPath, wrapAnsiString} from './utils';
import stringLength from 'string-length';
import {
getSummary,
trimAndFormatPath,
wrapAnsiString,
printDisplayName,
} from './utils';

const RUNNING_TEXT = ' RUNS ';
const RUNNING = chalk.reset.inverse.yellow.bold(RUNNING_TEXT) + ' ';
Expand Down Expand Up @@ -136,15 +142,16 @@ class Status {
this._currentTests.get().forEach(record => {
if (record) {
const {config, testPath} = record;

const projectDisplayName = config.displayName
? printDisplayName(config) + ' '
: '';
const prefix = RUNNING + projectDisplayName;

content +=
wrapAnsiString(
RUNNING +
trimAndFormatPath(
RUNNING_TEXT.length + 1,
config,
testPath,
width,
),
prefix +
trimAndFormatPath(stringLength(prefix), config, testPath, width),
width,
) + '\n';
}
Expand Down
4 changes: 2 additions & 2 deletions packages/jest-cli/src/reporters/__tests__/default_reporter.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ afterEach(() => {
});

test('normal output, everything goes to stdout', () => {
const reporter = new DefaultReporter({useStderr: false});
const reporter = new DefaultReporter({rootDir: '', useStderr: false});

reporter.onRunStart(aggregatedResults, options);
reporter.onTestStart(testCase);
Expand All @@ -67,7 +67,7 @@ test('normal output, everything goes to stdout', () => {
});

test('when using stderr as output, no stdout call is made', () => {
const reporter = new DefaultReporter({useStderr: true});
const reporter = new DefaultReporter({rootDir: '', useStderr: true});

reporter.onRunStart(aggregatedResults, options);
reporter.onTestStart(testCase);
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-cli/src/reporters/default_reporter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class DefaultReporter extends BaseReporter {
result: TestResult,
) {
if (!result.skipped) {
this.log(getResultHeader(result, config));
this.log(getResultHeader(result, this._globalConfig, config));

const consoleBuffer = result.console;
if (consoleBuffer && consoleBuffer.length) {
Expand Down
21 changes: 16 additions & 5 deletions packages/jest-cli/src/reporters/get_result_header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,23 @@
* @flow
*/

import type {Path} from 'types/Config';
import type {GlobalConfig, ProjectConfig} from 'types/Config';
import type {TestResult} from 'types/TestResult';

import chalk from 'chalk';
import {formatTestPath} from './utils';
import {formatTestPath, printDisplayName} from './utils';

const LONG_TEST_COLOR = chalk.reset.bold.bgRed;
// Explicitly reset for these messages since they can get written out in the
// middle of error logging
const FAIL = chalk.reset.inverse.bold.red(' FAIL ');
const PASS = chalk.reset.inverse.bold.green(' PASS ');

module.exports = (result: TestResult, config: {rootDir: Path}) => {
module.exports = (
result: TestResult,
globalConfig: GlobalConfig,
projectConfig?: ProjectConfig,
) => {
const testPath = result.testFilePath;
const status =
result.numFailingTests > 0 || result.testExecError ? FAIL : PASS;
Expand All @@ -39,8 +43,15 @@ module.exports = (result: TestResult, config: {rootDir: Path}) => {
testDetail.push(`${toMB(result.memoryUsage)} MB heap size`);
}

const projectDisplayName =
projectConfig && projectConfig.displayName
? printDisplayName(projectConfig) + ' '
: '';

return (
`${status} ${formatTestPath(config, testPath)}` +
(testDetail.length ? ` (${testDetail.join(', ')})` : '')
`${status} ${projectDisplayName}${formatTestPath(
projectConfig ? projectConfig : globalConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm a little worried about that. Is this for resolving the relative test path?

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'm not sure, but the behavior here should not change with this PR

testPath,
)}` + (testDetail.length ? ` (${testDetail.join(', ')})` : '')
);
};
8 changes: 7 additions & 1 deletion packages/jest-cli/src/reporters/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
* @flow
*/

import type {Path} from 'types/Config';
import type {Path, ProjectConfig} from 'types/Config';
import type {AggregatedResult} from 'types/TestResult';

import path from 'path';
Expand All @@ -23,6 +23,11 @@ type SummaryOptions = {|

const PROGRESS_BAR_WIDTH = 40;

const printDisplayName = (config: ProjectConfig) =>
config.displayName
? chalk.reset.inverse.white.dim(` ${config.displayName} `)
: '';

const trimAndFormatPath = (
pad: number,
config: {rootDir: Path},
Expand Down Expand Up @@ -235,6 +240,7 @@ module.exports = {
formatTestPath,
getSummary,
pluralize,
printDisplayName,
relativePath,
trimAndFormatPath,
wrapAnsiString,
Expand Down
1 change: 1 addition & 0 deletions packages/jest-config/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ const getConfigs = (
cacheDirectory: options.cacheDirectory,
clearMocks: options.clearMocks,
coveragePathIgnorePatterns: options.coveragePathIgnorePatterns,
displayName: options.displayName,
globals: options.globals,
haste: options.haste,
moduleDirectories: options.moduleDirectories,
Expand Down
1 change: 1 addition & 0 deletions packages/jest-config/src/normalize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,7 @@ function normalize(options: InitialOptions, argv: Argv) {
case 'collectCoverage':
case 'coverageReporters':
case 'coverageThreshold':
case 'displayName':
case 'expand':
case 'globals':
case 'findRelatedTests':
Expand Down
1 change: 1 addition & 0 deletions packages/jest-config/src/valid_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ module.exports = ({
branches: 50,
},
},
displayName: 'project-name',
expand: false,
forceExit: false,
globals: {},
Expand Down
1 change: 1 addition & 0 deletions test_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const DEFAULT_PROJECT_CONFIG: ProjectConfig = {
cacheDirectory: '/test_cache_dir/',
clearMocks: false,
coveragePathIgnorePatterns: [],
displayName: undefined,
globals: {},
haste: {
providesModuleNodeModules: [],
Expand Down
26 changes: 14 additions & 12 deletions types/Config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export type HasteConfig = {|
defaultPlatform?: ?string,
hasteImplModulePath?: string,
platforms?: Array<string>,
providesModuleNodeModules: Array<string>,
providesModuleNodeModules: Array<string>
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened to the formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was about to add that comment... this is what my Prettier config is formatting this file to... I'll double check if I have something wrong on my setup

|};

export type ReporterConfig = [string, Object];
Expand All @@ -37,7 +37,7 @@ export type DefaultOptions = {|
mapCoverage: boolean,
moduleDirectories: Array<string>,
moduleFileExtensions: Array<string>,
moduleNameMapper: {[key: string]: string},
moduleNameMapper: { [key: string]: string },
modulePathIgnorePatterns: Array<string>,
noStackTrace: boolean,
notify: boolean,
Expand All @@ -58,7 +58,7 @@ export type DefaultOptions = {|
useStderr: boolean,
verbose: ?boolean,
watch: boolean,
watchman: boolean,
watchman: boolean
|};

export type InitialOptions = {
Expand All @@ -71,11 +71,12 @@ export type InitialOptions = {
changedFilesWithAncestor?: boolean,
collectCoverage?: boolean,
collectCoverageFrom?: Array<Glob>,
collectCoverageOnlyFrom?: {[key: string]: boolean},
collectCoverageOnlyFrom?: { [key: string]: boolean },
coverageDirectory?: string,
coveragePathIgnorePatterns?: Array<string>,
coverageReporters?: Array<string>,
coverageThreshold?: {global: {[key: string]: number}},
coverageThreshold?: { global: { [key: string]: number } },
displayName?: string,
expand?: boolean,
findRelatedTests?: boolean,
forceExit?: boolean,
Expand All @@ -89,7 +90,7 @@ export type InitialOptions = {
moduleDirectories?: Array<string>,
moduleFileExtensions?: Array<string>,
moduleLoader?: Path,
moduleNameMapper?: {[key: string]: string},
moduleNameMapper?: { [key: string]: string },
modulePathIgnorePatterns?: Array<string>,
modulePaths?: Array<string>,
name?: string,
Expand Down Expand Up @@ -124,15 +125,15 @@ export type InitialOptions = {
testRunner?: string,
testURL?: string,
timers?: 'real' | 'fake',
transform?: {[key: string]: string},
transform?: { [key: string]: string },
transformIgnorePatterns?: Array<Glob>,
unmockedModulePathPatterns?: Array<string>,
updateSnapshot?: boolean,
useStderr?: boolean,
verbose?: ?boolean,
watch?: boolean,
watchAll?: boolean,
watchman?: boolean,
watchman?: boolean
};

export type SnapshotUpdateState = 'all' | 'new' | 'none';
Expand All @@ -142,10 +143,10 @@ export type GlobalConfig = {|
changedFilesWithAncestor: boolean,
collectCoverage: boolean,
collectCoverageFrom: Array<Glob>,
collectCoverageOnlyFrom: ?{[key: string]: boolean},
collectCoverageOnlyFrom: ?{ [key: string]: boolean },
coverageDirectory: string,
coverageReporters: Array<string>,
coverageThreshold: {global: {[key: string]: number}},
coverageThreshold: { global: { [key: string]: number } },
expand: boolean,
findRelatedTests: boolean,
forceExit: boolean,
Expand Down Expand Up @@ -175,7 +176,7 @@ export type GlobalConfig = {|
verbose: ?boolean,
watch: boolean,
watchAll: boolean,
watchman: boolean,
watchman: boolean
|};

export type ProjectConfig = {|
Expand All @@ -185,6 +186,7 @@ export type ProjectConfig = {|
cacheDirectory: Path,
clearMocks: boolean,
coveragePathIgnorePatterns: Array<string>,
displayName: ?string,
globals: ConfigGlobals,
haste: HasteConfig,
moduleDirectories: Array<string>,
Expand Down Expand Up @@ -213,5 +215,5 @@ export type ProjectConfig = {|
timers: 'real' | 'fake',
transform: Array<[string, Path]>,
transformIgnorePatterns: Array<Glob>,
unmockedModulePathPatterns: ?Array<string>,
unmockedModulePathPatterns: ?Array<string>
|};