search

dangreenisrael / eslint-plugin-jest-formatting

ESLint rules for formatting test suites written for jest.
MIT License
156 stars 13 forks source link

Allow configurable test file pattern #50

Closed benkimpel closed 5 years ago

benkimpel commented 5 years ago

Description

The plugin currently identifies a Jest file by checking the filepath for the string "test" or "spec". This probably covers most real world usage, but it could also result in some false positives or false negatives.

False Positives

With false positives the impact is low. It's unlikely that the false positive is also using functions matching Jest names so it will probably just result in wasted time during a lint run.

False Negatives

False negatives will be high impact, because they're the target of the plugin and they're missed.

Proposal

Jest supports a configuration option called testRegex that let's one customize how test files are matched.

It makes sense for this plugin to match its default to the Jest default. It will also be important to allow configuration of the testRegex in this plugin in case any users have configured their Jest to something different.

~A possibly clever or possibly terrible solution could be to see if we could read the Jest config instead of duplicating the configuration ability.~ <= Nah... too brittle.

benkimpel commented 5 years ago

It might make sense to do this in two parts. 1) Use value of Jest default testRegex with case insensitivity and 2) Allow configuration of testRegex.

dangreenisrael commented 5 years ago

Great point, I never considered that someone could use a non-english name for their tests. 🤦‍♂

benkimpel commented 5 years ago

When we get to this one we'll have a decision to make when it comes to how users can configure the test pattern to identify their tests...

(regex in examples is the default Jest pattern to identify tests. just for demonstration purposes)

Option 1 Have them pass it to each rule.

{
  "rules": {
    "padding-around-describe-blocks": ["error", "(/__tests__/.*|(\\.|/)(test|spec))\\.[jt]sx?$"],
    "padding-around-after-all-blocks": ["error", "(/__tests__/.*|(\\.|/)(test|spec))\\.[jt]sx?$"],
    "padding-around-test-blocks": ["error", "(/__tests__/.*|(\\.|/)(test|spec))\\.[jt]sx?$"],
    // And so on for each rule
  }
}

Option 2 Create a padding-around-all rule, but build it dynamically from all other rules so there's no manual maintenance. (We'd define each rule as a set of config constants. We'd pass each to makeRule, but we'd also compose them all into one and pass that to makeRule, too.)

{
  "rules": {
    "padding-around-all": ["error", "(/__tests__/.*|(\\.|/)(test|spec))\\.[jt]sx?$"]
  }
}

Option 3 (ew) Environment variable? Some other weird hack?

Option 4 (hrm) Don't allow configuration. Just use the default Jest regex.

I'll vote for option 2, but I also don't have a strong opinion on this.

benkimpel commented 5 years ago

Important to note... Option 2 wouldn't prevent us from also having a recommended set.

dangreenisrael commented 5 years ago

+1 to Option 2 + recommended set.

benkimpel commented 5 years ago

IF the only reason we were considering padding-around-all was for ease of configuration on file patterns then we might be better served just relying on overrides. As of pretty recently overrides now supports extends, so a user's config could look like this...

{
  plugins: ["jest-formatting"],
  overrides: [
    {
      files: ["**/*.test.js", "**/*.spec.js"],
      extends: ["plugin:jest-formatting/recommended"]
    }
  ]
}

And now the rules will only target those files. I think we can also put this in the recommended config we export.

(My 2 cents on padding-around-all is that there is probably still some value in having it in case a user wants to configure these rules as warnings rather than errors. I also reopened #51 because we missed a few more jest identifiers... so maybe we work in padding-around-all with those missed ones.)

Ok, back to configuring file patterns... overrides seems like the most correct way to do custom file targeting. My guess is that the rules only really receive the file path for the purposes of making assertions against the directory structure or file name anyway.

dangreenisrael commented 5 years ago

Perhaps we could put this in the README then, and close this issue?

benkimpel commented 5 years ago

Yep! @dangreenisrael I'll take care of updating that readme with details above and then close this issue.