Add displayName to MPR #4327

rogeliog · 2017-08-22T22:37:21Z

Summary

Fixes #4065

Adds the displayName option for multi project runner

Pending items

Add docs for displayName

Test plan

I have some questions regarding the UX for this:

What happens when some of the projects have displayName and some don't do we simply show the name for some and leave it empty for the rest? Or does displayName need to be present in either all or none of the projects?
What happens when the displayName of the project is really long, or not all caps, or with spaces? Do we normalize the diplayName to all caps no spaces?
Do we allow non alpha numeric characters like emojis?

aaronabramov

This is a game changer!
a few nits

aaronabramov · 2017-08-22T22:45:28Z

integration_tests/__tests__/__snapshots__/multi_project_runner.test.js.snap

 "
 `;

 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


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

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?

yeah! i'll rerecord snapshots in www

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

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

This is not the right place to have this conversation, I commented here: #4183 (comment) cc @thymikee

@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

aaronabramov · 2017-08-22T22:48:43Z

types/Config.js

@@ -15,7 +15,7 @@ export type HasteConfig = {|
  defaultPlatform?: ?string,
  hasteImplModulePath?: string,
  platforms?: Array<string>,
-  providesModuleNodeModules: Array<string>,
+  providesModuleNodeModules: Array<string>


what happened to the formatting?

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

aaronabramov · 2017-08-22T22:49:06Z

packages/jest-cli/src/reporters/get_result_header.js

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


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

I'm not sure, but the behavior here should not change with this PR

cpojer · 2017-08-22T22:53:47Z

Woohoo, this is awesome and @jkassens literally asked for this at FB just two hours ago.

Here is my opinion on the UX:

I think the solution you have in the screenshot is fine: show the displayName for projects where it is there, omit it for the ones that aren't.
This is a tough one. I'd probably print it out as the user specified and not try to uppercase the text in JS. People can choose to use all lowercase, all uppercase, or whatever @aaronabramov prefers for his projects.
Emojis must be supported for sure. 💯

Overall, the three answers all point to: be lenient with the user, and allow them to customize and control this behavior without Jest exerting too much control.

aaronabramov · 2017-08-23T00:19:37Z

or whatever @aaronabramov prefers for his projects.

just let it go haha 😃

what would be the strategy for emoji display name in non-terminal logs though?

// jest.conf.js
{
  displayName: isTTY ? 🦐 : 'i_like_underscores',
}

cpojer · 2017-08-23T00:27:59Z

Uh.. yeah this is kinda bad because the transform cache is dependent on the ProjectConfig, so if this value changes on different runs, the cache will be wiped. I definitely would like to support emoji though :P We could allow both a string and object value for displayName, and normalize to the object in the normalize.js file, and then have something like {interactive: '🐛', default: 'TEXT'}?

rogeliog · 2017-08-23T00:35:14Z

So we are going for string|{default: string, interactive: string} right?

rogeliog · 2017-08-23T00:36:36Z

I wonder if it is better if we flip it, so that we have {default: '🐛', nonInteractive: 'TEXT'}

aaronabramov · 2017-08-23T00:39:16Z

So we are going for string|{default:string, interactive: string} right?

yes for InitialOptions type and only {default: string, interactive: string} for ProjectConfig type

cpojer · 2017-08-23T00:42:51Z

I think default + interactive works for me. I generally don't like negations in variable names.

cpojer · 2017-08-23T00:43:32Z

Possible follow-up PR: should we also print the display name in the test suite typeahead in watch mode?

rogeliog · 2017-08-23T00:47:43Z

Got it, that makes sense!

Integrating this with the typeaheads would be nice!

rogeliog · 2017-08-23T20:02:04Z

@thymikee Maybe I'm missing something, but does jest-validate support mixed types? I'm having some trouble with that

#4327 (comment)

thymikee · 2017-08-23T20:11:38Z

@rogeliog nope :(. For now you'll have to declare just one, without the feedback for the other.

rogeliog · 2017-08-23T20:33:57Z

@cpojer @aaronabramov given @thymikee's comment, should we start with just displayName: string? or should we first add mixed types to jest-validate?

thymikee · 2017-08-23T20:36:19Z

I'd go with displayName: string and refactor later. This week is really busy for me, so I won't be able to implement it in nearest future :D

cpojer · 2017-08-23T20:39:28Z

I'm fine if we merge this first, and then make the change. @aaronabramov can you review this and merge it? You are the expert on this code.

rogeliog · 2017-08-24T23:14:02Z

integration_tests/__tests__/__snapshots__/multi_project_runner.test.js.snap

 "
 `;

 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


@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?

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

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

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!

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

codecov-io · 2017-08-27T05:11:17Z

Codecov Report

Merging #4327 into master will increase coverage by <.01%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #4327      +/-   ##
==========================================
+ Coverage   55.85%   55.85%   +<.01%     
==========================================
  Files         189      189              
  Lines        6383     6393      +10     
  Branches        6        6              
==========================================
+ Hits         3565     3571       +6     
- Misses       2815     2819       +4     
  Partials        3        3

Impacted Files	Coverage Δ
packages/jest-config/src/valid_config.js	`100% <ø> (ø)`	⬆️
packages/jest-config/src/index.js	`0% <ø> (ø)`	⬆️
packages/jest-config/src/normalize.js	`78.26% <ø> (ø)`	⬆️
...ackages/jest-cli/src/reporters/default_reporter.js	`91.04% <100%> (ø)`	⬆️
packages/jest-cli/src/reporters/Status.js	`93.93% <100%> (+0.18%)`	⬆️
...ckages/jest-cli/src/reporters/get_result_header.js	`82.35% <100%> (+3.78%)`	⬆️
packages/jest-cli/src/reporters/utils.js	`60.39% <20%> (-2.11%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db4465e...fdb0d1f. Read the comment docs.

cpojer · 2017-08-27T08:05:22Z

Nice work @rogeliog, thank you!

github-actions · 2021-05-13T07:09:31Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

rogeliog added 4 commits August 22, 2017 10:34

Modify flow types to support displayName

84d73c7

Add project displayName

372a1bb

Some small fixes

417416a

Add tests

a6ae265

facebook-github-bot added the CLA Signed ✔️ label Aug 22, 2017

Fix eslint

1192760

aaronabramov suggested changes Aug 22, 2017

View reviewed changes

cpojer mentioned this pull request Aug 23, 2017

Jest 21 Master Task #4332

Closed

6 tasks

Merge branch 'master' into add-display-name

27a07dc

rogeliog commented Aug 24, 2017

View reviewed changes

rogeliog added 4 commits August 26, 2017 21:47

Better handling for non interactive terminals

dc72c42

Update tests

1084ae9

Merge branch 'master' into add-display-name

1f6f3df

Use supportColor

fdb0d1f

cpojer merged commit e535665 into jestjs:master Aug 27, 2017

rogeliog mentioned this pull request Aug 27, 2017

Docs for project displayName #4379

Closed

palmerj3 mentioned this pull request Sep 1, 2017

Add displayName to TestResult #4408

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add displayName to MPR #4327

Add displayName to MPR #4327

rogeliog commented Aug 22, 2017

aaronabramov left a comment

aaronabramov Aug 22, 2017

cpojer Aug 22, 2017

aaronabramov Aug 22, 2017

aaronabramov Aug 22, 2017

aaronabramov Aug 22, 2017

cpojer Aug 22, 2017

rogeliog Aug 22, 2017 •

edited

Loading

aaronabramov Aug 22, 2017

rogeliog Aug 22, 2017

aaronabramov Aug 22, 2017

rogeliog Aug 22, 2017

cpojer commented Aug 22, 2017

aaronabramov commented Aug 23, 2017

cpojer commented Aug 23, 2017

rogeliog commented Aug 23, 2017 •

edited

Loading

rogeliog commented Aug 23, 2017 •

edited

Loading

aaronabramov commented Aug 23, 2017

cpojer commented Aug 23, 2017

cpojer commented Aug 23, 2017

rogeliog commented Aug 23, 2017 •

edited

Loading

rogeliog commented Aug 23, 2017 •

edited

Loading

thymikee commented Aug 23, 2017

rogeliog commented Aug 23, 2017

thymikee commented Aug 23, 2017

cpojer commented Aug 23, 2017

rogeliog Aug 24, 2017

cpojer Aug 24, 2017

rogeliog Aug 25, 2017 •

edited

Loading

rogeliog Aug 25, 2017 •

edited

Loading

cpojer Aug 25, 2017

codecov-io commented Aug 27, 2017

cpojer commented Aug 27, 2017

github-actions bot commented May 13, 2021

Add displayName to MPR #4327

Add displayName to MPR #4327

Conversation

rogeliog commented Aug 22, 2017

aaronabramov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogeliog Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Aug 22, 2017

aaronabramov commented Aug 23, 2017

cpojer commented Aug 23, 2017

rogeliog commented Aug 23, 2017 • edited Loading

rogeliog commented Aug 23, 2017 • edited Loading

aaronabramov commented Aug 23, 2017

cpojer commented Aug 23, 2017

cpojer commented Aug 23, 2017

rogeliog commented Aug 23, 2017 • edited Loading

rogeliog commented Aug 23, 2017 • edited Loading

thymikee commented Aug 23, 2017

rogeliog commented Aug 23, 2017

thymikee commented Aug 23, 2017

cpojer commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogeliog Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

rogeliog Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 27, 2017

Codecov Report

cpojer commented Aug 27, 2017

github-actions bot commented May 13, 2021

rogeliog Aug 22, 2017 •

edited

Loading

rogeliog commented Aug 23, 2017 •

edited

Loading

rogeliog commented Aug 23, 2017 •

edited

Loading

rogeliog commented Aug 23, 2017 •

edited

Loading

rogeliog commented Aug 23, 2017 •

edited

Loading

rogeliog Aug 25, 2017 •

edited

Loading

rogeliog Aug 25, 2017 •

edited

Loading