Regression in executable line identification for match expressions in 9.2.21
#967
Comments
|
Hi, For that reason I've highlighted in the main topic #964 that the only deterministic and consistent way to handle |
|
Thanks for the quick response! I can certainly appreciate that mutation testing may be the only 100% reliable way to assess code coverage for match arms, but that's asking a lot for users who don't have the means or need to set it up. The issue with enums giving false positives is kinda wild; would that be mitigated if the autoloader was lazier in match expressions? I'm curious what other types of setups result in undesirable behavior for match expressions. Are they a sufficiently small subset that there's still value to providing more precise coverage for simple cases? I'm guessing the answer is no, but I suppose it doesn't hurt to ask. :) |
|
@Slamdunk would you be please so kind to post here analysis of xdebug data that are causing this issue? |
|
Given the following command: $ XDEBUG_MODE=coverage php -r 'require "vendor/autoload.php";xdebug_start_code_coverage(XDEBUG_CC_UNUSED|XDEBUG_CC_DEAD_CODE);(new Foo)->foo("HIT");print_r(xdebug_get_code_coverage()[realpath("src/Foo.php")]);'For which I need to highlight 2 things:
And the following directory structure, the // src/Bar.php
class Bar { public const BAR = 'bar'; }
// src/Baz.php
class Baz { public const BAZ = 'baz'; }
// src/Xyz.php
class Xyz { public const XYZ = 'xyz'; }
// src/Foo.php
class Foo
{
public const FOO = 'foo';
public function foo($var): string
{
return match ($var) { // 1 : green
self::FOO => 'self::FOO', // -1 : RED
Bar::BAR => 'Bar::BAR', // 1 : green
'Baz::BAZ' => Baz::BAZ, // 1 : green
uniqid(1) => 'uniqid(1)', // 1 : green
'uniqid(2)' => uniqid(2), // 1 : green
'HIT' => 'HIT', // 1 : green
Xyz::XYZ => 'Xyz::XYZ', // -1 : RED
uniqid(3) => 'uniqid(3)', // -1 : RED
'uniqid(4)' => uniqid(4), // -1 : RED
default => 'default', // 1 : green
};
}
}So despite only asking for Line Code-Coverage for
Activating Infection requires 3 steps:
If you set up PHPUnit and Code-Coverage, you certainly have the means for it. I understand the psychological burden of adding another tool to the build. @hemberger as far as I can tell, this only affect |
hemberger
changed the title
match expressions in 9.2.21
|
There seems to be a bug in VLD, it doesn't show the "default" jump target in the list of targets (from the original report): The coverage from PHP-CC 9.2.20 looks correct, as cases 0, 2, and the default are tested, and shown in line coverage. I don't know what you have done in 9.2.21, where it is broken.
That seems to be because PHP checks whether the return type of the
That's a nonsense statement. In Slamdunks example, there is no jump table, as the LHS has code running required for evaluations. This is similar to how switch works when it can't use a jump table either: As this code is run, case by case until it matches, these LHS statements The example doesn't talk about, or show, branch coverage, so I can't comment on So the only bug here, is that VLD misses showing the default case, and I've |
|
Thank you, Derick, for the detailed response. |
|
@Slamdunk Do you have an idea how we should proceed here? Thanks! |
|
From my POV, the result from 9.2.20 was correct, and no special hacks should be done to make the whole match being marked as covered. The output of Xdebug is, as far as I can tell from this example, the right one. |
|
@Slamdunk Do you still plan to work on this? Thanks! |
|
In |
|
I think I've just hit this problem with the following code and test: <?php
class Updater
{
public function update(ICLass $item): IClass
{
return match (true) {
$item instanceof ClassA => (
static fn() => new ClassA($item->number + 1)
)(),
$item instanceof ClassB => (static function () use ($item) {
return new ClassB($item->text . ' updated');
})(),
$item instanceof ClassU => new ClassU(array_map($this->update(...), $item->items)),
};
}
}
class ClassBTest extends TestCase
{
public function testThatClassBIsUpdated():void{
$result = (new Updater())->update(new ClassB('test'));
$this->assertEquals(new ClassB('test updated'), $result);
}
}(the convoluted match branches are only for testing) In that scenario, I get 100% coverage for
Specs: Switching to Infection is a poor excuse in my opinion - it completely invalidates the advantage/feature of having code coverage in PHPUnit (I'd rather go back to |
|
@Slamdunk wrote in #967 (comment):
Would it be only ede00c7 that needs to be reverted to address this? That commit's message mentions a dependency on "autoloading runtime behavior". Is this what you mean by "test execution order"? I think it would be better to have a solution that is sometimes wrong due to test execution order / autoloading runtime behavior than having a solution that appears to be always wrong. |
I disagree on this.
@uuf6429 code coverage tells what lines of code have been used by your tests, mutation testing tells if your tests are resilient to code changes and thus effective at all. Your statement doesn't make sense to me. |
I know the difference - your comment and followup comment suggests switching to Infection as a workaround to this bug... The only practical workaround is to replace |
Adding rather than switching, actually
Been there, done that. |
Yes, I would add a note about this to the documentation.
Sorry to bother you, but you did not give an answer to the above question.
I understand your point of view and suspect that you must be even more frustrated about this situation than I am, but I also see sense in the argument made by @uuf6429 and the others here on this ticket. I trust you, Filippo, on your assessment that code coverage of As much as I love Infection, I do not think that it makes sense to suggest to use mutation testing as a workaround for shortcomings when it comes to code coverage for Maybe we can even recommend strategies to reduce the sensitivity to autoloading runtime behavior and test execution order. The first idea that comes to mind would be to use a combination of static loading and dynamic autoloading which is what we use for PHPUnit's PHAR. |
As far as I remember you are correct |
Some valuable precision in the identification of executable lines is lost as a result of #964. I have not reviewed that PR carefully enough to judge its merits, but I see how the concerns raised by @mvorisek can manifest as a regression in real-world test suites. I appreciate the many, many hours of effort by the contributors working on this issue and I hope that we can find a solution that is still friendly to mutation testing without impacting precision.
For example, in 9.2.20, only the branches of a match expression that were actually executed were displayed as covered. In 9.2.21, the entire match expression is now reported as being covered, even though it is not. It was quite useful to know which branches were being exercised by the test.
I'm having a bit of deja vu here, because I reported a nearly identical regression to match expression coverage in #904, which was fixed somewhere between 9.2.13 and 9.2.20! :)
Here is an example class:
And here is the corresponding test:
Coverage report in 9.2.20 (only 3 of the 5 branches are covered, as expected):
Coverage report in 9.2.21 (all branches are marked as covered):
Thank you for your time!
The text was updated successfully, but these errors were encountered: