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

[internal] Generator-expression exit arcs aren't always filtered out of executed_branch_arcs #1852

Closed
zackw opened this issue Sep 12, 2024 · 6 comments
Labels
bug Something isn't working fixed

Comments

@zackw
Copy link
Contributor

zackw commented Sep 12, 2024

Consider this code snippet:

import sys

def foo(args):
    if any(arg.startswith("-") for arg in args):
        print("option found")

foo(sys.argv)

(Any use of a generator expression in the controlling condition of an if should provoke the problem, albeit I have not tried it with a multi-line controlling condition.)

If I run this snippet as coverage run --branch test.py or coverage run --branch test.py nonoption it produces this arc table:

sqlite3 -table .coverage 'select fromno, tono from arc order by fromno, tono'
fromno tono
-4 4
-3 4
-1 1
1 3
3 7
4 -4
4 -3
4 4
7 -1

If I run it as coverage run -branch test.py -flag I get this arc table instead:

fromno tono
-4 4
-3 4
-1 1
1 3
3 7
4 4
4 5
5 -3
7 -1

The problem is with the arc (4, -4), which is (as far as I can tell) the exit arc for the generator expression. The analysis code is supposed to be filtering this arc out as uninteresting, but either it only does this for missing_branch_arcs() and not for executed_branch_arcs(), or there's a bug in its handling of this construct. When the input is the first of the above two arc tables, executed_branch_arcs()[4] is {-4, -3, 4}.

The HTML report generator doesn't care about this because it only looks at missing arcs, but the LCOV report generator looks at both missing and executed arcs and trips over the (4, -4) edge. With the code in #1851, the difference in coverage lcov output for the above two arc tables is

--- coverage.1.lcov	2024-09-12 14:19:43.422893468 -0400
+++ coverage.2.lcov	2024-09-12 14:20:11.648031969 -0400
@@ -2,17 +2,16 @@
 DA:1,1
 DA:3,1
 DA:4,1
-DA:5,0
+DA:5,1
 DA:7,1
 LF:5
-LH:4
+LH:5
 FN:3,5,foo
 FNDA:1,foo
 FNF:1
 FNH:1
-BRDA:4,0,to line 5,0
-BRDA:4,0,to exit 3,1
-BRDA:4,0,to exit 4,1
+BRDA:4,0,to line 5,1
+BRDA:4,0,to exit,0
 BRF:2
 BRH:1
 end_of_record

The BRDA: lines we should be generating are

-BRDA:4,0,to line 5,0
-BRDA:4,0,to exit,1
+BRDA:4,0,to line 5,1
+BRDA:4,0,to exit,0

but I do not see any way to make that happen from inside lcovreport.py.

@nedbat
Copy link
Owner

nedbat commented Sep 15, 2024

Are you sure you are running the code from the tip of master when measuring coverage? There should no longer be a (4, -4) arc in the data, as of commit 2afe04d:

2afe04d1 2024-08-26 refactor: don't test arcs, test branches

zackw added a commit to MillionConcepts/coveragepy-fixes that referenced this issue Sep 16, 2024
@zackw
Copy link
Contributor Author

zackw commented Sep 16, 2024

Yes, I'm sure. I added three tests to the testsuite specifically for this; all of the test failures on the most recent push for #1851 are because in two out of three cases the equivalent of the (4, -4) arc is visible to the lcov reporter.

zackw added a commit to MillionConcepts/coveragepy-fixes that referenced this issue Sep 16, 2024
@zackw
Copy link
Contributor Author

zackw commented Sep 16, 2024

Fine-tuned the tests a little more in commit 402bee8. Given this function

def foo(a):
    if any(x > 0 for x in a):
        print(f"{a!r} has positives")

the genexpr exit arc is pruned by the analysis engine if and only if the arc exiting the whole function is never taken. For example, if foo is never called at all, or if the only call is foo([1]), then the genexpr exit arc will be pruned, but if the only call is foo([0]) or if it's called twice as foo([0]) and foo([1]) then the genexpr exit arc will not be pruned.

CI test results for 402bee8

zackw added a commit to MillionConcepts/coveragepy-fixes that referenced this issue Sep 24, 2024
zackw added a commit to MillionConcepts/coveragepy-fixes that referenced this issue Sep 24, 2024
nedbat added a commit that referenced this issue Oct 2, 2024
* refactor: don't need this aux variable anymore

* feature: add arc_description method for #1850

* [lcov] Refactor LcovReporter.lcov_file

Split the bulk of the code in LcovReporter.lcov_file out into two free helper
functions, lcov_lines and lcov_arcs.  This is easier to read and will make it
easier to do future planned changes in a type-safe manner.

No functional changes in this commit.

* [lcov] Improve reporting of branch destinations.

The branch field of a BRDA: record can be an arbitrary textual label.
Therefore, instead of emitting meaningless numbers, emit the string
“to line <N>” for ordinary branches (where <N> is the arc destination
line, and “to exit” for branches that exit the function.  When there is
more than one exit arc from a single line, provide the negated arc
destination as a disambiguator.

Thanks to Henry Cox (@henry2cox), one of the LCOV maintainers, for
clarifying the semantics of BRDA: records for us.

* Implement function coverage reporting in lcov reports.

Quite straightforward: a function has been executed if any of its region’s
lines have been executed.

* [lcov] Ignore vacuous function regions.

Should fix the test failures with pypy pretending to be python 3.8.

* Adjust test expectations for lcov reports generated under PyPy 3.8.

There is a bug somewhere, in which if we collect data in --branch mode under
PyPy 3.8, regions for top-level functions come out of the analysis engine with
empty lines arrays.  The previous commit prevented this from crashing the lcov
reporter; this commit adjusts the tests of the lcov reporter so that we expect
the function records affected by the bug to be missing.

I don’t think it’s worth trying to pin down the cause of the bug, since Python
3.8 is approaching end-of-life for both CPython and PyPy.

* Split up test_multiple_exit_branches into 3 tests exercising #1852.

* add a fourth case to the tests for #1852

* Revise lcovreport.lcov_arcs using the new arc_description API.

Doesn’t quite work yet, see discussion in #1850.

* fix: forgot to add arc_descriptions to Python filereporter

* refactor: make arc possibilities and arcs executed available

* fix: executed_branch_arcs should limit itself to parsed possible arcs

* Use tests.helpers.xfail_pypy38 for lcov tests that fail with pypy 3.8.

This replaces the custom fn_coverage_missing_in_pypy_38 logic that was
used in earlier commits.

* tests: split xfail_pypy38 decorator into _older_ and _all_ variants

The lcov output tests that are affected by bugs in PyPy 3.8, fail with the
current version of PyPy 3.8 (7.3.11), unlike the other tests annotated with
@xfail_pypy38.  Split this decorator into two variants, xfail_older_pypy38
(used for all the tests that were labeled xfail_py38 prior to this patchset)
and xfail_all_pypy38 (used for the lcov output tests).

---------

Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>
@nedbat
Copy link
Owner

nedbat commented Oct 3, 2024

Is this issue done now also?

@zackw
Copy link
Contributor Author

zackw commented Oct 9, 2024

Thanks for your patience; have only just now had a chance to test the new code against the full application that prompted this report. It generally looks good. There are no longer any cases where the LCOV report includes three or more branch destinations for a single line. There are also no longer any cases where negative arc destination numbers appear in the LCOV output.

I do see a few places where the generic "jump to the function exit" text appears in the LCOV output. I will file follow-up bugs for these later today. I think you can go ahead and mark this issue as resolved in 7.6.2.

@nedbat
Copy link
Owner

nedbat commented Oct 9, 2024

This is now fixed as part of coverage 7.6.2.

@nedbat nedbat closed this as completed Oct 9, 2024
@nedbat nedbat added the fixed label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed
Projects
None yet
Development

No branches or pull requests

2 participants