PHPUnit 9.3 compatibility issue

[sanmai]

Question	Answer
Infection version	0.16.x, master
Test Framework version	PHPUnit 9.3
PHP version	n/a
Platform	n/a

PHPUnit 9.3 comes with a change of terminology and structure, mainly going away from blacklist/whitelist, but not only.

Infection has to change provided phpunit.xml to run each and every mutation. So far I can tell these changes are in the clear.
The problem is in the changes Infection adding to collect coverage during the initial test. Specifically it will add <filter> tag like below, even if there is PHPUnit 9.3 compatible <coverage> directive:

  <coverage>
    <include>
      <directory>path/to/src</directory>
    </include>
  </coverage>
+  <filter>
+    <whitelist>
+      <directory>path/to/src</directory>
+    </whitelist>
+  </filter>

Signs of legacy configuration:

 <phpunit disableCodeCoverageIgnore="...">
 <phpunit ignoreDeprecatedCodeUnitsFromCodeCoverage="...">
 <phpunit cacheTokens="...">
 <filter><whitelist>...
 <logging><log type="...">

Signs of updated configuration:

<coverage>
<logging><*> <!-- where * isn't 'log' -->

Possible solution

There are several consideration:

• One way to tackle this issue is to check for <coverage> element before adding <filter> element. If it so happens that this phpunit.xml is being used with earlier version of PHPUnit that isn't aware of this new element, then this will be caught during validation against xsd.
• Additionally we have to deal with the case where there isn't <coverage> or <filter> elements. In this case we can check if vendor/phpunit/phpunit/phpunit.xsd mentions <coverage> element, and then decide to add either, but to be honest I'd rather see Infection fail with an explanation.
• Since Infection does not depend on PHPUnit, we are not guaranteed to find phpunit.xsd. This does not say that we cannot have a negative dependency, that is a future version of Infection can be in conflict with earlier versions of PHPUnit by standard means offered by composer.json.

Alternative solution

As for failing with a message, Infection could check if there are <coverage> or <filter> elements, and if not, fail with an explanation instead of adding such elements.

Workaround

Current workaround is to run PHPUnit to collect coverage before running Infection like so:

vendor/bin/phpunit --coverage-xml=build/logs/coverage-xml --log-junit=build/logs/junit.xml
vendor/bin/infection --skip-initial-tests --threads=$(nproc) --coverage=build/logs --show-mutations --no-interaction 

I'd like to think this is the right way to run Infection during CI, but there might be other opinions.

Another workaround

If you do not plan to use PHPUnit older than 9.3, there's another workaround. One problem here is that Infection tries to validate updated phpunit.xml, but it uses newest schema where PHPUnit will be happy to use older one. When we instruct Infection to use older schema, it just works. Just like so:

-    xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd"
+    xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/schema/9.2.xsd"

This workaround requires the least amount of effort if you don't mind being nagged by PHPUnit about "--migrate-configuration" now and then.

If you do plan to use PHPUnit 9.2 or earlier together with 9.3, then you would probably need to inject something like this somewhere in your CI pipeline:

test -f vendor/phpunit/phpunit/schema/9.2.xsd || 
    mkdir -p vendor/phpunit/phpunit/schema && 
    cp vendor/phpunit/phpunit/phpunit.xsd vendor/phpunit/phpunit/schema/9.2.xsd

---

Thanks to @terabytesoftw for pointing at these changes.

[sebastianbergmann]

Infection has to change provided phpunit.xml to run each and every mutation. So far I can tell these changes are in the clear.

I would assume so, too. To be sure: which changes does Infection make to phpunit.xml for this?

On a related note: are you missing CLI options that you could use instead of manipulating the XML configuration file?

The problem is in the changes Infection adding to collect coverage during the initial test.

I am not convinced that it is Infection's responsibility to configure code coverage when the project Infection is used for does not configure code coverage in its XML configuration file for PHPUnit.

[sanmai]

I would assume so, too. To be sure: which changes does Infection make to phpunit.xml for this?
On a related note: are you missing CLI options that you could use instead of manipulating the XML configuration file?

It does two sets of changes, one is to remove certain things like logging out of way, and add our own bootstrap procedure, and second set is about adding a list of tests we want to run to tests against certain mutation. Last time I checked it wasn't possible to specify a list of files to run from the command line, only a directory.

Indeed, some of the things we have to do here have a corresponding command line option, but I think since we have to alter phpunit.xml to remove things either way, it is just easier to do in one place.

[dominikzogg]

@sanmai done, thanks #1289

[lcobucci]

Indeed, some of the things we have to do here have a corresponding command line option, but I think since we have to alter phpunit.xml to remove things either way, it is just easier to do in one place.

@sanmai based on what I saw in the code Infection is only removing the existing loggers/printers. How these affect Infection's behaviour? Apart from that, what's the minimum PHPUnit version that Infection offers compatibility?

@sebastianbergmann I see PHPUnit offers the argument --stderr to write to STDERR instead of STDOUT (similar to stderr="true") but is there anything to disable that behaviour - if it was enabled via the configuration file?

[sebastianbergmann]

@sebastianbergmann I see PHPUnit offers the argument --stderr to write to STDERR instead of STDOUT (similar to stderr="true") but is there anything to disable that behaviour - if it was enabled via the configuration file?

No.

[sanmai]

but is there anything to disable that behaviour - if it was enabled via the configuration file?

We have several cases where we disable/enable certain things from the configuration file.

About removing the existing printers, I can speculate as to why but @maks-rafalko might know a definite answer.

[sanmai]

Apart from that, what's the minimum PHPUnit version that Infection offers compatibility?

I remember there was a discussion, but I don't remember seeing a conclusion. But I might be biased towards no conclusion since I routinely use Infection with the likes of PHPUnit 6. But if we support PHPUnit 8, supporting earlier versions should require no effort.

[sanmai]

@sanmai based on what I saw in the code Infection is only removing the existing loggers/printers. How these affect Infection's behaviour?

Right. When Infection removes loggers, it does so for performance reasons. If you have a coverage logger, PHPUnit will activate the machinery to collect coverage data, which in turn will make everything slower.

$ phpdbg -qrr vendor/bin/phpunit  | grep Time
Time: 778 ms, Memory: 16.00 MB
$ phpdbg -qrr vendor/bin/phpunit --coverage-text | grep Time
Time: 1.33 seconds, Memory: 62.00 MB

Let's consider what Infection does. It takes a file, and makes a small change to it. Then it runs a bunch of tests to see if they notice the change. And repeats this process over and over. If you leave coverage loggers this will quickly add up, at worst cases making all things twice as slow.

@lcobucci Makes sense?

[lcobucci]

@sanmai sure thing, I was just trying to find the right balance for this.

My suggestion to solve this issue is a mix of everything we discussed:

1. We keep the generation of the PHPUnit config file but never adding anything regarding coverage reports.
   This is, somewhat, a BC-break since it also means not to create the filter or coverage configuration (this should be provided by the project)
2. The initial coverage retrieval should be done via CLI arguments (since they're pretty much compatible across all versions).

What do you think?

[sanmai]

I'm on board with this idea because:

• It solves the root compatibility problem.
• I tend to think if our users are intelligent enough to grasp the concept and the importance of mutation testing, surely they can manage to configure PHPUnit to collect coverage.
• This is not going to be the final state of affairs, we can add several checks and questions here and there to guide a user if the happen to have not configured PHPUnit properly.

@theofidry @maks-rafalko What do you think?

[theofidry]

@sanmai sorry it's been a while, at what point and in what case do we try to configure the coverage? I thought this was purely for the loggers (which IMO we should be the ones configuring)

[sanmai]

Under "configure the coverage" I mean the loggers and the filtering, where the latter is going to be called just coverage moving forward. They have changed very substantially in 9.3.

The problem here is that we're configuring them using old syntax, which kind of works sometimes, but sometimes it does not, and that's a problem because it'll get convoluted very quickly if we try to work around it straight away.

[theofidry]

I am quite conflicted here.

I see where you guys are coming from and we can question this whole PHPUnit manipulation. Instead we could generate one at the init step and then require the user to keep it up to date, Infection would use it directly. This is another direction which has its pros and cons.

Meanwhile for now we are configuring PHPUnit ourselves. Regarding the filtering: we are explicitly asking the user which files to cover (the source directories) so IMO requiring the user to configure his PHPUnit config to match this (and in case one forgets, gets a big disparity in the result without any warning) is a non insignificant step back from a UX.

An alternative solution: we know the PHPUnit version used, since we check it to ensure we are working with a supported version (especially useful when using the PHPUnit PHAR or using the Infection PHAR). So maybe there is a way to pass that version and adjust the filtering settings depending of the version used?

[lcobucci]

we know the PHPUnit version used, since we check it to ensure we are working with a supported version (especially useful when using the PHPUnit PHAR or using the Infection PHAR). So maybe there is a way to pass that version and adjust the filtering settings depending of the version used?

@theofidry not really, running PHPUnit 9.3 with the old format works. The issue happens for users who have already updated the config file because Infection mixes up the old and new formats.

[theofidry]

Then it should be easy to inspect if there is already a configuration in place and either adjust it or leave it untouched (since the user already configured it) no?

[lcobucci]

@theofidry have you seen the patch? It does what you're suggesting in a way that's compatible with all versions.
Honestly it's quite a hacky workaround and, as @sanmai said, opens a can of worms.

I'll leave the decision up to you folks, I'm changing all my libs to generate the coverage with phpunit directly to avoid this issue, so I won't put much pressure on the matter.

Let me know if I can help in any way.

[theofidry]

Oh totally missed that patch thanks.

I can't say I find it pretty, but looks ok to me and it's also a quite easily testable class so I could live with it. But I'm quite deep a rabbit hole at work with very dirty and obscure workarounds and issues so maybe it makes this workaround look much better than it is...

[sanmai]

I've added a description of another workaround. It seems to be working fine at the moment, but if anyone wishes to test it, I'd be happy to hear any feedback.

[sanmai]

@sebastianbergmann Is it reasonable to expect that PHPUnit 10 won't accept legacy configuration format?

[sebastianbergmann]

Yes.