feat: CLI argument to filter tests by projects #8612

yacinehmito · 2019-06-26T21:59:37Z

Summary

This PR should close #7542.

The idea is to introduce a runProjects CLI argument so that a user can selectively run tests on provided projects.

Test plan

An integration test ensures that the proper tests are run. It will be augmented as we discover new cases.

Current status

Currently, the pull request only has the definition for the new CLI argument and failing e2e tests.

Things to do:

Validate runProjects API
~~Determine how to flag projects as running or not running.~~ Not applicable given our approach.
Determine if and how we can reuse the config field name to identify the projects. A: We will not; we will use displayName.
Determine how to select for projects that are only defined with a path. A: We will not; you just can't filter on a project that has no name.

I will update the description of this PR every time I make meaningful progress.

As this is the first time I'm contributing to jest, I might need some help to figure out where to find stuff.

facebook-github-bot · 2019-06-26T21:59:51Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2019-06-26T22:09:41Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

SimenB · 2019-08-20T20:56:01Z

Hey @yacinehmito, sorry about the lack of feedback here! Is there anything in particular you're stuck on?

yacinehmito · 2019-11-23T09:29:56Z

Sorry I did not come back to this.

I wanted to have the API validated (i.e. the runProjects CLI argument). I'll move forward with it today; I hope it's ok.

packages/jest-cli/src/cli/args.ts

SimenB · 2019-11-23T09:33:49Z

packages/jest-cli/src/cli/args.ts

@@ -501,6 +501,13 @@ export const options = {
      'rare.',
    type: 'boolean' as 'boolean',
  },
+  runProjects: {


We can go with this for now, and bikeshed before landing 🙂

packages/jest-haste-map/src/lib/FSEventsWatcher.ts

SimenB · 2019-11-23T09:35:12Z

@yacinehmito wonderful, thank you! Not sure about the name, but that's easy to change once we're happy with the implementation 🙂

yacinehmito · 2019-11-23T10:45:53Z

Thank you very much for the fast response.

I have an initial implementation working.
I have also rebased on master and removed the alias.

I'd like a preliminary review to determine the following:

Is filtering at the level of runCLI the right approach?
Should we think about whitespace edge cases? Example: I have a project called my-project and the other called __my-project, where the underscores are whitespace. How is this going to affect the behaviour of runProjects? Should we trim the names (if it's not already done) and validate the unicity of project names?
We don't validate that each name passed to runProjects does belong to one of the projects. Is it something we should do? If so, I don't really know where to put that logic; I don't like the idea of throwing in getConfigsOfProjectsToRun because of this.
What should be the behaviour for unnamed projects?

Also, the filtering relies on the name attribute for projects; however the part of the documentation that mentions projects exclusively uses displayName in the examples. It is something we should consider changing?

(There is a bunch of TODO still in the PR that can be disregarded for now. I will also remove the commits related to the VSCode settings and the yarn.lock if they shouldn't be merged)

codecov-io · 2019-11-23T10:50:24Z

Codecov Report

Merging #8612 into master will decrease coverage by 0.15%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8612      +/-   ##
==========================================
- Coverage   63.92%   63.77%   -0.16%     
==========================================
  Files         293      297       +4     
  Lines       12496    12526      +30     
  Branches     3083     3092       +9     
==========================================
  Hits         7988     7988              
- Misses       3863     3893      +30     
  Partials      645      645

Impacted Files	Coverage Δ
...ackages/jest-core/src/getConfigsOfProjectsToRun.ts	`0.00% <0.00%> (ø)`
packages/jest-core/src/getProjectDisplayName.ts	`0.00% <0.00%> (ø)`
...ges/jest-core/src/getProjectNamesMissingWarning.ts	`0.00% <0.00%> (ø)`
packages/jest-core/src/getSelectProjectsMessage.ts	`0.00% <0.00%> (ø)`

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 0a4e4a1...d5a681c. Read the comment docs.

yacinehmito · 2019-11-23T13:54:32Z

I managed to answer my own questions.

Is filtering at the level of runCLI the right approach?

Very likely. The final name of the projects is given by readConfigs which is the first function called in readCLI. Then it calls _run that does the heavy-handed logic so by that point filtering should already be done. It can't be in readConfigs either because this function should only read the configs. It shouldn't know and shouldn't get to decide what runs.

Should we think about whitespace edge cases?

Turns out it doesn't matter. It's easy to include whitespace in the CLI (e.g. jest --runProjects ' my-project').

We don't validate that each name passed to runProjects does belong to one of the projects. Is it something we should do? If so, I don't really know where to put that logic

Exiting because of one project has not been found doesn't seem like a good experience. If in the future we provide the ability to filter projects with globs we wouldn't throw because one glob hasn't had a match. Instead we could report on what was found so the user can compare. I have added this.

What should be the behaviour for unnamed projects?

Nothing in particular. Jest will create a random name (an md5 hash, found it here). You just can't filter if you don't name the project.

Other matters

There is nothing that guarantees the unicity of project names. I don't know if this is something we'd like to check for. I've done it in my form just in case and included it just after endureNoDuplicateConfigs. It is close to the place where we get the final names for the projects and it fits "thematically". If it's desirable I can open a PR for this.

About the displayName issue, I'm suggesting that we do the following:
If a Jest instance runs multiple projects, for each project that has an explicit name (not a hash) but not an explicit displayName, we set displayName to name by default.
Then we can update the documentation and put name instead of displayName in the relevant examples. I can also open a PR for this.

SimenB · 2019-11-23T14:11:20Z

Thanks for taking the time to really think this through! 👏

Is filtering at the level of runCLI the right approach?

Very likely. The final name of the projects is given by readConfigs which is the first function called in readCLI. Then it calls _run that does the heavy-handed logic so by that point filtering should already be done. It can't be in readConfigs either because this function should only read the configs. It shouldn't know and shouldn't get to decide what runs.

Inside _run makes sense to me, as that's where we do async filtering etc. So right at the top of _run seems sensible

Should we think about whitespace edge cases?

Turns out it doesn't matter.

Perfect 🙂

What should be the behaviour for unnamed projects?

Nothing in particular. Jest will create a random name (an md5 hash, found it here). You just can't filter if you don't name the project.

Yeah, that seems reasonable.

There is nothing that guarantees the unicity of project names. I don't know if this is something we'd like to check for. I've done it in my form just in case and included it just after endureNoDuplicateConfigs. It is close to the place where we get the final names for the projects and it fits "thematically". If it's desirable I can open a PR for this.

Yeah, a separate PR for that sounds lovely!

About the displayName issue, I'm suggesting that we do the following:
If a Jest instance runs multiple projects, for each project that has an explicit name (not a hash) but not an explicit displayName, we set displayName to name by default.
Then we can update the documentation and put name instead of displayName in the relevant examples. I can also open a PR for this.

I think having both name and displayName is confusing. I wonder if ignoring name (renaming it to id or something in the future) and just going by displayName makes sense? name is internal and (afaik) undocumented, whilst displayName is documented.

/cc @thymikee @jeysal thoughts here?

yacinehmito · 2019-11-23T14:40:03Z

I'll move the filtering and hold off until we know whether to use name or displayName.

yacinehmito · 2019-11-30T21:37:55Z

Allow me to kindle the conversation. I'll sum up the issue quickly:

Context: We want to be able to specify which projects to run. However projects are currently identified in two ways:

An undocumented name property, with an auto-generated fallback, that uniquely identifies a project. As far as I know this is not really used.
A documented displayName property, that can take either a string or an object, and that decides whether there should be a label next to the test names when they are running; and, if there is such a label, how it should look like.

Problem: When specifying which projects to filter on, we don't know whether we should use the name or the displayName. None seem perfectly suited for the task: name is undocumented and displayName, by its name and function, is geared toward display.

Possible solutions (at the moment):

Document the name property. PRO: Consistent and straightforward. CON: Contributes to config bloat; no value beyond filtering; may clash with the internal use of the property (although unlikely).
Use displayName. PRO: Already known, so requires next to no configuration for users. CON: There's "display" in the name but it will now be used for more than just displaying.

@SimenB suggests the second solution; and, if I may share my opinion, while it's an imperfect solution it's clearly the better one.

@thymikee @jeysal Thoughts? (Sorry for pinging you, I'd just like to move forward with this)

jeysal · 2019-11-30T21:55:58Z

I'd go with displayName, I think even though it's "geared towards display" it will in practice be fine, even if people don't name their project kebab case test-frontend (which I'm sure many devs do), jest --runProjects 'Test Frontend' isn't that bad either.

rogeliog · 2019-12-02T18:11:55Z

@jeysal I agree. This is what we are using for in the watch plugin for selecting projects. https://github.com/jest-community/jest-watch-select-projects

yacinehmito · 2019-12-08T11:54:30Z

I just changed the code so that it uses displayName.
I did not move the filtering logic to _run though because I needed to access argv.runProjects to know whether to display the list of projects. This message can be removed at your request if you don't like it.

The PR is ready for review. Also, let me know if you want a cleaner commit history.

SimenB · 2019-12-08T12:12:47Z

Also, let me know if you want a cleaner commit history.

We squash merge, so separate commits is preferred (especially as we review).

I'll try to review this later today

packages/jest-cli/src/cli/args.ts

SimenB

Thanks, this turned out pretty small and contained! I'm also fine with the runProjects name

SimenB · 2019-12-09T07:09:45Z

packages/jest-core/src/getConfigsOfProjectsToRun.ts

+  const namesOfProjectsToRun = new Set<string>(argv.runProjects);
+  return projectConfigs.filter(config => {
+    const name = getProjectDisplayName(config);
+    return name && namesOfProjectsToRun.has(name);


Should we print a warning if name is not defined? If you're using this option you should (probably) also set a name for all your projects

It is not. Doing it right now.

This is now addressed here: https://github.com/yacinehmito/jest/blob/1c35afb7a9e7265d9547f031b8ec6e1272824390/packages/jest-core/src/getSelectProjectsMessage.ts#L33

We can discuss the wording of the warning.

SimenB · 2019-12-09T07:10:55Z

packages/jest-core/src/getConfigsOfProjectsToRun.ts

+  if (!argv.runProjects) {
+    return projectConfigs;
+  }
+  const namesOfProjectsToRun = new Set<string>(argv.runProjects);


Might make sense to warn if the name here doesn't match any project

is this and the feedback below addressed?

We display: "No project to run". https://github.com/yacinehmito/jest/blob/1cd4d1a28f7d9898941005779c849d09482f7109/packages/jest-core/src/getProjectsRunningMessage.ts#L15

I think it's not enough; how do you think the warning should be worded?

Suggestion: "You provided values for --selectProjects but no projects were found matching the selection".

Might be too verbose, and I don't know if the 2nd second person form is suitable.

UPDATE: Changed the code with the suggestion above. Let's discuss wording.

SimenB · 2019-12-09T07:18:45Z

packages/jest-core/src/getProjectDisplayName.ts

+
+import {Config} from '@jest/types';
+
+export default function getProjectDisplayName(


This looks like https://github.com/facebook/jest/blob/d7a7b4294a4507030f86fe4f78e1790f53d0bda9/packages/jest-reporters/src/utils.ts#L18-L34 without the color. Can we share the code somehow?

Thinking about it, normalize should probably spit out the object form with the default white color, so we'd only have to deal with the one form inside jest

That's what I was telling myself when I wrote this. However this bit of code is in jest-reporters and I didn't know if or where I should move the code.

Leveraging normalize makes more sense to me. I'll try that.

I tried and I failed. I can't give a detailed account of the blockers because it was too long ago.

Opened up #10010 for this btw, so we don't forget 🙂

yacinehmito · 2020-05-07T22:22:47Z

I like consistency with the watch plugin

Changed it to --selectProjects.

e2e/__tests__/selectProjects.test.ts

dyaa · 2020-05-08T16:33:27Z

🤞

SimenB · 2020-05-09T11:49:59Z

e2e/__tests__/selectProjects.test.ts

+  });
+
+  describe('when Jest is started with `--selectProjects third-project`', () => {
+    const result = runWithJson('select-projects', [


can you stick this inside the test? describe runs before tests start. can combine the 2 its, I guess, or stick the runWithJson inside a beforeAll

SimenB · 2020-05-09T11:50:55Z

e2e/__tests__/selectProjects.test.ts

+      'third-project',
+    ]);
+    it('does not run any tests', () => {
+      expect(result.json.success).toBe(true);


should be false, shouldn't it? If you pass a non-existing name, I think we should fail

We do fail. Here it is true because we also provide --passWithNoTests. If we don't we can't get JSON output.

I'll change the tests to look for failure though.

In the error cases we don't need JSON, we can just do the normal run (no json) and verify exitCode and stderr

yacinehmito · 2020-05-09T17:50:14Z

@SimenB It's ready for review.

SimenB · 2020-05-09T19:04:05Z

This is great! @jeysal @thymikee wanna look it over?

thymikee

👍

thymikee · 2020-05-09T19:19:15Z

packages/jest-core/src/cli/index.ts

+  if (argv.selectProjects) {
+    const namesMissingWarning = getProjectNamesMissingWarning(configs);
+    if (namesMissingWarning) {
+      outputStream.write(namesMissingWarning);


we usually go with console.warn for warnings, but I don't think it matters this much here

console.warn will always write the string to stdout, whereas with outputStream we will write to either stdout or stderr depending on what is appropriate.

Here, if I replace outputStream.write by console.warn the tests will fail as they rely on the JSON output, and the JSON output can only be valid if all messages are written to stderr instead of stdout.

SimenB · 2020-05-10T07:34:04Z

Thanks for a great PR and patience @yacinehmito!

yacinehmito · 2020-05-10T08:30:13Z

I can thank you for your patience just as much! Thanks a lot for allowing me to contribute to Jest. 🤗

jayarjo · 2020-05-19T12:33:03Z

Is this feature already a part of some release?

SimenB · 2020-05-19T12:41:41Z

Nope.

You can look for it here: https://github.com/facebook/jest/releases

…esolve-outside * upstream/master: (106 commits) docs: fix jest-diff example (jestjs#10067) Cleanup `displayName` type (jestjs#10049) docs: correct confusing filename in `enableAutomock` example (jestjs#10055) chore: minor optimize getTransformer (jestjs#10050) chore: fix TestUtils.ts to match the types (jestjs#10034) Minor test name typo fix (jestjs#10033) chore: upgrade to typescript 3.9 (jestjs#10031) feat: CLI argument to filter tests by projects (jestjs#8612) chore: bump `istanbul-lib-instrument` (jestjs#10009) feat: support config files exporting (`async`) `function`s (jestjs#10001) fix: add missing haste-map dep to jest-snapshot (jestjs#10008) 🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉 (jestjs#10000) Fix typo in dependency warning (jestjs#10006) chore: add missing comma (jestjs#9999) fix: Control no diff message color with diff options (jestjs#9997) fix(jest-jasmine2): fix Error message (jestjs#9990) docs: fix jest-object ids for docusaurs (jestjs#9994) docs: fix Configuration, JestPlatform and JestObjectAPI docs for 26 (jestjs#9985) Add migration notes for breaking changes (jestjs#9978) chore: fix date and heading in blog post (jestjs#9977) ...

github-actions · 2021-05-11T06:06:52Z

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.

facebook-github-bot added the cla signed label Jun 26, 2019

SimenB reviewed Nov 23, 2019

View reviewed changes

packages/jest-cli/src/cli/args.ts Outdated Show resolved Hide resolved

SimenB reviewed Nov 23, 2019

View reviewed changes

yacinehmito force-pushed the run-projects branch 2 times, most recently from b067bc6 to 3ed9fff Compare November 23, 2019 10:37

yacinehmito force-pushed the run-projects branch from 3f6024c to 7289f31 Compare November 23, 2019 13:54

yacinehmito force-pushed the run-projects branch from 7289f31 to f33e371 Compare December 8, 2019 11:50

yacinehmito marked this pull request as ready for review December 8, 2019 11:54

yacinehmito force-pushed the run-projects branch 2 times, most recently from cf2a8ce to 64768d6 Compare December 8, 2019 12:03

yacinehmito requested a review from SimenB December 8, 2019 12:05

yacinehmito mentioned this pull request Dec 8, 2019

Expose a way for jest-cli to run jest with a specific project(s) #7542

Closed

niieani reviewed Dec 8, 2019

View reviewed changes

packages/jest-cli/src/cli/args.ts Outdated Show resolved Hide resolved

SimenB approved these changes Dec 9, 2019

View reviewed changes

SimenB requested a review from jeysal December 9, 2019 07:22

Fix when there are no missing name

100ccdd

SimenB requested a review from thymikee May 8, 2020 07:59

SimenB reviewed May 8, 2020

View reviewed changes

e2e/__tests__/selectProjects.test.ts Show resolved Hide resolved

Fix warnings and add tests

36e7294

yacinehmito requested a review from SimenB May 8, 2020 16:32

yacinehmito added 2 commits May 8, 2020 18:34

Added advice in docs

d5a681c

Merge remote-tracking branch 'upstream/master' into run-projects

8102c94

SimenB reviewed May 9, 2020

View reviewed changes

yacinehmito added 2 commits May 9, 2020 19:32

Merge remote-tracking branch 'upstream/master' into run-projects

5550bc8

Refactor tests

52eaa67

yacinehmito requested a review from SimenB May 9, 2020 17:50

yacinehmito added 2 commits May 9, 2020 19:52

Fix typo in test

67a66f8

Fix more typos

c0262e9

SimenB approved these changes May 9, 2020

View reviewed changes

thymikee approved these changes May 9, 2020

View reviewed changes

SimenB mentioned this pull request May 9, 2020

Cleanup displayName type #10010

Closed

SimenB changed the title ~~Filter tests by projects~~ feat: CLI argument to filter tests by projects May 10, 2020

SimenB merged commit 0e0eeed into jestjs:master May 10, 2020

yacinehmito deleted the run-projects branch May 10, 2020 08:30

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


		import {Config} from '@jest/types';

		export default function getProjectDisplayName(

feat: CLI argument to filter tests by projects #8612

feat: CLI argument to filter tests by projects #8612

Conversation

yacinehmito commented Jun 26, 2019 • edited Loading

Summary

Test plan

Current status

facebook-github-bot commented Jun 26, 2019

facebook-github-bot commented Jun 26, 2019

SimenB commented Aug 20, 2019

yacinehmito commented Nov 23, 2019

Choose a reason for hiding this comment

SimenB commented Nov 23, 2019

yacinehmito commented Nov 23, 2019 • edited Loading

codecov-io commented Nov 23, 2019 • edited Loading

Codecov Report

yacinehmito commented Nov 23, 2019

Other matters

SimenB commented Nov 23, 2019

yacinehmito commented Nov 23, 2019

yacinehmito commented Nov 30, 2019 • edited Loading

jeysal commented Nov 30, 2019

rogeliog commented Dec 2, 2019

yacinehmito commented Dec 8, 2019 • edited Loading

SimenB commented Dec 8, 2019

SimenB left a comment

Choose a reason for hiding this comment

SimenB Dec 9, 2019 • 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

yacinehmito May 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacinehmito Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

yacinehmito May 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacinehmito commented May 7, 2020

dyaa commented May 8, 2020

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

yacinehmito commented May 9, 2020

SimenB commented May 9, 2020

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented May 10, 2020

yacinehmito commented May 10, 2020

jayarjo commented May 19, 2020

SimenB commented May 19, 2020

github-actions bot commented May 11, 2021

yacinehmito commented Jun 26, 2019 •

edited

Loading

yacinehmito commented Nov 23, 2019 •

edited

Loading

codecov-io commented Nov 23, 2019 •

edited

Loading

yacinehmito commented Nov 30, 2019 •

edited

Loading

yacinehmito commented Dec 8, 2019 •

edited

Loading

SimenB Dec 9, 2019 •

edited

Loading

yacinehmito May 7, 2020 •

edited

Loading

yacinehmito Dec 9, 2019 •

edited

Loading

yacinehmito May 7, 2020 •

edited

Loading