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(ignore patterns): add "ignorePatterns" config option #2848

Merged
merged 26 commits into from
May 3, 2021

Conversation

nicojs
Copy link
Member

@nicojs nicojs commented Apr 21, 2021

Add the ability to specify ignorePatterns in your Stryker configuration. This replaces files option which is now deprecated.

By specifying ignorePatterns you choose which files should not be copied to the Sandbox directory (inside the .stryker-tmp directory). This effectively excludes them from being used during mutation testing.

For example:

{
  "ignorePatterns": ["dist"]
}

This will discover all files in your current working directory, except for files matching the "dist" pattern. Stryker uses .gitignore rules to resolve these patterns.

Stryker always adds ['.git', 'node_modules', '/reports', 'stryker.log', '.stryker-tmp'] when resolving these patterns. If you really want them in your sandbox, you can un-ignore them using a ! prefix in your pattern, for example:

{
  "ignorePatterns": ["!node_modules"]
}

The ignorePatterns is now also the default way files are discovered. Previously, Stryker used git ls-files --others --exclude-standard --cached --exclude .stryker-tmp to discover files by default. This had some limitations:

  • ❌ It only worked inside a git repository and you had to have git installed. If not, you got an error and had to use files
  • ❌ If a .gitignore'd file is needed to run your tests, you needed to fall back on files.

Specifying files is now deprecated, but still works for the time being. Stryker will rewrite your file patterns internally to ignore patterns.

Closes #1593
Closes #2739

BREAKING CHANGE: Stryker will no longer use a git command to determine which files belong to your project. Instead, it will rely on sane defaults. You can change this behavior by defining ignorePatterns.

BREAKING CHANGE: The files configuration option is deprecated and will be removed in a future release. Please use ignorePatterns instead.

This:

{
  "files": ["foo.js"]
}

Is equivalent to:

{
  "ignorePatterns": ["**", "!foo.js"]
}

@nicojs
Copy link
Member Author

nicojs commented Apr 21, 2021

@stryker-mutator/stryker-js-and-friends-maintainers What do you think about this? A couple of thoughts:

  • I've decided to not go with a .strykerignore file in the end. It would take us a lot of time and code to maintain, which I think isn't really worth it for this niche feature (who is going to override ignorePatterns anyway? 🤷‍♂️)
  • I've chosen the name ignorePatterns, which is in line with how eslint calls it, but this could potentially be confusing, since users might expect these patterns to be ignored for mutating... ?
  • Should we deprecate or even drop support for "files"? I don't see a reason for users to really use a files array. However, since this was the only way to include non-gitignored files in the past, it might be a bit harsh to out-right deprecate or even remove it.

@nicojs
Copy link
Member Author

nicojs commented Apr 21, 2021

Maybe we should rename it to ignoreFiles, so it's more in line with files itself. However, you might think you can specify a .strykerignore or .gitignore file there 🤷‍♂️

@nicojs nicojs marked this pull request as ready for review April 21, 2021 20:10
@nicojs
Copy link
Member Author

nicojs commented Apr 23, 2021

Ok, I think I know what I want to do.

I think I'll deprecate files. If a user defines files, I'll log a warning and rewrite them to ignorePatterns rules:

{  
  "files": [ "src/**/*.ts", "dist/**/*.js"]
}

becomes

{
  "ignorePatterns": ["**", "!src/**/*ts", "!dist/**/*.js"]
}

@nicojs nicojs marked this pull request as draft April 26, 2021 10:30
@nicojs nicojs marked this pull request as ready for review April 27, 2021 20:39
@nicojs nicojs changed the title feat(ignore patterns): allow to ignore files feat(ignore patterns): add "ignorePatterns" config option Apr 27, 2021
@nicojs nicojs requested a review from simondel April 27, 2021 21:13
@nicojs nicojs added this to the 5.0 milestone Apr 29, 2021
@simondel
Copy link
Member

Changing

{  
  "files": [ "src/**/*.ts", "dist/**/*.js"]
}

into

{
  "ignorePatterns": ["**", "!src/**/*ts", "!dist/**/*.js"]
}

will work, but removing the ignorePatterns all together may also work. Should we let the user know that it might be smart to try running without the suggested change?

When using the config file you can provide an array with `string`s
Default: `[]`<br />
Command line: `--ignorePatterns dist,dist-test`<br />
Config file: `"ignorePatterns": ["dist", "dist-test"]`<br />
Copy link
Member

@simondel simondel Apr 29, 2021

Choose a reason for hiding this comment

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

Perhaps we should also add a coverage folder to this example

Copy link
Member Author

@nicojs nicojs May 2, 2021

Choose a reason for hiding this comment

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

Yeah, good one, I'll replace dist-test with coverage


You can *ignore* files by adding an exclamation mark (`!`) at the start of an expression.
These patterns are **always ignored**: `['node_modules', '.git', '/reports', '/stryker.log', '.stryker-tmp']`. Because Stryker always ignores these, you should rarely have to adjust the `"ignorePatterns"` setting at all. If you want to undo one of these ignore patterns, you can use the `!` prefix, for example: `['!node_modules']`.
Copy link
Member

Choose a reason for hiding this comment

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

Why do you specify reports and stryker.log with a / at the front, but not for other files/folders?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I know for a fact that Stryker places them at the root of your project. For example. I would like the src/reports directory that someone might have to be included. Starting with a / means to only match when it exists in the root of the cwd.

"fileLogLevel": {
"description": "Set the log level that Stryker uses to write to the \"stryker.log\" file",
"$ref": "#/definitions/logLevel",
"default": "off"
},
"files": {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this still be in the schema, but deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

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

As explained during the community meeting, I don't want people to use it. The rewrite to ignorePatterns is being done before the options are validated. This is in line with other deprecated options.

@@ -66,6 +66,7 @@
"file-url": "~3.0.0",
"get-port": "~5.0.0",
"glob": "~7.1.2",
"ignore": "^5.1.8",
Copy link
Member

Choose a reason for hiding this comment

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

Was the ^ intentional here? Or did you mean ~?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, not using it. Will remove 😅

// Inspired by https://github.com/npm/ignore-walk/blob/0e4f87adccb3e16f526d2e960ed04bdc77fd6cca/index.js#L124
const matchesDirectory = (entryName: string, entryPath: string, rule: IMinimatch) => {
return (
matchesFile(entryName, entryPath, rule) ||
Copy link
Member

Choose a reason for hiding this comment

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

This is quite unreadable as you're combining a lot of matches. It might be more readable if the sets are more on one line (for example rule.negate && (rule.match(/${entryPath}, true) || rule.match(entryPath, true))

It might also be smart to place the checks in separate variables so you can actually name what's going on

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we're using prettier I can't play around with the formatting. Separate variables would work, but it would technically cost performance since all matches would always be done (instead of stopping at the first failing match).

I've added a third matchesXxx helper function:

const matchesDirectoryPartially = (entryPath: string, rule: IMinimatch) => {
      // @ts-expect-error Missing overload in type definitions. See https://github.com/isaacs/minimatch/issues/134
      return rule.match(`/${entryPath}`, true) || rule.match(entryPath, true);
    };

This helps a lot I think.

@nicojs
Copy link
Member Author

nicojs commented May 2, 2021

I've added a remark about completely removing "files":

DEPRECATED. Use of "files" is deprecated, please use "ignorePatterns" instead (or remove "files" altogether will probably work as well). For now, rewriting them as ["","!src//*.js","src/index.js"]. See https://stryker-mutator.io/docs/stryker-js/configuration/#ignorepatterns-string

@simondel
Copy link
Member

simondel commented May 3, 2021

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants