Conversation
The test compared round_value output against a recomputation that could differ by 1 ULP for large values due to f64 precision limits. Replace with an idempotency check: round(round(x)) == round(x).
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/currency/btc.rs (1)
92-101: Test no longer verifies the decimal-place quantization contract.The
round_valuemethod's contract (perCurrencytrait inmod.rs) is to quantize values todecimal_places()precision. This idempotency test only confirmsround(round(x)) == round(x), but doesn't verify that the output actually has the correct number of decimal places.Consider adding a complementary test (not property-based) with known values to verify quantization, e.g.:
#[test] fn round_value_quantizes_correctly() { let btc = BitcoinUnit::BTC; assert_eq!(btc.round_value(1.123456789), 1.12345679); // 8 decimal places }This would maintain coverage of the original intent without the flakiness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/currency/btc.rs` around lines 92 - 101, Add a non-property unit test that verifies round_value actually quantizes to the Currency::decimal_places() precision (in addition to the existing round_value_is_idempotent property test): create a test (e.g., round_value_quantizes_correctly) that uses a concrete unit like BitcoinUnit::BTC, calls round_value on a known value with excess decimals, and asserts equality against the expected value rounded to btc.decimal_places(); reference the round_value method and decimal_places() on the Currency/BitcoinUnit types so the test fails if quantization is incorrect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/currency/btc.rs`:
- Around line 91-102: The test round_value_is_idempotent uses an absolute
tolerance f64::EPSILON which is too strict for large magnitudes; update the
assertion in the round_value_is_idempotent test to use a relative or
magnitude-scaled tolerance instead of f64::EPSILON: compute an allowed_delta as
something like (once.abs().max(1.0) * unit.precision_factor_or_decimal_places *
f64::EPSILON) or derive it from the unit's rounding factor, then assert (once -
twice).abs() <= allowed_delta; reference the test function
round_value_is_idempotent and the method unit.round_value to locate where to
change the assertion.
---
Nitpick comments:
In `@src/currency/btc.rs`:
- Around line 92-101: Add a non-property unit test that verifies round_value
actually quantizes to the Currency::decimal_places() precision (in addition to
the existing round_value_is_idempotent property test): create a test (e.g.,
round_value_quantizes_correctly) that uses a concrete unit like
BitcoinUnit::BTC, calls round_value on a known value with excess decimals, and
asserts equality against the expected value rounded to btc.decimal_places();
reference the round_value method and decimal_places() on the
Currency/BitcoinUnit types so the test fails if quantization is incorrect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| #[test] | ||
| fn round_value_has_correct_decimal_places( | ||
| fn round_value_is_idempotent( | ||
| amount in 0.0_f64..1.0e8, | ||
| unit in arb_btc_unit(), | ||
| ) { | ||
| let rounded = unit.round_value(amount); | ||
| let factor = 10_f64.powi(unit.decimal_places().into()); | ||
| let check = (rounded * factor).round() / factor; | ||
| let diff = (rounded - check).abs(); | ||
| let once = unit.round_value(amount); | ||
| let twice = unit.round_value(once); | ||
| prop_assert!( | ||
| diff < 1.0e-10, | ||
| "round_value produced too many decimal places: {rounded} (expected {check})" | ||
| (once - twice).abs() < f64::EPSILON, | ||
| "round_value is not idempotent: round({amount}) = {once}, round({once}) = {twice}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The f64::EPSILON tolerance is too strict and causes test failures.
The pipeline shows this test is still flaky: round(37515032.691102825) = 37515032.69110283, round(37515032.69110283) = 37515032.69110284.
f64::EPSILON (~2.2e-16) is the precision near 1.0, but for values around 3.7e7, the actual precision is roughly 3.7e7 * 2.2e-16 ≈ 8e-9. When round_value divides by the factor, the result may not be exactly representable, and re-rounding can shift by 1 ULP—which is far larger than f64::EPSILON.
Use a tolerance proportional to the value's magnitude and the decimal precision being tested:
Proposed fix using relative tolerance
#[test]
fn round_value_is_idempotent(
amount in 0.0_f64..1.0e8,
unit in arb_btc_unit(),
) {
let once = unit.round_value(amount);
let twice = unit.round_value(once);
+ // Tolerance must account for f64 representation limits at this magnitude.
+ // For 8 decimal places, 1e-7 provides margin for 1-ULP drift.
+ let tol = 1.0e-7_f64.max(once.abs() * 1.0e-14);
prop_assert!(
- (once - twice).abs() < f64::EPSILON,
+ (once - twice).abs() < tol,
"round_value is not idempotent: round({amount}) = {once}, round({once}) = {twice}"
);
}Alternatively, if strict idempotence is required, consider using fixed-point or decimal types for currency calculations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn round_value_has_correct_decimal_places( | |
| fn round_value_is_idempotent( | |
| amount in 0.0_f64..1.0e8, | |
| unit in arb_btc_unit(), | |
| ) { | |
| let rounded = unit.round_value(amount); | |
| let factor = 10_f64.powi(unit.decimal_places().into()); | |
| let check = (rounded * factor).round() / factor; | |
| let diff = (rounded - check).abs(); | |
| let once = unit.round_value(amount); | |
| let twice = unit.round_value(once); | |
| prop_assert!( | |
| diff < 1.0e-10, | |
| "round_value produced too many decimal places: {rounded} (expected {check})" | |
| (once - twice).abs() < f64::EPSILON, | |
| "round_value is not idempotent: round({amount}) = {once}, round({once}) = {twice}" | |
| ); | |
| } | |
| #[test] | |
| fn round_value_is_idempotent( | |
| amount in 0.0_f64..1.0e8, | |
| unit in arb_btc_unit(), | |
| ) { | |
| let once = unit.round_value(amount); | |
| let twice = unit.round_value(once); | |
| // Tolerance must account for f64 representation limits at this magnitude. | |
| // For 8 decimal places, 1e-7 provides margin for 1-ULP drift. | |
| let tol = 1.0e-7_f64.max(once.abs() * 1.0e-14); | |
| prop_assert!( | |
| (once - twice).abs() < tol, | |
| "round_value is not idempotent: round({amount}) = {once}, round({once}) = {twice}" | |
| ); | |
| } |
🧰 Tools
🪛 GitHub Actions: Build and Test
[error] 98-98: Assertion context for failing proptest 'currency::btc::tests::round_value_is_idempotent' (minimal failing input: amount=37515032.691102825, unit=BTC).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/currency/btc.rs` around lines 91 - 102, The test
round_value_is_idempotent uses an absolute tolerance f64::EPSILON which is too
strict for large magnitudes; update the assertion in the
round_value_is_idempotent test to use a relative or magnitude-scaled tolerance
instead of f64::EPSILON: compute an allowed_delta as something like
(once.abs().max(1.0) * unit.precision_factor_or_decimal_places * f64::EPSILON)
or derive it from the unit's rounding factor, then assert (once - twice).abs()
<= allowed_delta; reference the test function round_value_is_idempotent and the
method unit.round_value to locate where to change the assertion.
|
Superseded by #510 |
1 participant
Summary
round_value_has_correct_decimal_placestest comparedround_valueoutput against a recomputation that could differ by 1 ULP for large values (e.g.40392622.397074476with 8 decimal places)round(round(x)) == round(x)) which tests the actual property we care about without being sensitive to f64 precision limitsSummary by CodeRabbit