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

feat: CLI argument to filter tests by projects #8612

Merged
merged 21 commits into from May 10, 2020

Conversation

yacinehmito
Copy link
Contributor

@yacinehmito yacinehmito commented Jun 26, 2019

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
Copy link
Contributor

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 cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

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

@SimenB
Copy link
Member

SimenB commented Aug 20, 2019

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

@yacinehmito
Copy link
Contributor Author

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 Outdated Show resolved Hide resolved
@@ -501,6 +501,13 @@ export const options = {
'rare.',
type: 'boolean' as 'boolean',
},
runProjects: {
Copy link
Member

Choose a reason for hiding this comment

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

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

packages/jest-haste-map/src/lib/FSEventsWatcher.ts Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Nov 23, 2019

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

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

yacinehmito commented Nov 23, 2019

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
Copy link

codecov-io commented Nov 23, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

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
Copy link
Member

SimenB commented Nov 23, 2019

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
Copy link
Contributor Author

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

@yacinehmito
Copy link
Contributor Author

yacinehmito commented Nov 30, 2019

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):

  1. 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).
  2. 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
Copy link
Contributor

jeysal commented Nov 30, 2019

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
Copy link
Contributor

rogeliog commented Dec 2, 2019

@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
Copy link
Contributor Author

yacinehmito commented Dec 8, 2019

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
Copy link
Member

SimenB commented Dec 8, 2019

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

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

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

@SimenB SimenB Dec 9, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not. Doing it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

is this and the feedback below addressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


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

export default function getProjectDisplayName(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 tried and I failed. I can't give a detailed account of the blockers because it was too long ago.

Copy link
Member

Choose a reason for hiding this comment

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

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

@SimenB SimenB requested a review from jeysal December 9, 2019 07:22
@yacinehmito
Copy link
Contributor Author

I like consistency with the watch plugin

Changed it to --selectProjects.

@SimenB SimenB requested a review from thymikee May 8, 2020 07:59
@yacinehmito yacinehmito requested a review from SimenB May 8, 2020 16:32
@dyaa
Copy link

dyaa commented May 8, 2020

🤞

});

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@yacinehmito
Copy link
Contributor Author

@SimenB It's ready for review.

@yacinehmito yacinehmito requested a review from SimenB May 9, 2020 17:50
@SimenB
Copy link
Member

SimenB commented May 9, 2020

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

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

👍

if (argv.selectProjects) {
const namesMissingWarning = getProjectNamesMissingWarning(configs);
if (namesMissingWarning) {
outputStream.write(namesMissingWarning);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 SimenB mentioned this pull request May 9, 2020
@SimenB SimenB changed the title Filter tests by projects feat: CLI argument to filter tests by projects May 10, 2020
@SimenB SimenB merged commit 0e0eeed into jestjs:master May 10, 2020
@SimenB
Copy link
Member

SimenB commented May 10, 2020

Thanks for a great PR and patience @yacinehmito!

@yacinehmito
Copy link
Contributor Author

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

@yacinehmito yacinehmito deleted the run-projects branch May 10, 2020 08:30
@jayarjo
Copy link

jayarjo commented May 19, 2020

Is this feature already a part of some release?

@SimenB
Copy link
Member

SimenB commented May 19, 2020

Nope.

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

jeysal added a commit to jeysal/jest that referenced this pull request May 25, 2020
…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
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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