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 configuring using an array of testRegex #7180

Closed
jamietre opened this issue Oct 16, 2018 · 13 comments · Fixed by #7209
Closed

Allow configuring using an array of testRegex #7180

jamietre opened this issue Oct 16, 2018 · 13 comments · Fixed by #7209

Comments

@jamietre
Copy link
Contributor

jamietre commented Oct 16, 2018

🚀 Feature Proposal

Creating dynamic configurations to include or exclude specific things is difficult because of inconsistency in how config allows specifying inclusions and exclusions.

  • testRegex allows only a single regexp
  • testMatch allows an array of globs
  • testPathIgnorePatterns allows an array of regexps

So there's no format (regex or glob) that can be used to build an array that works both for including and excluding.

It would be very convenient if either there were an analog to testMatch for ignoring (an array of globs), or it were permissible to provide an array of regexps instead of only a single one for testRegex.

I think the easiest solution to implement would be simply accepting an array or a single regexp for testRegex. Additionally, the regexp array convention is much widely used already (coveragePathIgnorePatterns, modulePathIgnorePatterns, transformPathIgnorePatterns, watchPathIgnorePatterns, testPathIgnorePatterns). Alternatively, a new config option testPathPatterns could be introduced that only accepts an array, for consistency with all the ignore options, but it seems redundant.

I'd like to take a shot at coding it, but wanted to vet the feature idea before doing any work.

Motivation

Motivation is creating simple configuration that can be used to run buckets of tests that are dynamically specified by pattern matching. For example, I would like to be able to specify patterns that match slow tests, and be able to produce dynamic config that can both exclude only slow tests, or run only slow tests.

Example

jest.config.js

const mode = process.env.JEST_MODE;
let testRegex = [/__tests__/\.tsx*$/];
const testPatternIgnorePaths = []'

const slowTests = /__tests__/.*\.slow\.tsx*$/;

switch(mode) {
   case 'slow':
       testRegex = [slowTests];
       break;
   case 'fast':
      testPatternIgnorePaths.push(slowTests);
      break;
   case default: // err
}
      
module.exports = {
  testRegex,
  testPatternIgnorePaths,
  ...
}

Pitch

Why does this feature belong in the Jest core platform?

It would be great for creating optimized configurations in large test environments. It makes the way file matching patterns are specified in configuration more consistent as well.

@SimenB
Copy link
Member

SimenB commented Oct 16, 2018

@rickhanlonii @thymikee we just discussed config overhaul as it's quite the hot mess, and the mess that is regex vs glob, separate ignore fields etc should definitely be on the chopping block.

A single issue going through all config options (and CLI options) might make sense, although I guess file matching is big enough to be discussed on its own (e.g. in this issue).


So to be on topic; for file matching, of the top of my head, this is my suggestion: coveragePatterns: Array<Glob>, testPatterns: Array<Glob>, transformPatterns: Array<Glob> and remove everything else.

@jamietre
Copy link
Contributor Author

Using globs exclusively is definitely preferable imho. I was going for a quick fix here; but I'm definitely in favor of standardization if breaking changes are on the table.

@rickhanlonii
Copy link
Member

Short term I think it makes sense for testRegex to accept either a string or array 👍

@jamietre
Copy link
Contributor Author

If you all are amenable to that solution for now I can take a crack at it, it doesn't seem like it would be a big change.

@SimenB
Copy link
Member

SimenB commented Oct 16, 2018

Doing config.testRegex = Array.isArray(config.testRegex) ? config.testRegex : [config.testRegex]; in https://github.com/facebook/jest/blob/master/packages/jest-config/src/normalize.js should be easy enough to land, yeah

@rickhanlonii
Copy link
Member

All you @jamietre!

@jamietre
Copy link
Contributor Author

jamietre commented Oct 17, 2018

I have this working, except that in https://github.com/facebook/jest/blob/master/packages/jest-config/src/normalize.js line 348, we call validate which validates by matching types from an objectVALID_CONFIG: https://github.com/facebook/jest/blob/master/packages/jest-config/src/ValidConfig.js

There's no obvious way to make this work without adding features to the validator. I can think of lots of ways to make the validator handle multiple types (and indeed implemented one), but there are consequences beyond just accepting more than one option.

For example, when validation fails, it shows an example based on the ValidConfig object, and that would all have to be revised to support some syntax to show the multiple options correctly when there are errors. (Right now it just emits the key). This isn't undoable by any means but not sure it's a in the scope of what any of us were thinking as far as the impact this change would have. If you all are OK with changing the validator this way I'll finish it up, but this seems like it probably falls within the area that's under discussion for an overhaul anyway.

Other options I can think of are just skip validation for testRegex or add a new config option testRegexArray instead of overloading testRegex, and just map it in normalize to the internal testRegex. The second option seems to muddy the waters further though since you're looking at overhauling options processing anyway. Or maybe add it but leave it undocumented... what do you think.

@SimenB
Copy link
Member

SimenB commented Oct 17, 2018

Mind pushing what you have to a branch so we cab play with it?

@thymikee thoughts on the validation?

@jamietre
Copy link
Contributor Author

jamietre commented Oct 17, 2018

Here's the branch on my fork; lmk if I should make a pull request. (ignore launch.json changes - the default settings resulting in formatting being wrong every time I save!)

https://github.com/jamietre/jest/commits/jamiet/multiple-test-regexes

@thymikee
Copy link
Collaborator

I think I like the "MultipleValidOptions" addition to jest-validate, makes sense imho. Send a PR :)

@jamietre
Copy link
Contributor Author

OK, sounds good... I still need to fix up how validation errors involving multiple valid options are reported, will do that & send it on.

@rubennorte
Copy link
Contributor

Linking to #7185. This is something we'll probably want to include there

SimenB pushed a commit that referenced this issue Oct 18, 2018
## Summary

Split from #7194
Prerequisite to resolving #7180

This PR provides a syntax for `jest-validate` to accept more than one example for a single config value, which can be of different types. This allows overloading config options to accept multiple types.

## Test plan

- existing unit tests pass (except as expected where internal config changed types)
- added unit tests to cover new valid and error conditions
@github-actions
Copy link

This issue 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 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants