Add color to displayName in project configuration. #8025

natealcedo · 2019-03-03T07:33:17Z

Summary

The displayName configuration under project configuration is a cool feature. One thing I have an issue with it is that it's not immediately apparent which project a test is from without reading the displayName. Adding an option to colorize this field makes it visually apparent and I think that it's a nice to have.

Hence, I'm just opening up this PR to get feedback on this feature if you guys think it's something that's worth adding. Since we use chalk underneath to do the coloring, I thought why not just open up the whole suite of colors for developers to choose. I'd like to highlight that some developers are already asking for colors in jest diff as in #7980 so I'm sure someone will ask for this. If not, I myself would like this feature.

If you guys like think this is worth adding, it would be good to have a discussion on whether or not to limit which colors we should allow. Here's a screenshot of me having two projects and setting their colors to blue and magenta.

Cheers!

SimenB · 2019-03-03T12:47:53Z

packages/jest-types/src/Config.ts

@@ -223,6 +224,25 @@ type NotifyMode =
  | 'success-change'
  | 'failure-change';

+type DisplayNameColor =


Would be good to extract these from chalk instead of hard coding them

Oh I was hard coding this because of what I mentioned about not being sure if we should curate the colors that we should allow or if we should allow everything. Hence the early PR 😄

I've taken a look at the types provided by chalk and they don't expose the colors. https://github.com/chalk/chalk/blob/master/index.d.ts#L231

Just as an FYI, opened up this PR. Hopefully this gets merged and we won't have to hardcode colors here. chalk/chalk#336

SimenB · 2019-03-03T12:48:29Z

packages/jest-types/src/Config.ts

@@ -313,7 +333,8 @@ export type ProjectConfig = {
  dependencyExtractor?: string;
  detectLeaks: boolean;
  detectOpenHandles: boolean;
-  displayName: string | null | undefined;
+  displayName?: string;


I wonder if we should do displayName?: string | {name: string; color: DisplayNameColor} instead of adding a new option?

Oh, I like this implementation better actually.

I like the API that @SimenB is suggesting.

SimenB · 2019-03-03T12:48:49Z

I like the idea!

/cc @thymikee @rogeliog @jeysal

jeysal · 2019-03-03T13:02:32Z

I also love the idea! We could also consider doing this automagically without configuration changes, which is what Lerna does.

SimenB · 2019-03-03T13:15:01Z

We could also consider doing this automagically without configuration changes, which is what Lerna does.

I think I'd like to keep colors consistent. Or maybe I get used to lint being magenta, tests being green (of course) and compiling being yellow? It'd be nice to not rely on ordering.

If colors are not specified though, I'm definitely in favor of assigning different colors to each automatically

thymikee · 2019-03-03T17:30:55Z

I'm a fan of what @SimenB says (API- and UI-wise)

jeysal · 2019-03-03T18:32:55Z

Fine with configuration overrides too.
For the defaults we could hash and modulo the displayName || name to pick something from a set of colors consistently, not based on ordering.
Or try to be smart and look at runner to pick a consistent color for jest-runner-tsc, jest-runner-eslint, etc.?

natealcedo · 2019-03-03T23:42:53Z

I agree with @SimenB. I'd like to have a sane set of defaults but yet allows developers to override.

thymikee · 2019-03-18T15:49:35Z

ping @natealcedo :)

natealcedo · 2019-03-19T04:29:47Z

Oh hey sorry I haven't had the chance to take a look at this again. It doesn't seem like there was a conclusion to the discussion above though. To get this PR moving forward, should I start taking a look at what runners we have and think about what colors to assign them as a default? Thoughts @jeysal @SimenB?

SimenB · 2019-03-19T10:19:07Z

I don't think jest should set a default for runners, I just meant that I as a user should be able to set colors so it's consistent in a project

jeysal · 2019-03-19T10:35:18Z

The default color should be stable though, so jest-runner-asdfghjkl should get the same color by default every time.

natealcedo · 2019-03-19T11:39:30Z

So going by what @SimenB says, let's just keep the default color to white then and if a consumer provides an override in his project configuration, we choose what the consumer wants? I've given it some thought and I've come to the conclusion that I don't quite like having jest predetermine colors for runners. Seems like we're overthinking it IMO by going down the path of implementing a color wheel.

jeysal · 2019-03-19T11:43:55Z

If you've got 5 packages with jest-runner, jest-runner-eslint, and jest-runner-tsc each, having your 15 Jest projects grouped into blue tests, red lints, green typechecks or whatever would be nice IMO

natealcedo · 2019-03-19T12:46:25Z

That's a point but doesn't jest NOT group the output by runner? I'm maybe wrong here but I'm just speaking from the top of my head.

jeysal · 2019-03-19T12:53:43Z

What do you mean by grouped? I'm referring to the colors as a means of grouping, even though the individual output lines may be interspersed.

natealcedo · 2019-03-19T13:34:22Z

@jeysal Ah yes, I was confused about what you said initially. It seems like we're on the same page then.

To carry on the discussion, it seems now that we're going about this slightly different. The purpose of this PR is to enable users to change the color output of a project by way of adding in a displayNameColor or displayName object as per what @SimenB has suggested in projectConfiguration. What you're suggesting entails reading the runner option in a project configuration and if it's not the default jest-runner AND displayNameColor is not set, we set default colors for the runner based on the output of hashing and modulo-ing the runner from a list of colors in a color wheel. Is that correct?

rogeliog · 2019-03-19T14:35:57Z

I guess that most repos that use projects and displayNames, don’t use custom runners, but rather multiple jest configurations depending on how their repo is structured.(for example, server vs client).

rogeliog · 2019-03-19T14:38:54Z

What if we start by adding the color as an opt-in feature, and then continue the “smart default” discussion post merge?

natealcedo · 2019-03-19T14:55:14Z

Sure thing. That sounds like a reasonable compromise to get this feature moving. I don't have the bandwidth currently but let me take a look at it over the weekend.

jeysal · 2019-03-19T15:21:59Z

I'd be fine with starting out as a strictly opt-in feature, of course.

What you're suggesting entails reading the runner option in a project configuration and if it's not the default jest-runner AND displayNameColor is not set, we set default colors for the runner based on the output of hashing and modulo-ing the runner from a list of colors in a color wheel. Is that correct?

Yes, essentially (minus the "if it's not the default jest-runner", don't see why that would matter)

natealcedo · 2019-03-19T15:31:47Z

Oh I was thinking that way because we’d want to keep the default behaviour as is if it’s the default runner. But then I realised that it wouldn’t matter since the implementation ideally should do the same thing.

…playNameColor

natealcedo · 2019-03-25T11:59:52Z

Just pushed in a couple of updates and added tests. You guys think this needs an integration test or is testing printDisplayName good enough? What else does this PR need besides updating the CHANGELOG? I'm guessing since this a feature, some docs are going to need an update. Could someone point me in the right direction? :)

SimenB · 2019-03-26T09:53:30Z

Just pushed in a couple of updates and added tests. You guys think this needs an integration test or is testing printDisplayName good enough?

An integration test for displayName in general would be nice. We also replace colors in the snapshots, so you'll get nice <red>name</> in the snapshots

I'm guessing since this a feature, some docs are going to need an update. Could someone point me in the right direction? :)

Doesn't seem like displayName is documented - it should be. Mind adding it (to Configuration.md)? In docs/ it should match the logic as of this PR, while in website/versioned_docs it should match the behavior (single string only) currently published

natealcedo · 2019-03-26T09:57:48Z

An integration test for displayName in general would be nice. We also replace colors in the snapshots, so you'll get nice name</> in the snapshots

Will do so. I'll just update the projects configuration integration test and chuck in a displayName with a color

Doesn't seem like displayName is documented

I found this. Though it's just a small hint about it https://jestjs.io/docs/en/configuration#projects-array-string-projectconfig

SimenB · 2019-03-26T09:58:44Z

I found this. Though it's just a small hint about it jestjs.io/docs/en/configuration#projects-array-string-projectconfig

Yeah, it should have its own section (even though it's essentially just useful if you have multiple projects)

…playNameColor

natealcedo · 2019-03-26T12:55:36Z

Hey, I'm facing an issue with integration testing.
https://github.com/facebook/jest/blob/master/e2e/runJest.ts#L74
We don't allow colors in integration testing.

I've tried adding in an option to force color in runJest. Even then, the snapshot isn't accurate.

The colors you see at the top is me printing stderr,
The actual snapshot is like this


exports[`displayName should print out colors 1`] = `
"</><inverse><bold><green> PASS </></></></> </><inverse> SERVER </></></> <dim>backend/</><bold>a.test.js</>
</><inverse><bold><green> PASS </></></></> </><inverse> CLIENT </></></> <dim>client/</><bold>b.test.js</>

<bold>Test Suites: </><bold><green>2 passed</></>, 2 total
<bold>Tests:       </><bold><green>2 passed</></>, 2 total
<bold>Snapshots:   </>0 total
<bold>Time:</>        <<REPLACED>>
<dim>Ran all test suites</><dim> in </>2<dim> projects</><dim>.</>"
`;

natealcedo · 2019-03-26T13:25:37Z

I've updated the docs and the changelog. I think this should be in a good enough shape to receive a full review. I'm opting not to add an integration test due to difficulties getting snapshots to work with it since runJest doesn't support colors and I've added in unit tests for the changes.

SimenB

This is awesome, thanks!

SimenB · 2019-03-26T15:57:42Z

website/versioned_docs/version-24.0/Configuration.md

@@ -289,6 +289,12 @@ The `extract` function should return an iterable (`Array`, `Set`, etc.) with the

 That module can also contain a `getCacheKey` function to generate a cache key to determine if the logic has changed and any cached artifacts relying on it should be discarded.

+### `displayName` [string, object]


just string

natealcedo · 2019-03-26T16:23:48Z

One last set of changes to just add a new line and a colon for validation error in normalize.

github-actions · 2021-05-11T20:07:08Z

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.

Alcedo Nathaniel De Guzman Jr added 4 commits March 3, 2019 10:03

WIP

e3bf4a3

Update jest-types to have displayNameColor

eefc4ff

Update default value to white

e0f09e2

Add displayNameColor to Initial Options

ec8b7c9

facebook-github-bot added the cla signed label Mar 3, 2019

SimenB reviewed Mar 3, 2019

View reviewed changes

Alcedo Nathaniel De Guzman Jr added 3 commits March 23, 2019 10:39

Merge remote-tracking branch 'origin/master' into feature/support-dis…

7bb02fc

…playNameColor

Delete Config.js

a810c31

Remove displayNameColor

789cb8b

Alcedo Nathaniel De Guzman Jr added 3 commits March 26, 2019 19:29

Merge branch 'master' into feature/support-displayNameColor

ec0d0d0

Remove test scenario which will never happen

68a6ae4

Merge remote-tracking branch 'origin/master' into feature/support-dis…

d1fe278

…playNameColor

Alcedo Nathaniel De Guzman Jr added 3 commits March 26, 2019 21:16

Update docs with new displayName behavior

87dc8cf

Update versioned docs

f1dbd72

Update changelog

aada1fb

natealcedo changed the title ~~[WIP] Add color to displayName in project configuration.~~ dd color to displayName in project configuration. Mar 26, 2019

natealcedo changed the title ~~dd color to displayName in project configuration.~~ Add color to displayName in project configuration. Mar 26, 2019

Alcedo Nathaniel De Guzman Jr added 3 commits March 26, 2019 21:49

Update snapshot

bea007a

Lint fix

8928a2e

Lint md fix

c1e5a38

SimenB approved these changes Mar 26, 2019

View reviewed changes

Alcedo Nathaniel De Guzman Jr added 3 commits March 27, 2019 00:12

Respond to feedback

4462019

Add new line

21b99bb

Update snapshot

3665d38

SimenB merged commit bd34500 into jestjs:master Mar 26, 2019

jeysal mentioned this pull request Jun 17, 2019

fix displayName object configuration jest-community/jest-watch-select-projects#8

Merged

jeysal mentioned this pull request Jul 15, 2019

set default display name color based on runner #8689

Merged

snyk-bot mentioned this pull request Dec 15, 2019

[Snyk] Upgrade babel-jest from 24.0.0 to 24.9.0 jakeherp/burger-builder#14

Open

This was referenced Mar 21, 2020

[Snyk] Upgrade: jest, jest-cli lwojcik/sc2pte-panel-frontend#88

Closed

[Snyk] Upgrade jest from 24.0.0 to 24.9.0 DavidKindler/burger-builder#47

Merged

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

		@@ -289,6 +289,12 @@ The `extract` function should return an iterable (`Array`, `Set`, etc.) with the

		That module can also contain a `getCacheKey` function to generate a cache key to determine if the logic has changed and any cached artifacts relying on it should be discarded.

		### `displayName` [string, object]

Add color to displayName in project configuration. #8025

Add color to displayName in project configuration. #8025

Conversation

natealcedo commented Mar 3, 2019 • edited Loading

Summary

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

SimenB commented Mar 3, 2019

jeysal commented Mar 3, 2019

SimenB commented Mar 3, 2019

thymikee commented Mar 3, 2019

jeysal commented Mar 3, 2019

natealcedo commented Mar 3, 2019

thymikee commented Mar 18, 2019

natealcedo commented Mar 19, 2019

SimenB commented Mar 19, 2019

jeysal commented Mar 19, 2019

natealcedo commented Mar 19, 2019

jeysal commented Mar 19, 2019

natealcedo commented Mar 19, 2019

jeysal commented Mar 19, 2019

natealcedo commented Mar 19, 2019

rogeliog commented Mar 19, 2019

rogeliog commented Mar 19, 2019 • edited Loading

natealcedo commented Mar 19, 2019

jeysal commented Mar 19, 2019

natealcedo commented Mar 19, 2019

natealcedo commented Mar 25, 2019 • edited Loading

SimenB commented Mar 26, 2019

natealcedo commented Mar 26, 2019 • edited Loading

SimenB commented Mar 26, 2019

natealcedo commented Mar 26, 2019 • edited Loading

natealcedo commented Mar 26, 2019

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natealcedo commented Mar 26, 2019

github-actions bot commented May 11, 2021

natealcedo commented Mar 3, 2019 •

edited

Loading

rogeliog commented Mar 19, 2019 •

edited

Loading

natealcedo commented Mar 25, 2019 •

edited

Loading

natealcedo commented Mar 26, 2019 •

edited

Loading

natealcedo commented Mar 26, 2019 •

edited

Loading