From bf4c01cb68b4d8fe45bf1f5553d3fee6e5c79cd2 Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Thu, 24 Sep 2026 23:47:59 +0200 Subject: [PATCH] Fix floating point bug --- coverage_comment/files.py | 28 ++++++--- coverage_comment/main.py | 3 +- coverage_comment/template_files/comment.md.j2 | 4 +- tests/unit/test_files.py | 63 ++++++++++++++++++- tests/unit/test_template.py | 37 +++++++++-- 5 files changed, 117 insertions(+), 18 deletions(-) diff --git a/coverage_comment/files.py b/coverage_comment/files.py index 5d3dba43..b79e92aa 100644 --- a/coverage_comment/files.py +++ b/coverage_comment/files.py @@ -110,18 +110,32 @@ def compute_datafile( ) -def parse_datafile(contents: str) -> tuple[coverage.Coverage | None, decimal.Decimal]: +def parse_datafile( + contents: str, current_rate: decimal.Decimal +) -> tuple[coverage.Coverage | None, decimal.Decimal]: file_contents = json.loads_dict(contents) - coverage_rate = decimal.Decimal(str(file_contents["coverage"])) / decimal.Decimal( - 100 - ) try: - return coverage.extract_info( + previous_coverage = coverage.extract_info( data=file_contents["raw_data"], # pyright: ignore[reportArgumentType] coverage_path=pathlib.Path(file_contents["coverage_path"]), # pyright: ignore[reportArgumentType] - ), coverage_rate + ) except KeyError: - return None, coverage_rate + stored_rate = file_contents["coverage"] + assert isinstance(stored_rate, int | float) + return None, rate_from_stored_float( + stored_rate=stored_rate, current_rate=current_rate + ) + return previous_coverage, previous_coverage.info.percent_covered + + +def rate_from_stored_float( + stored_rate: float, current_rate: decimal.Decimal +) -> decimal.Decimal: + # The stored float carries less precision than a freshly computed Decimal: + # compare in float space so an unchanged rate isn't seen as a tiny delta. + if float(current_rate * 100) == stored_rate: + return current_rate + return decimal.Decimal(str(stored_rate)) / decimal.Decimal(100) class ImageURLs(TypedDict): diff --git a/coverage_comment/main.py b/coverage_comment/main.py index 5bf20fea..21a9bd69 100644 --- a/coverage_comment/main.py +++ b/coverage_comment/main.py @@ -146,7 +146,8 @@ def process_pr( previous_coverage, previous_coverage_rate = None, None if previous_coverage_data_file: previous_coverage, previous_coverage_rate = files.parse_datafile( - contents=previous_coverage_data_file + contents=previous_coverage_data_file, + current_rate=coverage.info.percent_covered, ) marker = template.get_marker(marker_id=config.SUBPROJECT_ID) diff --git a/coverage_comment/template_files/comment.md.j2 b/coverage_comment/template_files/comment.md.j2 index f637f34e..24a183a5 100644 --- a/coverage_comment/template_files/comment.md.j2 +++ b/coverage_comment/template_files/comment.md.j2 @@ -6,7 +6,7 @@ {%- if previous_coverage_rate %} {%- set text = "Coverage for the whole project went from " ~ (previous_coverage_rate | pct) ~ " to " ~ (coverage.info.percent_covered | pct) -%} {%- set color = (coverage.info.percent_covered - previous_coverage_rate) | get_evolution_color(neutral_color='blue') -%} - + {%- else -%} {%- set text = "Coverage for the whole project is " ~ (coverage.info.percent_covered | pct) ~ ". Previous coverage rate is not available, cannot report on evolution." -%} @@ -93,7 +93,7 @@ {%- set text = "This PR doesn't change the coverage rate in " ~ path ~ ", which is " ~ percent_covered | pct ~ " (" ~ covered_statements_count ~ "/" ~ statements_count ~ ")." -%} {%- endif -%} {%- set color = coverage_diff | get_evolution_color() -%} -{%- set message = "(" ~ previous_covered_statements_count | compact ~ "/" ~ previous_statements_count | compact ~ " > " ~ covered_statements_count | compact ~ "/" ~ statements_count | compact ~ ")" -%} +{%- set message = "(" ~ previous_covered_statements_count | compact ~ "/" ~ previous_statements_count | compact ~ " → " ~ covered_statements_count | compact ~ "/" ~ statements_count | compact ~ ")" -%} {%- else -%} {%- set text = "The coverage rate of " ~ path ~ " is " ~ percent_covered | pct ~ " (" ~ covered_statements_count ~ "/" ~ statements_count ~ "). The file did not seem to exist on the base branch." -%} {%- set message = "(" ~ covered_statements_count | compact ~ "/" ~ statements_count | compact ~ ")" -%} diff --git a/tests/unit/test_files.py b/tests/unit/test_files.py index 77fcb650..5bfd376d 100644 --- a/tests/unit/test_files.py +++ b/tests/unit/test_files.py @@ -4,6 +4,8 @@ import json import pathlib +import pytest + from coverage_comment import files @@ -68,7 +70,9 @@ def test_compute_datafile(): def test_parse_datafile(): - assert files.parse_datafile(contents="""{"coverage": 12.34}""") == ( + assert files.parse_datafile( + contents="""{"coverage": 12.34}""", current_rate=decimal.Decimal("0.5") + ) == ( None, decimal.Decimal("0.1234"), ) @@ -82,10 +86,65 @@ def test_parse_datafile__previous(coverage_json, coverage_obj): "raw_data": coverage_json, "coverage_path": ".", } + ), + current_rate=decimal.Decimal("0.5"), + ) + + assert result == (coverage_obj, coverage_obj.info.percent_covered) + + +def test_parse_datafile__previous_rate_is_exact(coverage_json, coverage_obj): + current_rate = coverage_obj.info.percent_covered + _, previous_rate = files.parse_datafile( + contents=files.compute_datafile( + raw_coverage_data=coverage_json, + line_rate=current_rate * 100, + coverage_path=pathlib.Path("."), + ), + current_rate=current_rate, + ) + + assert previous_rate == current_rate + + +@pytest.mark.parametrize( + "current_rate", + [ + decimal.Decimal(1) / decimal.Decimal(3), + decimal.Decimal(2) / decimal.Decimal(3), + decimal.Decimal(3931) / decimal.Decimal(4166), + decimal.Decimal(3931) / decimal.Decimal(4167), + ], +) +def test_rate_from_stored_float__unchanged(current_rate): + assert ( + files.rate_from_stored_float( + stored_rate=float(current_rate * 100), current_rate=current_rate ) + == current_rate + ) + + +@pytest.mark.parametrize( + "previous_rate, current_rate", + [ + ( + decimal.Decimal(999_999) / decimal.Decimal(1_000_000), + decimal.Decimal(999_998) / decimal.Decimal(999_999), + ), + ( + decimal.Decimal(999_998) / decimal.Decimal(999_999), + decimal.Decimal(999_999) / decimal.Decimal(1_000_000), + ), + ], +) +def test_rate_from_stored_float__tiny_change(previous_rate, current_rate): + result = files.rate_from_stored_float( + stored_rate=float(previous_rate * 100), current_rate=current_rate ) - assert result == (coverage_obj, decimal.Decimal("0.1234")) + assert (current_rate - result > 0) is (current_rate > previous_rate) + assert result != current_rate def test_get_urls(): diff --git a/tests/unit/test_template.py b/tests/unit/test_template.py index ca1b50e2..6820c943 100644 --- a/tests/unit/test_template.py +++ b/tests/unit/test_template.py @@ -51,6 +51,31 @@ def test_get_comment_markdown(coverage_obj, diff_coverage_obj): assert result == expected +def test_template__coverage_unchanged(coverage_obj, diff_coverage_obj): + result = template.get_comment_markdown( + coverage=coverage_obj, + diff_coverage=diff_coverage_obj, + previous_coverage=None, + previous_coverage_rate=coverage_obj.info.percent_covered, + minimum_green=decimal.Decimal(79), + minimum_orange=decimal.Decimal(40), + files=[], + count_files=0, + max_files=25, + github_host="https://github.com", + repo_name="org/repo", + pr_number=5, + branch_name=None, + base_template=template.read_template_file("comment.md.j2"), + marker="", + ) + + assert ( + "https://img.shields.io/badge/Coverage%20evolution-62%25%20%E2%86%92%2062%25-blue.svg" + in result + ) + + def test_template(coverage_obj, diff_coverage_obj): files, total = template.select_files( coverage=coverage_obj, @@ -83,7 +108,7 @@ def test_template(coverage_obj, diff_coverage_obj): expected = """## Coverage report (foo) -
Click to see where and how coverage changed +
Click to see where and how coverage changed
@@ -208,25 +233,25 @@ def test_template_full(make_coverage, make_coverage_and_diff): expected = """## Coverage report -
Click to see where and how coverage changed
FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
+
Click to see where and how coverage changed
- + - + - + - +
FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  codebase
  code.py12-14, 22
12-14, 22
  other.py
  third.py
Project Total