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] Make missing arc fragments useful to more report generators #1850

Closed
zackw opened this issue Sep 11, 2024 · 8 comments
Closed

[internal] Make missing arc fragments useful to more report generators #1850

zackw opened this issue Sep 11, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@zackw
Copy link
Contributor

zackw commented Sep 11, 2024

This is following on from the discussion near the end of #1846. parser.py has a sophisticated algorithm for characterizing branch arc destinations - ordinary if statement, exiting the function, exiting a with clause, etc. - but the way it's presently implemented, it is only useful to the HTML report generator. To make it useful to more reporters, I'd like to ask for the following:

  • A new FileReporter API, analogous to the existing missing_arc_description function, that tersely describes the destination of an arc without any leading text about whether it was or wasn't executed. For ordinary if destinations, it'd be most useful to get just "line {lineno}". For function returns, "return from {self.name!r}". And so on.
  • Similarly, an exposed API that describes the circumstances where an arc would fail to have been taken - the existing "condition on line {lineno} was never {true/false}", "pattern on line {lineno} {always/never} matched" texts - in isolation, without the rest of the missing_arc_description verbiage, would be useful (not directly to the LCOV reporter, but I bet someone will want it eventually).
  • And finally, an exposed API that tells you whether an arc should be ignored, whether or not it was ever taken or not taken. This is for things like if any(pred(x) for x in xs), where the uninteresting exit arc from the generator expression gets filtered out of the HTML report by (it seems) always getting executed, but a reporter like LCOV that needs to talk about all the arcs has no good way to tell. (Split to [internal] Generator-expression exit arcs aren't always filtered out of executed_branch_arcs #1852.)
@zackw zackw added the enhancement New feature or request label Sep 11, 2024
@nedbat
Copy link
Owner

nedbat commented Sep 11, 2024

I need to understand more about what you are requesting. I've been de-emphasizing arcs to focus more on true branches. I haven't checked the code, but I thought exit arcs from generator expressions were already removed.

@zackw
Copy link
Contributor Author

zackw commented Sep 12, 2024

You know what, this should have been two separate issues. I've split the exit-arcs-from-generator-expressions item to #1852 and I hope I've explained it better there. Let's focus this issue on the first two items.

The code at stake for the first two items is like this

def fn(pred):
    if pred:
        print("yes")
fn(True)
fn(False)

The LCOV reporter (using the code in #1851) currently generates these BRDA: records for this code:

BRDA:2,0,to line 3,1
BRDA:2,0,to exit,1

I want to make "to exit" be "return from 'fn'" instead, the way the HTML reporter would do it. But the problem is that the HTML reporter gets the text "return from 'fn'" from missing_arc_description, which produces a much longer sentence saying that the arc wasn't executed and why. The LCOV reporter only wants the "return from 'fn'" part, and it wants it whether or not the arc was executed, and I can't tell if it's legit to call missing_arc_description at all on arcs that were executed.

So I am suggesting there should be an API more like the internal missing_arc_fragments but exposed (to report generators), and which works on any arc, and uses words that don't imply anything about whether or not the arc was executed. The LCOV reporter only wants descriptions for arc destinations, but I can imagine other report generators might have a use for text describing when arcs would be executed, like the existing "cause" slot of a TArcFragment but reworded.

@nedbat
Copy link
Owner

nedbat commented Sep 22, 2024

I've added an arc_description method that I think does what you need, in branch https://github.com/nedbat/coveragepy/tree/nedbat/arc-description-for-1850. Try it out and let me know.

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

zackw commented Sep 24, 2024

Thanks! I rebased https://github.com/MillionConcepts/coveragepy-fixes/tree/lcov-report-improvements-2 on top of https://github.com/nedbat/coveragepy/tree/nedbat/arc-description-for-1850 and adjusted the code in lcovreport.py to use arc_description. It definitely makes things simpler in the reporter to have this API available. However, it doesn't seem to work for exit arcs. I'm getting "jump to line -1" instead of "return from function 'foo'" consistently. It's supposed to be called via the FileReporter for the source file, right? That wasn't 100% clear to me.

The current batch of test failures for #1851 are because of this (and also #1852).

@nedbat
Copy link
Owner

nedbat commented Sep 25, 2024

I've made a pull request into your branch that fixes things.

@zackw
Copy link
Contributor Author

zackw commented Oct 2, 2024

Have now gotten around to checking your PR; it looks good on my end, let's see how it does in CI...

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 <[email protected]>
nedbat added a commit that referenced this issue Oct 2, 2024
@nedbat
Copy link
Owner

nedbat commented Oct 2, 2024

Fixed in commit 6118798.

@nedbat nedbat closed this as completed Oct 2, 2024
@nedbat
Copy link
Owner

nedbat commented Oct 9, 2024

This is now released as part of coverage 7.6.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants