fix test_spec_page order-29 tie-break probe (issue #123)#125
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Assessment: LGTM (COMMENT only). Verified locally: on main this test dies with ValueError: invalid literal for int() with base 10: 'p' at the int(dec[-1]) line; on this branch TestDecimalParseTieBreak is 2 passed (full test_spec_page.py green). The fix targets the exact root cause and is faithful to issue #123's intent.
Correctness checks I ran through:
- Pins the intent, does not silence it.
dec = marked.rstrip("p")correctly rebuilds the unmarked order-29 probe after asserting the point repr carriesp(spec §2 / issue #120). The §4 tie-break assertion (unmarked → AREA word, suffix28 + t28*5 + t29 + 1, same-path prefix,parsed != word) is preserved on a well-formed string, not weakened. - Strengthens, not weakens. The added
_decimal_to_word(marked) == wordgenuinely pins that thep-marked string round-trips to the POINT word — new coverage the old test lacked. The non-injectivity assertion now correctly compares the area word's repr against the unmarkeddec(area renders withoutp), which is the right pin now that the marker disambiguates the two renders. - No off-by-one.
t28 = dec[-2]-1,t29 = dec[-1]-1operate on the last two body digits of the unmarked string exactly as before #120 broke it; the point slot48 + t28*4 + t29(→ 48..63) and area slot29 + t28*5 + t29(→ 29..47) ranges are consistent. No assertion is vacuous or tautological — the arithmetic formulas independently pin the Rust output. - Genuinely test-only. Only
mortie/tests/test_spec_page.pychanged (+14/-5); no kernel /decimal_repr/ runtime change, noskip, no# noqa, no lint-ignore. Style matches the surrounding class. - Governance. Commit message is title-only (§3). PR body carries the substance —
Closes #123, what/why, approach, how-tested, Questions-for-review — with no stray🤖 *from Claude*attribution in the description (§6). Draft,implementlabel.
One optional style nit left inline (rstrip vs [:-1]); non-blocking. Nothing here should hold the PR.
Generated by Claude Code
| # order-29 string, so strip the marker to build the ambiguous probe. | ||
| marked = arr.decimal_repr()[0] | ||
| assert marked.endswith("p") | ||
| dec = marked.rstrip("p") |
There was a problem hiding this comment.
🤖 from Claude (review)
Non-blocking nit (optional): marked.rstrip("p") strips all trailing ps, whereas the intent is to drop the single kind marker. It's harmless here — the body digits are 1..4 and the preceding assert marked.endswith("p") guarantees exactly one p to remove — so rstrip and [:-1] are equivalent on any well-formed input. marked[:-1] would state "drop the one marker" more precisely, but this is purely stylistic; no change required.
Generated by Claude Code
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 94.33% 94.38% +0.04%
==========================================
Files 9 9
Lines 1307 1317 +10
==========================================
+ Hits 1233 1243 +10
Misses 74 74
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Merging this PR will degrade performance by 10.88%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | coverage_triangle[4] |
130.6 µs | 146.5 µs | -10.88% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/123-spec-page-tiebreak (7a0e860) with main (4b57a1f)
Footnotes
-
1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩
Closes #123
What / why
mainwas red:TestDecimalParseTieBreak::test_order29_string_parses_to_area_wordfailed withValueError: invalid literal for int() with base 10: 'p'.The test derives
t28, t29fromdec[-2]/dec[-1], but issue #120's "delegate point suffix to kernel" madedecimal_repr()terminate a point word with thepkind suffix (spec §2 — the render form carriespon point ids). Sodec[-1] == 'p'andint('p')raises.The runtime is correct per spec — this is test-only. §2 says the render carries
p; §4 says an unmarked order-29 decimal string ties to the AREA word. The pin, not the behavior, was stale, so no kernel /decimal_reprchange is warranted.Approach
Strip the marker to build the unmarked probe the tie-break is actually about (
dec = marked.rstrip("p")), then keep the existing tie-break assertions on that well-formed string. Also, while there:marked.endswith("p")(documents decimal repr kind suffix: emit p for point ids, accept on parse (spec §2/§4) #120's render form),parsed & 0x3F == 28 + t28*5 + t29 + 1, same-path prefix,parsed != word, non-injectivity of the unmarked string),p-marked string is unambiguous and round-trips back to the POINT word (_decimal_to_word(marked) == word) — the marker is what disambiguates.The non-injectivity assertion now compares the area word's repr against the unmarked
dec(the area word renders withoutp), which is the correct pin.How tested
maturin develop --release(rebuilt the Rust ext; no Rust change)flake8 mortie --select=E9,F63,F7,F82— cleanpytest -v1 failed, 3 passedintest_spec_page.py(full suite red)4 passedintest_spec_page.py; full suite684 passed, 11 skippedQuestions for review
None — the fix is faithful to the test's stated purpose (unmarked → area, marked → point) and leaves runtime behavior untouched. Flagging only that
main-red is resolved by this single test-only change.🤖 Generated with Claude Code
Generated by Claude Code