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

Allow configuration object in projects array #5176

Merged
merged 10 commits into from
Dec 28, 2017
Merged

Conversation

azz
Copy link
Contributor

@azz azz commented Dec 25, 2017

Summary

Closes #5174

Test plan

Added an integration test. Happy to add more if you can think of edge cases.

@azz
Copy link
Contributor Author

azz commented Dec 25, 2017

I'm not familiar with jest-cli/jest-config so let me know if I'm going about implementing it wrong!

): {
configPath: ?Path,
globalConfig: GlobalConfig,
hasDeprecationWarnings: boolean,
projectConfig: ProjectConfig,
} {
let rawOptions;
let configPath;
let configPath = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there anywhere that requires a configPath to be set other than readConfigFileAndSetRootDir?

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

This is pretty cool, I like it a lot! We got a bug report previously that having only one project doesn’t work. Can we, as part of this PR, fix that too? By making it configurable with a custom object, it’s more likely people will end up with a single project while they are iterating on their setup and it would suck if that breaks because of a Jest bug. See #4117

rawOptions.rootDir = parentConfigPath;
} else {
throw new Error(
'jest: Cannot use configuration as an object without a file path',
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 end this sentence with a full stop (.) ?

Also, please capitalize Jest.

@codecov-io
Copy link

codecov-io commented Dec 25, 2017

Codecov Report

Merging #5176 into master will increase coverage by 0.25%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5176      +/-   ##
=========================================
+ Coverage   60.54%   60.8%   +0.25%     
=========================================
  Files         201     201              
  Lines        6707    6707              
  Branches        3       4       +1     
=========================================
+ Hits         4061    4078      +17     
+ Misses       2645    2628      -17     
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-config/src/normalize.js 93.1% <100%> (ø) ⬆️
packages/jest-config/src/index.js 25.92% <38.46%> (+2.11%) ⬆️
...st-config/src/read_config_file_and_set_root_dir.js 0% <0%> (ø) ⬆️
packages/jest-config/src/vendor/jsonlint.js 5.72% <0%> (+5.72%) ⬆️

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 2cb57a0...c9d5fec. Read the comment docs.

@azz
Copy link
Contributor Author

azz commented Dec 25, 2017

I did a bit of digging on that issue and got lost, do we want to treat:

{
  testMatch: ['...']
}

The same as

{
  projects: [{
    testMatch: ['...']
  }]
}

?

@cpojer
Copy link
Member

cpojer commented Dec 25, 2017

Yes, basically. I was thinking Jest should throw if there are ProjectConfig fields defined in the global config if there are one or more projects defined. In absence of a collision, it should just run whatever is specified in the projects settings.

To sum up:

  • No projects setting: use the whole config object for a single test run.
  • 1-N projects setting: throw when ProjectConfig options are part of the initial config (I believe right now we silently ignore those). Run N test runs.

@SimenB any thoughts?

@SimenB
Copy link
Member

SimenB commented Dec 26, 2017

I think we need to go through what's in ProjectConfig and what's in GlobalConfig, they don't really seems focused right now (at least looking at types/Config). I've never thought of the difference when adding options, and I can see that I should have... If we can clean that up, then I'm with you, it makes sense to throw if mixed.

Should we go through the docs as well and specify what we consider global config and what we consider project config? Not sure how to best say it without confusing people who don't care about the difference as they don't use projects. I suppose clear error messages might be enough

@cpojer
Copy link
Member

cpojer commented Dec 26, 2017

Alright, as a start, let’s just make sure a single project specified in the projects settings keeps working then :)

// Whether it needs to look into `--config` arg passed to CLI.
// It only used to read initial config. If the initial config contains
// `project` property, we don't want to read `--config` value and rather
// read individual configs for every project.
skipArgvConfigOption?: boolean,
parentConfigPath?: ?Path,
Copy link
Member

Choose a reason for hiding this comment

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

what does ? on both sides mean? From my understanding name: ?type allows null, undefined or a value, while name?: type allows undefined or a value.

https://flow.org/en/docs/types/primitives/#toc-maybe-types

Copy link
Contributor Author

@azz azz Dec 27, 2017

Choose a reason for hiding this comment

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

I'm not very proficient with Flow. I just added the ? on the LHS to satisfy the checker. Maybe I can remove the one left, but the property is optional so I think I'll get an error when it's not explicitly passed.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think so 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing it worked.

rawOptions = packageRootOrConfig;
rawOptions.rootDir = parentConfigPath;
} else {
throw new Error(
Copy link
Member

Choose a reason for hiding this comment

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

can we have a test for this (mostly for coverage)?

@@ -166,10 +185,3 @@ const getConfigs = (
}),
};
};

module.exports = {
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 on purpose?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it is, you export instead. Cool! 🙂

@SimenB
Copy link
Member

SimenB commented Dec 27, 2017

Red CI, seems like the new test is failing:
image

@azz
Copy link
Contributor Author

azz commented Dec 27, 2017

Yes I know, I still need to investigate a fix for the issue mentioned above

expect(stdout).toEqual('');
});

test('allows a single project', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test passes. Is there another case that needs to be accounted for?

});

const {stdout, stderr} = runJest(DIR);
expect(stderr).toContain('Test Suites: 2 passed, 2 total');
Copy link
Member

@SimenB SimenB Dec 28, 2017

Choose a reason for hiding this comment

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

I realize this probably follows the pattern of the other tests here, but could you use extractSummary from utils and use a snapshot? You get back both summary and rest from it with timestamps stripped

Also, just a quick status === 0 check would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, will do.

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.

@azz
Copy link
Contributor Author

azz commented Dec 28, 2017

Is the runner setting currently undocumented?

@SimenB
Copy link
Member

SimenB commented Dec 28, 2017

Seems like #4240 did not include docs. Feel free to add them 😀

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.

This is great, thanks!

@cpojer cpojer merged commit a439772 into jestjs:master Dec 28, 2017
@cpojer
Copy link
Member

cpojer commented Dec 28, 2017

@azz This is really solid work, thanks so much for this PR. This is such a nice little tweak that’s going to make the multi project runner much more approachable. I’m hoping to see more PRs from you that meaningfully extend Jest with new features.

@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 13, 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.

Allow configuration objects in "projects"
5 participants