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

Add an ability to ignore Mutators for source code line(s) by regular expression #1326

Merged
merged 4 commits into from Sep 16, 2020

Conversation

maks-rafalko
Copy link
Member

@maks-rafalko maks-rafalko commented Sep 7, 2020

Allows to avoid mutating such lines of code as:

- Assert::notNull($variable);

or

- $this->logger->error($message, ['user' => $user]);
+ $this->logger->error($message, []);

and similar if developers are not interested in them.

Example of new config setting usage:

{
    "mutators": {
        "MethodCallRemoval": {
            "ignoreSourceCodeByRegex": [
                "Assert::.*",
                "\\$this->logger.*"
            ]
        }
    }
}

Also, there is a new setting for mutators - global-ignoreSourceCodeByRegex that will be added to all enabled mutators if present. Works similar to global-ignore

{
    "mutators": {
        "global-ignoreSourceCodeByRegex": [
            "Assert::.*"
        ]
    }
}

From an implementation point of view, it's not possible to use IgnoreConfig and IgnoreMutator to ignore mutators for configured lines of code by regex, because, at the step of applying Mutator, we don't have any information about the source line of code.

While we technically can get it (using PrettyPrinter's prettyPrintExpr($expression) function or by passing source code to each mutator and finding the needed line), for me it seems like not the right solution.

This PR tries to filter out mutants before creating Mutant processes, similar to what is being discussed in pre-/post- hooks here #1323

So, before deciding whether we need to ignore mutator or no, we are applying regular expression (if any) to the diff of Mutant. Easy, yet so powerful.

I will continue adding tests/docs after we agree with the implementation.

I see this feature is most requested during the last months, so I would like to release it asap.

Regarding the suggested plugin system - #1323. When we will do it, this implementation can be redeveloped using plugin system/hooks, while for users configuration will be the same, so no BC here.

PS I'm aware of the failed build, it's ok for now since no tests have been updated

@Slamdunk
Copy link
Contributor

Slamdunk commented Sep 8, 2020

I'm very happy with the results. I miss only one functionality: a global-ignoreSourceCodeByRegex, like the already existing global-ignore, as many ignores may be specific to the environment of the mutation, instead of the specific mutation, like the assert one.

@maks-rafalko
Copy link
Member Author

@Slamdunk @sanmai great suggestions, thank you very much. Implemented.

Updated description. Also, added global-ignoreSourceCodeByRegex as you requested.

Ready for review.

@maks-rafalko maks-rafalko marked this pull request as ready for review September 12, 2020 09:46
@maks-rafalko maks-rafalko added this to the 0.18.0 milestone Sep 12, 2020
@maks-rafalko maks-rafalko linked an issue Sep 12, 2020 that may be closed by this pull request
@sanmai sanmai self-requested a review September 13, 2020 08:33
},
"MethodCallRemoval": {
"ignoreSourceCodeByRegex": [
"Assert::.*",
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 we dog-food this feature already?

Copy link
Contributor

Choose a reason for hiding this comment

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

As a non-native English speaker, may I ask you what does this mean?

Copy link
Member

@sanmai sanmai Sep 15, 2020

Choose a reason for hiding this comment

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

Sure. I'm slightly surprised to find that there's even a Wikipedia article for the term.

Eating your own dog food or dogfooding is the practice of an organization using its own product.

What I meant is that Infection could use exactly this regex to limit removal of these calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't we dog-food this feature already?

if we need to - we certainly can. I didn't do any analysis for this though

Copy link
Member

Choose a reason for hiding this comment

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

It can certainly wait.

…expression.

Fixes #697

Allows to avoid mutating such lines of code as:

```diff
- Assert::notNull($variable);
```

or

```diff
- $this->logger->error($message, ['user' => $user]);
+ $this->logger->error($message, []);
```

and so on.

Example of new config setting usage:

```json
{
    "mutators": {
        "MethodCallRemoval": {
            "ignoreSourceCodeByRegex": [
                "Assert::",
                "\\$this->logger"
            ]
        }
    }
}
```
src/Configuration/ConfigurationFactory.php Outdated Show resolved Hide resolved
src/Configuration/ConfigurationFactory.php Outdated Show resolved Hide resolved
@sanmai
Copy link
Member

sanmai commented Sep 16, 2020

If we can extend #1327 to allow injecting arbitrary configuration for a plugin, this feature is a great candidate for a standard plugin. Nevertheless I think this idea shouldn't be holding anything up since it should be possible to refactor this ignorer into a plugin without changing the configuration schema in a bit.

@sanmai
Copy link
Member

sanmai commented Sep 16, 2020

CS Fixer is going nuts 😞

@maks-rafalko
Copy link
Member Author

maks-rafalko commented Sep 16, 2020

CS Fixer is going nuts 😞

yeah, it's because it's downloading v2.15.8 instead of v2.16.4 as the latest PHAR for https://cs.sensiolabs.org/download/php-cs-fixer-v2.phar URL

2.16.4 works as expected locally. I will try to resolve it as a separate issue somewhere later today

@maks-rafalko
Copy link
Member Author

If we can extend #1327 to allow injecting arbitrary configuration for a plugin, this feature is a great candidate for a standard plugin.

Agree. That was my plan - to release this one ASAP, and then, when we are ready with the plugin system (which can take some time) - we will refactor internal implementation.

@maks-rafalko
Copy link
Member Author

maks-rafalko commented Sep 16, 2020

Regarding php-cs-fixer: there is an open issue, let's see how they will resolve it: PHP-CS-Fixer/PHP-CS-Fixer#5099

I think if they wont' fix it today, we can think about downloading particular hardcoded version for the time being

@sanmai
Copy link
Member

sanmai commented Sep 16, 2020

I did composer require --dev friendsofphp/php-cs-fixer for local CS works.

@sanmai
Copy link
Member

sanmai commented Sep 16, 2020

Shall we merge this?

@maks-rafalko maks-rafalko merged commit ee40e43 into master Sep 16, 2020
@maks-rafalko maks-rafalko deleted the feature/ignore-code-by-regex branch September 16, 2020 07:16
@maks-rafalko
Copy link
Member Author

Thank you @sanmai & @Slamdunk for great review

@sanmai sanmai mentioned this pull request Sep 21, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow adding mutation exclusions for specific calls
3 participants