From 4d08bcebdf85c405610c90ebe0dba064279647c8 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 29 Oct 2026 05:47:03 -0400 Subject: [PATCH] Read a period after a quoted reproducer id as the sentence's end Fix evidence failed #1969 on 29 Oct because its Reproducer line ended "`...::test_x`.": _unquote_id strips backticks only when the value ends with one, so the last id kept both. A period right after a closing backtick now ends the sentence. An unquoted id keeps a trailing period, since a YAML test's name can end with one. #2735 failed because its YAML names contain commas and the line left them unquoted. The hint now says to put such an id in backticks. Fixes #2743 Co-Authored-By: Claude Opus 5.5 --- .github/check_fix_pr.py | 9 ++++- changelog.d/2743.fixed.md | 1 + .../tests/code_health/test_check_fix_pr.py | 33 ++++++++++++++++++- 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 changelog.d/2743.fixed.md diff --git a/.github/check_fix_pr.py b/.github/check_fix_pr.py index 9c9452a38c..9e0d080d37 100644 --- a/.github/check_fix_pr.py +++ b/.github/check_fix_pr.py @@ -29,7 +29,9 @@ REPRODUCER_HINT = ( 'Use "Reproducer: policyengine_uk/tests/test_example.py::test_regression", ' '"Reproducer: policyengine_uk/tests/policy/example.yaml::Case name", ' - 'or "Reproducer: n/a: " when model source and dependencies are unchanged.' + 'or "Reproducer: n/a: " when model source and dependencies are unchanged. ' + "Separate several ids with commas or semicolons, and put an id in backticks " + "when its name contains either." ) @@ -112,6 +114,11 @@ def _exemption(reproducer: str) -> re.Match | None: def _unquote_id(value: str) -> str: value = value.strip() + # A line can end its sentence after a quoted id: "`a.py::test_b`." The + # period is not part of the id. An unquoted id keeps it, since a YAML + # test's name can end with one. + if value.startswith("`") and value.endswith("`."): + value = value[:-1] return value[1:-1] if value.startswith("`") and value.endswith("`") else value diff --git a/changelog.d/2743.fixed.md b/changelog.d/2743.fixed.md new file mode 100644 index 0000000000..3e7590d3b6 --- /dev/null +++ b/changelog.d/2743.fixed.md @@ -0,0 +1 @@ +Fix evidence reads a period after a quoted reproducer id as the end of the sentence, and its hint says to put an id in backticks when its name contains a comma or semicolon. diff --git a/policyengine_uk/tests/code_health/test_check_fix_pr.py b/policyengine_uk/tests/code_health/test_check_fix_pr.py index 986767f1d1..bb94aded6e 100644 --- a/policyengine_uk/tests/code_health/test_check_fix_pr.py +++ b/policyengine_uk/tests/code_health/test_check_fix_pr.py @@ -7,7 +7,8 @@ or a bug/bug-fix label, classifies a PR as a fix. 2. Deleted tests never count as changed executable tests or reproducers. 3. Formatting reproducer ids with supported separators and quoting round - trips through the body parser. + trips through the body parser, including a sentence's period after a + closing backtick. 4. A pytest reproducer requires a changed file and a directly defined test function or class method that pytest collects, checked against real pytest collection, ignoring parametrisation suffixes. A collected test marked to @@ -205,12 +206,15 @@ def test_deleted_tests_never_count_as_executable_or_reproducers(tmp_path, paths) @given(ids=st.lists(_valid_id, max_size=8), data=st.data()) def test_reproducer_line_round_trips_supported_formatting(ids, data): pieces = [] + quoted = False for index, nodeid in enumerate(ids): if index: pieces.append(data.draw(st.sampled_from([",", ";", ", ", " ; ", "\t;\t"]))) spaces = data.draw(st.sampled_from(["", " ", "\t", " "])) quoted = data.draw(st.booleans()) pieces.append(spaces + (f"`{nodeid}`" if quoted else nodeid) + spaces) + if quoted and data.draw(st.booleans()): + pieces[-1] = pieces[-1].rstrip() + "." key = data.draw( st.sampled_from(["Reproducer", "reproducer", "REPRODUCER", "RePrOdUcEr"]) ) @@ -475,6 +479,33 @@ def test_backticked_na_reason_can_include_commas_and_semicolons(tmp_path): assert _report(tmp_path, "Reproducer: `n/a: CI, docs; no model source`", []).passed +def test_a_period_after_a_closing_backtick_ends_the_sentence(tmp_path): + python_path = "policyengine_uk/tests/test_regression.py" + yaml_path = "policyengine_uk/tests/policy/regression.yaml" + _write(tmp_path, python_path, "def test_regression():\n pass\n") + _write( + tmp_path, + yaml_path, + '- name: "A case, with a comma"\n output: {x: 1}\n' + "- name: Ends with a period.\n output: {x: 1}\n", + ) + changes = [("A", python_path), ("A", yaml_path)] + for body in ( + f"Reproducer: `{python_path}::test_regression`, " + f"`{yaml_path}::A case, with a comma`.", + f"Reproducer: `{yaml_path}::Ends with a period.`.", + # Unquoted, the period belongs to the id. + f"Reproducer: {yaml_path}::Ends with a period.", + ): + assert _report(tmp_path, body, changes).passed, body + # Unquoted, a comma splits the name, and the message says to quote it. + report = _report( + tmp_path, f"Reproducer: {yaml_path}::A case, with a comma", changes + ) + assert not report.passed + assert "in backticks when its name contains" in " ".join(report.messages) + + @pytest.mark.parametrize( "source, suffix", [