test: Introduce Infection for mutation tests

[Wirone]

After recent discussion I decided to verify what is our mutation score 😁. It does look good to be honest with initial setup:

23750 mutations were generated:
   18693 mutants were killed
       0 mutants were configured to be ignored
       0 mutants were not covered by tests
    4581 covered mutants were not detected
      70 errors were encountered
     136 syntax errors were encountered
     231 time outs were encountered
      39 mutants required more time than configured

Metrics:
         Mutation Score Indicator (MSI): 80%
         Mutation Code Coverage: 100%
         Covered Code MSI: 80%

Concerns to address

• Infection is currently the slowest part of our QA workflow (with this initial configuration). We put a lot of work in the past to make our CI faster, so it's a no-go to add a job that takes much longer than the rest (21m30s on M1 with 10 cores). We need to determine if it's possible to improve it.
• There is a lot of S at the beginning, then it freezes for significant amount of time, then it runs further (why?).
• Mutating src/Runner/Runner.php causes some fixture files to be modified, which causes regular test failures and may lead to committing changes that should not be committed.

CC: @maks-rafalko, any suggestion are welcome 😃.

[coveralls]

coverage: 96.035%. remained the same
when pulling 9f5143c on Wirone:codito/mutation-tests
into 2091e39 on PHP-CS-Fixer:master.

[maks-rafalko]

Hello, I'm glad you decided to try it :)

• Infection is currently the slowest part of our QA workflow (with this initial configuration). We put a lot of work in the past to make our CI faster, so it's a no-go to add a job that takes much longer than the rest (21m30s on M1 with 10 cores). We need to determine if it's possible to improve it.

you can start with another approach first: instead of mutating all the files and running all the tests, you can mutate only those lines of code that actually changed (added, updated) in the Pull Request.

Basically, we do exactly that on Infection itself: https://github.com/infection/infection/blob/873cd3335774a114bef9ca93388e623bf362d820/.github/workflows/mt-annotations.yaml#L55

Infection automatically shows github annotations on the lines that were mutated by mutant wasn't killed, so it's quite convenient (example).

This is the fastest approach. Does it make sense?

• There is a lot of S at the beginning, then it freezes for significant amount of time, then it runs further (why?).

Regarding Skipped mutants: please read https://infection.github.io/2020/08/18/whats-new-in-0.17.0/#Skip-S-mutations-that-are-over-specified-time-limit.

Regarding freeze: Skipped mutants are calculated very fast, that's why you see them first, and then others are calculated in different threads step by step, that's why you see some delay. This is ok from Infection point of view.

• Mutating src/Runner/Runner.php causes some fixture files to be modified, which causes regular test failures and may lead to committing changes that should not be committed.

you can exclude any files from mutation process, please see here (exclude section in the config): https://infection.github.io/guide/usage.html

---

I'm happy to help with any issues you may face with integrating Infection, let me know please.

[keradus]

Hello, I'm glad you decided to try it :)

I tried it out for Fixer already few years ago (2019-06), yet it was too many hours to run on CI back then. Eager to see how fast it can be on CI nowadays !

[Wirone]

you can start with another approach first: instead of mutating all the files and running all the tests, you can mutate only those lines of code that actually changed (added, updated) in the Pull Request. (...) This is the fastest approach. Does it make sense?

Of course, I saw it in the docs but wanted to start with full check anyway, just to check how the MSI looks like for our tests 🙂. But it seems like full check is a no-go, so I will take a look at the diff approach. My only doubt is how to approach MSI, because some files may have lower, some may have higher indicator. How should it be done properly? In your case you don't use --min-covered-msi, just --git-diff-lines, so I suppose it's the way to go (no minimal indicator, just find escaped mutants)?

you can exclude any files from mutation process, please see here (exclude section in the config)

Yeah, I saw that. I think I'll try to figure out the exact mutator which causes this, I just did not have time for it yet. For now I was only able to ensure that composer infection -- --filter=src/Runner/Runner.php causes file modifications, so I'll try to narrow it down 🙂.

Thanks for great feedback @maks-rafalko, I'll improve this PR soon and if I have any more doubts, I'll let you know.

[maks-rafalko]

My only doubt is how to approach MSI, because some files may have lower, some may have higher indicator. How should it be done properly? In your case you don't use --min-covered-msi, just --git-diff-lines, so I suppose it's the way to go (no minimal indicator, just find escaped mutants)?

Let me clarify it with more details just to be sure we are on the same page about all the aspects of Infection.

I think we have 2 goals here to use Infection:

1. to understand the current MSI (the quality of the tests) - it's done, MSI=80%. This is our baseline.
2. to control the quality of the tests (MSI) and step by step improve it

How can we control the quality (MSI)?

Only by forcing all the new code to be merged with MSI >= 80%. And from time to time increase this value (to 80.1%, then to 80.2% and so on).

How can we force minimum MSI?

Only by failing the build, thanks to --min-msi / --min-covered-msi (difference is described below). So from my understanding, --min-covered-msi this is a must have option here. And yes, it can be used together with --git-diff-lines and --ignore-msi-with-no-mutations.

What value we should set --min-covered-msi to?

This depends on many factors. Good approach is to start with the baseline and continuously increase it as I described above, failing the builds if MSI < %baseline%.

Another approach is to set it to 100% and do not allow any escaped mutants. But this is quite hard for contributors and maintainers for such a big project - you will also have cases where old code is changed and developers will have to "fix" previous low-MSI code. And it's not that easy for new Infection users - you will have to understand each mutation, learn ignore features and be tolerant to the fact that there can be false-positive, which will take your time.

Why do I suggest to start with at least --git-diff-lines --min-covered-msi=80 --ignore-msi-with-no-mutations?

• --git-diff-lines is for performance - we will only mutate the diff (new and updated lines of code) instead of the whole project. Also, --git-diff-lines will show inline annotations only for the changed code, which is nice with PR-based development.
• --min-covered-msi=80 - new/updated code with MSI < 80% will not be allowed. It means that if we merge a piece of code with MSI >= 80%, then the whole project will have MSI >=80% and in fact will be continuously increasing.
• --ignore-msi-with-no-mutations - if we add/update lines of code that don't produce any mutations, covered MSI will be 0%. This option allows to not fail the build in this case. No mutations -> build is green.

---

I would also start with --only-covered option as you are probably interested in covered MSI, not just MSI.

In short,

• MSI is to force developers write more tests, because uncovered lines of code will decrease MSI.
• Covered MSI is to focus on covered code and improve the quality of the tests that cover it, ignoring uncovered code.

[Wirone]

Thank you @maks-rafalko for the great explanation, I believe we're on the same page here as my vision was more or less as you described 😁. I am just a little afraid about the practice part 😅. In the end, the goal is to provide more tests and raise MSI, but as the baseline MSI (80) is only an average value, some contributions may hit the problem described by you - too low MSI just because of historically not-tested-enough code. In this project only @keradus have rights to merge PRs with failures, but really often in practice most of the reviews and merges are done by me, so without the ability to ignore MSI while merging I can see myself getting stuck with PRs 🤔. I could be forced to help people with achieving proper MSI, guide them or maybe even add tests / change Infection config by myself. It's doable, but far from ideal. From this perspective, the best initial introduction of mutation tests would be if we had inline hints for escaped mutants (to point people and maintainers what can/should be improved), but without failed CI. What do you think about it?

[maks-rafalko]

From this perspective, the best initial introduction of mutation tests would be if we had inline hints for escaped mutants (to point people and maintainers what can/should be improved), but without failed CI. What do you think about it?

Sounds good. You can always add --min-covered-msi later if needed.

[Wirone]

Unfortunately there's the same error on v0.27.10 😟. Currently on mobile, so can't dig much, will take a look later.

[maks-rafalko]

The current failure is because of the old PHPUnit xml configuration file format that needs to be migrated.

vendor/bin/phpunit --migrate-configuration will do the job.

After it, Infection will run (just tried locally).

If we look into other actions, there is always different version of phpunit/phpunit installed. E.g. here it's phpunit/phpunit (9.6.0).

But when we run Infection with PHPUnit on PHP 8.3, phpunit/phpunit (10.5.12) is installed, and the current xml file doesn't pass PHPUnit's schema validation, that Infection executes.

Since phpunit/phpunit ^9.6 || ^10.5.5 || ^11.0.2 is in the composer.json we can run vendor/bin/phpunit --migrate-configuration just before running Infection in GH Action.

Infection doesn't have an option to turn off validation, we do it intentionally to make sure project's PHPUnit configuration is in the correct format to make sure coverage and all other features work properly.

[Wirone]

Guys, it's working 😁

In this run you can see warnings related to escaped mutants because I've made one indentation to a random file causing "change" of 1 whole class. I'll revert that change and I believe PR is ready 🥳.

Thank you @maks-rafalko for your assistance 🍻!

[maks-rafalko]

Looks good from my side. Great start!

[Wirone]

@maks-rafalko 2m27s vs 8s 🥳!

Awesome improvement! 🍻

[maks-rafalko]

This is really awesome 😮🥳

[dkarlovi]

@maks-rafalko 2m27s vs 8s 🥳!

Awesome improvement! 🍻

@maks-rafalko 👏

[keradus]

@Wirone , this failed on master, mind to take a look?
https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/actions/runs/8428502226/job/23081111767#step:12:257

Perhaps it's a matter of using $GITHUB_BASE_REF in non-PR-triggered job?

[Wirone]

Yeah, Infection job should be triggered only for PRs at this point.