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

Group line code-coverage by branch #964

Merged

Conversation

Slamdunk
Copy link
Contributor

@Slamdunk Slamdunk commented Nov 24, 2022

Fix #959

  • Move tests over ExecutableLinesFindingVisitor from RawCodeCoverageDataTest to its own unit test
  • Inline test data expectations to ease readability and maintainability
  • Make test data expectations relative, so new test data can be added before or after the current ones without the need to rewrite other test data
  • Save an associative array of line <=> branch index
  • Add a test for every Control Structure and language definitions (Functions and Classes)
  • Intersect line <=> branch index association with CC driver's output to obtain the final CC
  • Test multiple CC data merge
  • Test PR over existing code bases

Effects on real code bases

Before

image

After

image

As you can see, the difference can be quite high on line coverage: the code base in the shown here uses for example a lot of array declarations; prior to this PR, a single 1.000 array would have produced lines of CC only on non-scalar nodes; this PR instead marks all the 1.000 as executed (or not).

But as we know, line CC is convenient, quick and reliable only as a quantitative result.
Functions/Methods/Classes/Traits coverage has a much higher qualitative value, and this PR has very little effect on results. The difference in these cases are correct and expected: current 9.2.19 release has bugs that this PR fixes.

Special cases

Some PHP structure cannot be deterministically handled well. For example:

return match($var) {
    MyEnum::A => 1,
    MyEnum::B => 1,
};

This code will always depend on autoload runtimes, but in contrast to #892 this will also always produce 4 executable lines from CC drivers.
This PR favour determinism and stability over CC precision, so for the example provided a single branch is reported, and thus all the 4 lines are going to be marked as covered if return has any hit.

If the user needs precise and reliable feedback over CC, only Mutation Testing can help; in the match example it would be MatchArmRemoval mutators from Infection, see https://infection.github.io/guide/mutators.html#Removal-Mutators

@mvorisek
Copy link
Contributor

mvorisek commented Nov 24, 2022

IMHO we should not build ranges like ($node->getStartLine(), $node->getEndLine()) but instead build tree of all possible coverage points (see 2. below)

  1. detect coverage points like now - these are always fine
  2. detect all possible coverage points no matter if const expr or not (ie. to match xdebug coverage /wo any opcache/optimization enabled)
  3. then compute dependent/branched coverage points
    • if one point from group is covered by xdebug, then mark all points as covered
    • if one point from group is not covered (but present) by xdebug, then mark all points as uncovered
    • you need to short circuiting correctly like expr && expr or a ? b : c
  4. the output must never cover more lines than in 2.
  5. must fix CodeCoverage depends on autoload order #889

I am not sure if this can solve every situation, imagine code like:

$a =
mt_rand() === 0
?
'a'
.
'a'
:
'b'
.
'b'
;

the 'a' . 'a' must NOT be marked as covered until really executed (and the chance of it is minimal, as 63 bit random number is very unlikely exactly 0)

if mt_rand() === 0 would be Cl::const instead, there are simply no data to tell which ternary path was taken

also please note

try {fxA()
.
'b'
;} catch (\Throwable) {}

must not mark 'b' as covered when fxA() will throw even if upper branches (try block, function block) are covered

also

$a = true ? 'a' : 'b'
.
'c';

must not mark 'c' as covered, again, there are not enought data (you must check if the data you use are always valid for 100% correct reasoning)

ping me once the PR is done, I can then comment on the impl. and find another test cases

@Slamdunk
Copy link
Contributor Author

IMHO we should not build ranges like ($node->getStartLine(), $node->getEndLine()) but instead build tree of all possible coverage points (see 2. below)

I think the opposite: line code-coverage is by definition wrong, but that's all we have. Starting from the AST is just a waste of time: let's go strait to lines without worrying about the tree.

  1. detect coverage points like now

What to you mean by "now", 9.2 branch or this PR?

the 'a' + 'a' must NOT be marked as covered until really executed

That's obvious, why did you doubt it?

if mt_rand() === 0 would be Cl::const instead, there are simply no data to tell which ternary path was taken

I don't get what you are trying to say: if you format the ternary correctly, we know which path has been taken, and we can mark as green only the taken path.

try {fxA()
.
'b'
;} catch (\Throwable) {}

Let's be clear: line code-coverage is flawed by design. I couldn't care less about this example: if you are collecting the code-coverage with the line CC driver, it is required that you format your code correctly:

try {
    $var = fxA() . 'b';
    $var .= 'c';
;} catch (\Throwable) {}

If fxA() throws an Exception, $var = fxA() . 'b'; is going do be marked as executed, while $var .= 'c'; not.
If a developer likes to write everything in one line, it's not this library's fault if the result is meaningless.

There is no solution that can fix every edge case. Let's focus on providing the best value for the sensible developers.

@mvorisek
Copy link
Contributor

I think the opposite: line code-coverage is by definition wrong, but that's all we have. Starting from the AST is just a waste of time: let's go strait to lines without worrying about the tree.

I don't get what you are trying to say: if you format the ternary correctly,

If a developer likes to write everything in one line, it's not this library's fault if the result is meaningless.

all about the same topic - I agree it is a convention to code that is usually "friendly" to your approach, but I very strongly disagree we should do it that way.

the 'a' + 'a' must NOT be marked as covered until really executed (and the chance of it is minimal, as 63 bit random number is very unlikely exactly 0)

if mt_rand() === 0 would be Cl::const instead, there are simply no data to tell which ternary path was taken

⚠️ this is quite common non-rare example and you must handle it, showing some code path/branch which was never taken as covered is very bad and very hard to debug from user side

  1. detect coverage points like now

What to you mean by "now", 9.2 branch or this PR?

Currently, for multiline statement/expression, we save only 1 coverage line/point when it is really emit by php/xdebug. This should stay IMHO. If something is a part of an expression/statement branch should be based on AST not an expr. start/end line, this has no physical meaning, no such data is emit by xdebug.

Example:

$v = <<<EOF
text
EOF
.
'b';

you should build about this tree for it:

assign (l1)
-> binary concat
-> -> string scalar (l2)
-> -> string scalar (l5)

then you know variable assign op can be covered if and only if the inner expression aka branch has completed, so, the l2 and l5 can be marked as covered if l1 was covered

but when the the code will be formatted like:

$v2 = 'x'; $v = <<<EOF
text
EOF
.
'b';

the tree should be:

assign (l1)
-> string scalar (l1)
assign (l1)
-> binary concat
-> -> string scalar (l2)
-> -> string scalar (l5)

and you cannot transfer any coverage information from assign l1 into l2/l5 as l1 source cannot be used as the coverage can be comming from unrelated statement

@Slamdunk
Copy link
Contributor Author

⚠️ this is quite common non-rare example and you must handle it, showing some code path/branch which was never taken as covered is very bad and very hard to debug from user side

I sincerely have no idea what you're talking about here. Ternary operator gets 4 line coverage from CC drivers:

$var            // 1
    =
    true        // 1
    ?
        'b'     // 1
        :
        'c'     // -1
    ;

Assignment and if-condition are on the same logical branch, and then we have the 2 branches b and c.
We redact 3 distinct branches, and look for their respective usages.
Constant expr and opcache optimisation don't matter here: CC driver will always report the result for at least the first line of each branch.

and you cannot transfer any coverage information from assign l1 into l2/l5 as l1 source cannot be used as the coverage can be comming from unrelated statement

Can you provide a real example? As far as I can tell, L1 and L2/L5 are coupled: you cannot execute one without the other.

I am already taking into account return \ exit \ throw \ goto statements as well as function and method calls to divert a new branch.

@Slamdunk Slamdunk force-pushed the line_cc_grouped_by_branch branch from 1fb092f to 7d6b533 Compare November 30, 2022 10:13
@mvorisek
Copy link
Contributor

@sebastianbergmann I am very disappointed by your review, the covered code percentage might look much better, but the reasoning behind is not done well and can mark more code as covered when in reality it is never executed.

Ocramius added a commit to Ocramius/infection that referenced this pull request Dec 14, 2022
This change reverts some of infection#1773 by allowing the latest PHPUnit coverage tooling to
be installed, for which @Slamdunk fixed coverage in regards of the `infection/infection`
use-case.

Ref: https://github.com/sebastianbergmann/php-code-coverage/releases/tag/9.2.20
Ref: sebastianbergmann/php-code-coverage#964
Ref: sebastianbergmann/php-code-coverage@71525ea
@Ocramius
Copy link
Contributor

Excellent, thanks @sebastianbergmann!

I sent a patch to re-enable infection/infection installation along with this package starting from 9.2.20: infection/infection#1777

Regarding the disagreement on precision

@mvorisek as discussed above, constant expressions are "executed code" from our PoV.

I suggest you propose a change in direction with your own patch/proposal, making it clear that there's a breakage in underlying understanding of coverage, and accompanied with documentation of how to get the more "precise" results (according to your own understanding).

I am aware that @Slamdunk's patch leads to some edge cases in which precision is lost, but practically, the endgame for most people involved in this specific thread is mutation testing regardless, while coverage on its own is an entry point, but nowadays an insufficient quality metric.

From our PoV, getting the mutation testing tooling running is much more important than this precision, at the moment, so some false-positive coverage is worse than the false-negative coverage before this patch. Please be aware of this trade-off.

@sebastianbergmann
Copy link
Owner

@Ocramius 9.2.21 has been tagged: https://github.com/sebastianbergmann/php-code-coverage/blob/9.2.21/ChangeLog.md#9221---2022-12-14

Ocramius added a commit to Ocramius/infection that referenced this pull request Dec 14, 2022
This change reverts some of infection#1773 by allowing the latest PHPUnit coverage tooling to
be installed, for which @Slamdunk fixed coverage in regards of the `infection/infection`
use-case.

Ref: https://github.com/sebastianbergmann/php-code-coverage/releases/tag/9.2.21
Ref: sebastianbergmann/php-code-coverage#964
Ref: sebastianbergmann/php-code-coverage@71525ea
@mvorisek
Copy link
Contributor

@Ocramius I understand this PoV, but still I am not convinced it is good and I am not sure if anyone done review on the actual analyser impl. Fixing the precision will require complete revert of this PR and basically redone this PR from scratch :(

@Ocramius
Copy link
Contributor

@mvorisek and that's fine: I encourage you to go for it, if you have the time/energy for it :)

@mvorisek
Copy link
Contributor

Why I? I spent quite a lot of time on writing the original tests. They were dropped by author of this PR. Shouldn't be he responsible for taking care of them?

@Slamdunk Slamdunk deleted the line_cc_grouped_by_branch branch December 14, 2022 14:02
@Slamdunk
Copy link
Contributor Author

That's by design: that line is considered to be part of variable assignment now, not of the lambda.

Lambda content is correctly reported as non executed indeed.

@mvorisek
Copy link
Contributor

Closing bracket has a meaning. If reached, it means the function has reached the end. When you for example call exit(); in it, it is not reached, and no coverage or red coverage should be shown. Rendering semicolon as covered is useless nor helpful with mutation testing.

Actually any software that honors the standard meaning, can look at the function as covered and it also returns #954 bug to empty closures.

@Slamdunk
Copy link
Contributor Author

Empty methods and functions are handled as expected.
Empty closures can easily be cheated by declaring them as one-liner in any case, so I don't think we should put energies into handling them.
Well formatted closures are handled as expected, and I think the report you linked in https://github.com/atk4/ui/blob/dedb4cea3191e45ba2334ad48c0f98c6d25edf32/src/VueComponent/InlineEdit.php#L85 is good as is.

I'll be more than happy to fix any meaningful bug on closures, if you find one.

@mvorisek
Copy link
Contributor

I would be happy to hear feedback from @sebastianbergmann and @Ocramius too. I stand strongly by the fact that end bracket has meaning in line coverage.

@Ocramius
Copy link
Contributor

Please close the convo here and start a new one.

If you have a test case on which to discuss it, I suggest adjusting that and showing a failure as a start. The closure declaration seems correctly covered there.

maks-rafalko pushed a commit to infection/infection that referenced this pull request Dec 14, 2022
…1777)

This change reverts some of #1773 by allowing the latest PHPUnit coverage tooling to
be installed, for which @Slamdunk fixed coverage in regards of the `infection/infection`
use-case.

Ref: https://github.com/sebastianbergmann/php-code-coverage/releases/tag/9.2.21
Ref: sebastianbergmann/php-code-coverage#964
Ref: sebastianbergmann/php-code-coverage@71525ea
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.

6 participants