Allow array of regexp strings for testRegex #7209

jamietre · 2018-10-18T13:56:15Z

Summary

Resolves #7180
Depends on #7207

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.

This PR allows testRegex to accept an array of regex strings as well as a string, resolving this problem.

Because this is the first configuration option that's overloaded with more than one valid type, jest-validation has also been enhanced with syntax that allows specifying more than one valid example in configuration.

Test plan

existing unit tests pass (except as expected where internal config changed types)
added unit tests to cover valid and error conditions

jamietre · 2018-10-18T14:31:55Z

The CI failure appears to be unrelated or random.

SimenB · 2018-10-18T17:47:01Z

Can you rebase? 🙂 Just doing an interactive one and removing the first commit should be enough, right?

jamietre · 2018-10-18T18:23:28Z

Rebased - I also had a type error in jest-validate which is fixed here, hope that's ok to do here. Not sure why that didn't cause a flow type failure in the last PR, the unit tests have a type that's not compatible with the incorrect type I used on MultipleValidOptions?? (I use TypeScript most of the time so I'm much less familiar with the nuances of flow..)

.vscode/settings.json

CHANGELOG.md

SimenB · 2018-10-18T18:52:17Z

packages/jest-runtime/src/should_instrument.js

-  if (config.testRegex && filename.match(config.testRegex)) {
+  if (
+    config.testRegex &&
+    config.testRegex.some(regex => filename.match(regex))


I know you didn't introduce it, but can you use test instead?

To do this efficiently, it was necessary to change the internal type in ProjectConfig to an array of RegExp instead of string (rather than parse the strings into RegExp on every use here). This makes more sense anyway, but it touched a few things.

packages/jest-validate/src/condition.js

packages/jest-config/src/normalize.js

.vscode/settings.json

SimenB · 2018-10-18T21:31:40Z

packages/jest-validate/src/condition.js

@@ -34,6 +34,6 @@ export function validationCondition(option: any, validOption: any): boolean {
  return getValues(validOption).some(e => validationConditionSingle(option, e));
 }

-export function MultipleValidOptions(...args: Array<any>) {


Oops. Failed rebase.

jamietre · 2018-10-18T22:05:00Z

.vscode/settings.json

@@ -1,5 +1,7 @@
 {
-  "editor.rulers": [80],
+  "editor.rulers": [


This is the most annoying thing.... so when I fixed this, then saved, of course it incorrectly reformatted the fixed file itself!! I keel you vscode

git checkout master .vscode/settings.json

SimenB

These are a couple of fantastic pull requests, thank you so much!

I'd like another set of eyes before merging, but this LGTM 🙂

thymikee · 2018-10-18T22:15:14Z

I'll have one more look tomorrow morning

thymikee

Looks good! I left 2 small things to address and we can ship it :)

packages/jest-config/src/normalize.js

thymikee · 2018-10-19T07:15:54Z

packages/jest-runtime/src/__tests__/should_instrument.test.js

+
+    it('when more than one testRegex is provided and filename is not a test file', () => {
+      testShouldInstrument('source_file.js', defaultOptions, {
+        testRegex: [/.*\_(test)\.(js)$/, /.*\.(test)\.(js)$/, /never/],
      });


I see that we only test happy path in this test file. It would be great to add at least two smoke tests (1 for testRegex, 1 for testMatch) when the file should not be instrumented (e.g. add 4th expected argument to testShouldInstrument with default value of true).

There are already two describe sections in this file, the 2nd -- "should return false" -- tests for scenarios that don't match, the unhappy path. I did add a test there already for testRegex.

I do see that the description of the spec was incorrect, I have fixed that, as well as a couple other existing ones that were also incorrect. Maybe this was throwing you off - either way if there's still some additional coverage you'd like to see let me know.

I have also added a test of error condition to normalize.test.js.

Oh my, I totally missed second testShouldInstrument. Should be renamed to testShouldNotInstrument 😛

thymikee · 2018-10-19T14:03:34Z

packages/jest-config/src/ValidConfig.js

@@ -100,7 +101,10 @@ export default ({
  testMatch: ['**/__tests__/**/*.js?(x)', '**/?(*.)+(spec|test).js?(x)'],
  testNamePattern: 'test signature',
  testPathIgnorePatterns: [NODE_MODULES_REGEXP],
-  testRegex: '(/__tests__/.*|(\\.|/)(test|spec))\\.jsx?$',
+  testRegex: MultipleValidOptions(


also, what do you think about naming this just multiple for simplicity? I'm good with both, so pick whatever you like :)

I don't feel strongly, but I guess I'd lean towards leaving it more verbose to provide clear context, since Mutliple could potentially mean more than one thing.

github-actions · 2021-05-12T07:08:43Z

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.

jamietre requested a review from orta as a code owner October 18, 2018 13:56

facebook-github-bot added the cla signed label Oct 18, 2018

jamietre mentioned this pull request Oct 18, 2018

Allow array of regexp strings for testRegex #7194

Closed

Allow array of regexp strings for testRegex

c2c23f0

jamietre force-pushed the jamiet/multiple-test-regexes-2 branch from fb5b31d to c2c23f0 Compare October 18, 2018 18:21