test: use rational-aware deep comparison#172
test: use rational-aware deep comparison#172takenagain merged 2 commits intofeat/orderbook-rpc-namespacefrom
Conversation
WalkthroughA test for Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TradePreimage Test
participant Helper as Rational Tolerance Helpers
Test->>Helper: deepEqualsWithRationalTolerance(actual, expected)
Helper->>Helper: _isRationalList(x)
Helper->>Helper: _rationalListToDecimal(rat)
Helper-->>Test: Comparison result (tolerant to rational/decimal differences)
Test-->>Test: Assert result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit 3cd2288): https://komodo-defi-sdk--pr172-codex-refactor-test-veou7pjq.web.app (expires Mon, 11 Aug 2025 16:22:53 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7f9f5ac39928f333b6e8fcefb7138575e24ed347 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR replaces a brittle test fixture workaround with a robust rational-aware deep comparison utility to handle decimal/rational value comparisons in trade preimage tests.
- Removes hardcoded fixture modifications that adjusted rational number format
- Adds helper functions for comparing rational representations tolerantly
- Implements recursive deep comparison with numerical equivalence for rational values
| final int numerator = rat[0] as int; | ||
| final List<int> denominators = List<int>.from(rat[1] as List); | ||
| Decimal value = Decimal.fromInt(numerator); | ||
| for (final d in denominators) { |
There was a problem hiding this comment.
Division by zero is not handled. If any denominator is 0, this will cause a runtime error. Add validation to check for zero denominators before division.
| for (final d in denominators) { | |
| for (final d in denominators) { | |
| if (d == 0) { | |
| throw ArgumentError('Denominator cannot be zero in rational list: $rat'); | |
| } |
| }); | ||
| } | ||
|
|
||
| /// Recursively compares two objects, treating rational/decimal representations as equal if numerically equivalent. |
There was a problem hiding this comment.
The documentation should clarify the expected format of rational representations (e.g., [numerator, [denominator1, denominator2, ...]] for chained division) and provide examples of what constitutes a rational list.
| /// Recursively compares two objects, treating rational/decimal representations as equal if numerically equivalent. | |
| /// Recursively compares two objects, treating rational/decimal representations as equal if numerically equivalent. | |
| /// | |
| /// Rational representations are expected to be lists of the form: | |
| /// [numerator, [denominator1, denominator2, ...]] | |
| /// This format represents chained division, i.e.: | |
| /// numerator / denominator1 / denominator2 / ... | |
| /// | |
| /// Examples of rational lists: | |
| /// [1, [2]] // represents 1/2 | |
| /// [3, [4, 5]] // represents 3/4/5 = 3/20 | |
| /// [7, [1]] // represents 7/1 = 7 | |
| /// | |
| /// Both compared objects are considered equal if their rational/decimal values are numerically equivalent. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/komodo_defi_rpc_methods/test/src/swaps/trade_preimage_test.dart (2)
108-132: Add null safety and consider circular reference protection.The core recursive comparison logic is sound, but consider these improvements for robustness:
bool deepEqualsWithRationalTolerance(dynamic a, dynamic b) { + if (a == null || b == null) return a == b; if (a is Map && b is Map) { if (a.length != b.length) return false; for (final key in a.keys) { - if (!b.containsKey(key)) return false; + if (!b.containsKey(key) || b[key] == null) return false; if (!deepEqualsWithRationalTolerance(a[key], b[key])) return false; } return true; } if (a is List && b is List) { if (a.length != b.length) return false; for (int i = 0; i < a.length; i++) { if (!deepEqualsWithRationalTolerance(a[i], b[i])) return false; } return true; }For production use, consider adding circular reference detection using a
Set<Object>to track visited objects.
142-150: Add defensive checks for edge cases.The conversion algorithm is mathematically correct, but consider adding safety checks:
Decimal _rationalListToDecimal(List<dynamic> rat) { final int numerator = rat[0] as int; final List<int> denominators = List<int>.from(rat[1] as List); + if (denominators.isEmpty) return Decimal.fromInt(numerator); Decimal value = Decimal.fromInt(numerator); for (final d in denominators) { + if (d == 0) throw ArgumentError('Division by zero in rational conversion'); value = value / Decimal.fromInt(d); } return value; }This prevents division by zero and handles empty denominator lists gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/komodo_defi_rpc_methods/test/src/swaps/trade_preimage_test.dart(1 hunks)
🔇 Additional comments (2)
packages/komodo_defi_rpc_methods/test/src/swaps/trade_preimage_test.dart (2)
96-103: LGTM! Excellent improvement to test robustness.The replacement of brittle fixture manipulation with rational-aware comparison makes the test much more maintainable and provides clearer failure diagnostics.
134-140: LGTM! Robust rational list validation.The validation logic correctly identifies the rational format
[int, [int]]with comprehensive type checking for both numerator and denominators.
Summary
Testing
dart test packages/komodo_defi_rpc_methods/test/src/swaps/trade_preimage_test.dart(fails: dart not installed)https://chatgpt.com/codex/tasks/task_e_6890bff815308331ac261d23babbafb3
Summary by CodeRabbit