Skip to content

Fix flaky round_value property test#510

Merged
gcomte merged 1 commit intomasterfrom
fix/property-test-floating-point-v2
Mar 30, 2026
Merged

Fix flaky round_value property test#510
gcomte merged 1 commit intomasterfrom
fix/property-test-floating-point-v2

Conversation

@gcomte
Copy link
Copy Markdown
Owner

@gcomte gcomte commented Mar 30, 2026

Summary

  • round_value_has_correct_decimal_places failed for large BTC values (e.g. 40392622.397074476) because the recomputation diverged by 1 ULP due to f64 precision limits
  • Replaced with a bounded-error test: rounding must not move the value by more than half a unit of the last decimal place (+ f64 tolerance scaled to magnitude)
  • Supersedes Fix flaky round_value property test #508

Summary by CodeRabbit

  • Tests
    • Improved rounding behavior validation tests to better measure and verify actual error margins during currency value operations. Tests now assert that rounding errors remain within mathematically defined acceptable tolerance thresholds, providing more robust verification of precision and accuracy in floating-point currency calculations across various edge cases.

The original test compared round_value output against a recomputation
that could differ by 1 ULP for large values due to f64 precision.
Replace with a bounded-error test: rounding must not move the value
by more than half a unit of the last decimal place.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 33b20109-b270-4d15-bc02-44d848027c44

📥 Commits

Reviewing files that changed from the base of the PR and between 4eebf6a and 9dd45ba.

📒 Files selected for processing (1)
  • src/currency/btc.rs

📝 Walkthrough

Walkthrough

A test function in the Bitcoin currency module was renamed and its assertion logic was updated to measure rounding error using a half-unit tolerance approach instead of verifying decimal-place behavior against a fixed tolerance.

Changes

Cohort / File(s) Summary
Test Assertion Logic Update
src/currency/btc.rs
Renamed round_value_has_correct_decimal_places to round_value_error_within_half_unit and changed the assertion from a fixed tolerance-based check to measuring rounding error as |rounded - amount| against half_unit + amount.abs() * f64::EPSILON. Updated failure message accordingly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • Round output values #355: Introduces decimal_places and rounding behavior additions in src/currency/btc.rs that directly relate to the test's new half-unit tolerance assertions.

Poem

🐰 A tighter tolerance takes flight,
Rounding errors measured just right,
Half-units and epsilon dance,
Testing precision at a glance,
Accuracy shines oh-so-bright!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly describes the main change: fixing a flaky property test for round_value functionality.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/property-test-floating-point-v2

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gcomte gcomte merged commit bfa9966 into master Mar 30, 2026
7 checks passed
@gcomte gcomte deleted the fix/property-test-floating-point-v2 branch March 30, 2026 09:01
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.

1 participant