Skip to content

Commit

Permalink
fix: a line that branches nowhere must always raise an exception
Browse files Browse the repository at this point in the history
  • Loading branch information
@nedbat
nedbat committed Nov 23, 2024
1 parent 2ace7a2 commit d1a916a
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 30 deletions.
8 changes: 7 additions & 1 deletion CHANGES.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,17 @@ upgrading your version of coverage.py.
Unreleased
----------

- fix: the LCOV report code assumed that a branch line that took no branches
- Fix: the LCOV report code assumed that a branch line that took no branches
meant that the entire line was unexecuted. This isn't true in a few cases:
the line might always raise an exception, or might have been optimized away.
Fixes `issue 1896`_.

- Fix: similarly, the HTML report will now explain that a line that jumps to
none of its expected destinations must have always raised an exception.
Previously, it would say something nonsensical like, "line 4 didn't jump to
line 5 because line 4 was never true, and it didn't jump to line 7 because
line 4 was always true." This was also shown in `issue 1896`_.

.. _issue 1896: https://github.com/nedbat/coveragepy/issues/1896


Expand Down
39 changes: 23 additions & 16 deletions coverage/html.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ def data_for_file(self, fr: FileReporter, analysis: Analysis) -> FileData:
contexts_by_lineno = self.data.contexts_by_lineno(analysis.filename)

lines = []
branch_stats = analysis.branch_stats()

for lineno, tokens in enumerate(fr.source_token_lines(), start=1):
# Figure out how to mark this line.
Expand All @@ -150,12 +151,22 @@ def data_for_file(self, fr: FileReporter, analysis: Analysis) -> FileData:
category = "mis"
elif self.has_arcs and lineno in missing_branch_arcs:
category = "par"
for b in missing_branch_arcs[lineno]:
if b < 0:
short_annotations.append("exit")
else:
short_annotations.append(str(b))
long_annotations.append(fr.missing_arc_description(lineno, b, arcs_executed))
mba = missing_branch_arcs[lineno]
if len(mba) == branch_stats[lineno][0]:
# None of the branches were taken from this line.
short_annotations.append("anywhere")
long_annotations.append(
f"line {lineno} didn't jump anywhere: it always raised an exception."
)
else:
for b in missing_branch_arcs[lineno]:
if b < 0:
short_annotations.append("exit")
else:
short_annotations.append(str(b))
long_annotations.append(
fr.missing_arc_description(lineno, b, arcs_executed)
)
elif lineno in analysis.statements:
category = "run"

Expand Down Expand Up @@ -486,16 +497,12 @@ def write_html_page(self, ftr: FileToReport) -> None:

if ldata.long_annotations:
longs = ldata.long_annotations
if len(longs) == 1:
ldata.annotate_long = longs[0]
else:
ldata.annotate_long = "{:d} missed branches: {}".format(
len(longs),
", ".join(
f"{num:d}) {ann_long}"
for num, ann_long in enumerate(longs, start=1)
),
)
# A line can only have two branch destinations. If there were
# two missing, we would have written one as "always raised."
assert len(longs) == 1, (
f"Had long annotations in {ftr.fr.relative_filename()}: {longs}"
)
ldata.annotate_long = longs[0]
else:
ldata.annotate_long = None

Expand Down
10 changes: 5 additions & 5 deletions tests/gold/html/b_branch/b_py.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ <h2>
<a id="indexLink" class="nav" href="index.html">&Hat; index</a> &nbsp; &nbsp;
<a id="nextFileLink" class="nav" href="index.html">&#xbb; next</a>
&nbsp; &nbsp; &nbsp;
<a class="nav" href="https://coverage.readthedocs.io/en/7.6.0a0.dev1">coverage.py v7.6.0a0.dev1</a>,
created at 2024-07-10 12:20 -0400
<a class="nav" href="https://coverage.readthedocs.io/en/7.6.8a0.dev1">coverage.py v7.6.8a0.dev1</a>,
created at 2024-11-23 15:15 -0500
</p>
<aside class="hidden">
<button type="button" class="button_next_chunk" data-shortcut="j"></button>
Expand Down Expand Up @@ -101,7 +101,7 @@ <h2>
<p class="run"><span class="n"><a id="t17" href="#t17">17</a></span><span class="t"><span class="key">def</span> <span class="nam">three</span><span class="op">(</span><span class="op">)</span><span class="op">:</span>&nbsp;</span><span class="r"></span></p>
<p class="run"><span class="n"><a id="t18" href="#t18">18</a></span><span class="t"> <span class="key">try</span><span class="op">:</span>&nbsp;</span><span class="r"></span></p>
<p class="pln"><span class="n"><a id="t19" href="#t19">19</a></span><span class="t"> <span class="com"># This if has two branches, *neither* one taken.</span>&nbsp;</span><span class="r"></span></p>
<p class="par run show_par"><span class="n"><a id="t20" href="#t20">20</a></span><span class="t"> <span class="key">if</span> <span class="nam">name_error_this_variable_doesnt_exist</span><span class="op">:</span>&nbsp;</span><span class="r"><span class="annotate short">20&#x202F;&#x219B;&#x202F;21,&nbsp;&nbsp; 20&#x202F;&#x219B;&#x202F;23</span><span class="annotate long">2 missed branches: 1) line 20 didn't jump to line 21 because the condition on line 20 was never true, 2) line 20 didn't jump to line 23 because the condition on line 20 was always true</span></span></p>
<p class="par run show_par"><span class="n"><a id="t20" href="#t20">20</a></span><span class="t"> <span class="key">if</span> <span class="nam">name_error_this_variable_doesnt_exist</span><span class="op">:</span>&nbsp;</span><span class="r"><span class="annotate short">20&#x202F;&#x219B;&#x202F;anywhere</span><span class="annotate long">line 20 didn't jump anywhere: it always raised an exception.</span></span></p>
<p class="mis show_mis"><span class="n"><a id="t21" href="#t21">21</a></span><span class="t"> <span class="nam">a</span> <span class="op">=</span> <span class="num">1</span>&nbsp;</span><span class="r"></span></p>
<p class="pln"><span class="n"><a id="t22" href="#t22">22</a></span><span class="t"> <span class="key">else</span><span class="op">:</span>&nbsp;</span><span class="r"></span></p>
<p class="mis show_mis"><span class="n"><a id="t23" href="#t23">23</a></span><span class="t"> <span class="nam">a</span> <span class="op">=</span> <span class="num">2</span>&nbsp;</span><span class="r"></span></p>
Expand All @@ -117,8 +117,8 @@ <h2>
<a class="nav" href="index.html">&Hat; index</a> &nbsp; &nbsp;
<a class="nav" href="index.html">&#xbb; next</a>
&nbsp; &nbsp; &nbsp;
<a class="nav" href="https://coverage.readthedocs.io/en/7.6.0a0.dev1">coverage.py v7.6.0a0.dev1</a>,
created at 2024-07-10 12:20 -0400
<a class="nav" href="https://coverage.readthedocs.io/en/7.6.8a0.dev1">coverage.py v7.6.8a0.dev1</a>,
created at 2024-11-23 15:15 -0500
</p>
</div>
</footer>
Expand Down
11 changes: 3 additions & 8 deletions tests/test_html.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,6 @@ def three():
cov = coverage.Coverage(branch=True)
b = self.start_import_stop(cov, "b")
cov.html_report(b, directory="out/b_branch")

compare_html(gold_path("html/b_branch"), "out/b_branch")
contains(
"out/b_branch/b_py.html",
Expand All @@ -856,13 +855,9 @@ def three():
('<span class="annotate short">12&#x202F;&#x219B;&#x202F;exit</span>' +
'<span class="annotate long">line 12 didn\'t return from function \'two\' ' +
'because the condition on line 12 was always true</span>'),
('<span class="annotate short">20&#x202F;&#x219B;&#x202F;21,&nbsp;&nbsp; ' +
'20&#x202F;&#x219B;&#x202F;23</span>' +
'<span class="annotate long">2 missed branches: ' +
'1) line 20 didn\'t jump to line 21 ' +
'because the condition on line 20 was never true, ' +
'2) line 20 didn\'t jump to line 23 ' +
'because the condition on line 20 was always true</span>'),
('<span class="annotate short">20&#x202F;&#x219B;&#x202F;anywhere</span>' +
'<span class="annotate long">line 20 didn\'t jump anywhere: ' +
'it always raised an exception.</span>'),
)
contains(
"out/b_branch/index.html",
Expand Down

0 comments on commit d1a916a

Please sign in to comment.