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

Lcov report fixes v2 #1849

Merged
merged 4 commits into from
Sep 11, 2024
Merged

Conversation

zackw
Copy link
Contributor

@zackw zackw commented Sep 9, 2024

Fixes #1846 (per my understanding of the lcov file format) and addresses several other issues. See individual commit logs for details. Should be ready to merge.

Changes from #1847:

  • Verified that the lcov report for real partially covered code is sensible
  • Adjusted handling of BRDA: based on what genhtml does with it on real code
  • "File checksums" mode removed; now there's just a toggle for line checksums
  • Passes tox locally with Python 3.8 through 3.12 and PyPy installed
  • I think I have fixed the formatting issues you were concerned with; if I still didn't get it quite right please point me at specific sections you'd like formatted differently

Zack Weinberg added 4 commits September 9, 2024 14:47
- Rename LcovReporter.get_lcov to LcovReporter.lcov_file because the
  old name sounds like it returns something.  lcov_file was selected
  by analogy with XmlReporter.xml_file.

- Move the increment of self.total from lcov_file to the loop in
  LcovReporter.report for better separation of concerns.
Implement skip_empty mode in LcovReporter.

Also, improve consistency with what you get by running ‘coverage xml’
and then ‘xml2lcov’ by not emitting any vacuous TN: lines, not emitting
LF:0 LH:0 for empty files, and not emitting BRF:0 BRH:0 for files with
no branches.
DA-line checksums bulk up the .lcov file and provide only a weak assurance
that the source file being processed for analysis matches the source file
that was used to generate coverage data.  Current versions of the LCOV suite
discourage their use.

Add a boolean configuration option, lcov.line_checksums, which controls
whether checksums are generated for DA lines.  Consistent with the current
behavior of the LCOV suite, the default is to not generate any checksums.
…rts.

This fixes five serious bugs:

- The first field of a BRDA: line may not be zero (nedbat#1846).
- The first field of a BRDA: line is supposed to be the *source* line of each
  instrumented branch, not the destination line.
- The fourth field of a BRDA: line is supposed to be “-” when the branch
  was *never reached*, not when it was reached but never/always taken (which
  is what a branch’s presence in missing_arcs means).  As far as I can tell,
  coverage.py currently doesn’t know of the existence of branches that were
  never reached.

- The decision of whether to emit DA: and BRDA: lines at all is now taken
  strictly according to what’s in analysis.statements.  This is important
  because some lines may appear in analysis.executed and/or
  analysis.executed_branch_arcs but *not* in analysis.statements.
  For example, the beginnings of docstrings are like this, as is the
  phantom line 1 of an empty __init__.py in Python 3.10 and earlier.

  (I am pleased to note that the special casing of empty __init__.py in
  the test suite is no longer required after this patch.)

- We no longer attempt to call branch-coverage-related Analysis methods
  when analysis.has_arcs is false.

And two minor annoyances:

- DA: and BRDA: lines are now emitted strictly in ascending order by (source)
  line number.
- Source file records are now sorted by *relative* pathname, not absolute
  pathname from the coverage database.
@nedbat
Copy link
Owner

nedbat commented Sep 10, 2024

Thank you so much for doing all this! I'm still curious about the checksums. They used to always be present. You want to disable them. There's a new style which we don't support. Help me understand the lcov community feelings about these checksums. What if we simply removed them with no option?

@zackw
Copy link
Contributor Author

zackw commented Sep 10, 2024

I don't have any special insight into the lcov community. I'm just going by what I read in the lcov manpages and a bit of source archaeology. That said:

  • The lcov documentation gives me the impression that its maintainers think it's very important to be sure that the code being used to generate coverage reports matches the code that was used to measure coverage in the first place. They spend hundreds of words on this in both the genhtml and lcov manpages. This is the entire purpose of the DA: line checksums and of the newer VER: records.
  • Having said that, there has been an option to turn off generation of DA: line checksums ever since they were introduced, in version 1.2 of lcov (released July 2003), and the default behavior changed to "don't generate per-line checksums" in version 1.6 (released July 2007).
  • VER: records and the --version-script mechanism are quite new, with lcov 2.0, which came out in May of 2023. The lcov documentation does not quite say these are considered a replacement for DA: line checksums, but it's strongly implied.

Putting those three things together, I am fairly confident in saying that DA: line checksums are considered obsolete by the lcov maintainers - but not obsolete enough to drop completely. Whether coverage lcov should drop them completely is something I don't have a sense for. I would not miss them, but I don't know how many other people might.

I think generation of VER: records by coverage lcov can wait until someone comes around asking for it. For what I'm using coverage lcov for, I have a different way of ensuring generation-to-analysis consistency. And the implementation I had in #1847 is definitely no good. If coverage lcov is to generate VER: records at all, it needs to support using lcov's version scripts to generate VER: records, not just make up its own payloads.

@nedbat
Copy link
Owner

nedbat commented Sep 11, 2024

Thanks, let's merge this and see if people have concerns.

@nedbat nedbat merged commit 21d3e31 into nedbat:master Sep 11, 2024
35 checks passed
@zackw zackw deleted the lcov-report-fixes branch September 11, 2024 18:07
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.

lcov reports should not use line number zero in BRDA: records
2 participants