tests: Fix randomly failing win_prob smoke test #7230
Conversation
📝 WalkthroughWalkthroughA local variable Changes
Sequence Diagram(s)sequenceDiagram
participant Test as test_hoprd_should_relay_packets_with_lower_win_prob_then_redeem_them
participant Func as check_unredeemed_tickets_value
Test->>Func: Call with quantized decimal values
Func-->>Test: Await until unredeemed tickets value ≥ target (quantized)
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Ruff (0.11.9)tests/test_win_prob.py116-116: Local variable Remove assignment to unused variable (F841) ⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR stabilizes the win_prob smoke test by rounding the expected unredeemed ticket value to avoid random failures due to floating‐point precision.
- Introduces a
precisionvariable for rounding - Wraps the expected value calculation in
round(..., precision)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_win_prob.py (2)
116-116: Prefer a named, module-level constant over a magic number
precision = 10is a hard-coded literal inside the test body. Declaring a small constant such asDECIMAL_PRECISION = 10near the top of the module (or re-using an existing constant) makes the intent explicit and avoids scattering the same literal in future edits.
120-123: Avoidfloat → Decimalconversion; constructDecimaldirectly from a string
Decimal(win_prob)converts the binary float0.1toDecimal('0.100000000000000005551...'), defeating the purpose of the new rounding safeguard and still leaving room for non-deterministic comparisons.Convert from a string (or declare
win_probitself as aDecimal) and, while at it, usequantizefor explicit rounding:- statistics_before.unredeemed_value + (ticket_price + ticket_price / Decimal(win_prob)), - precision, + ( + statistics_before.unredeemed_value + + ticket_price + + ticket_price / Decimal(str(win_prob)) + ).quantize(Decimal(f"1e-{precision}")),This keeps the arithmetic purely in
Decimalspace and guarantees deterministic, half-even rounding to exactlyprecisiondecimal places.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_win_prob.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_win_prob.py (2)
tests/utils.py (1)
check_unredeemed_tickets_value(114-119)tests/conftest.py (1)
swarm7(95-107)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Docs / Rust docs
- GitHub Check: Linter
- GitHub Check: Build Candidate Binaries (hoprd)
- GitHub Check: tests-unit
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: tests-unit-nightly
- GitHub Check: zizmor
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
df9f5da to
a180118
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_win_prob.py (1)
116-118:precisionis unused — remove or pass it tocheck_unredeemed_tickets_valueThe local variable
precisionis assigned but never referenced, tripping Ruff F841 and adding noise.
Either delete the line or use the variable by passing an explicitquantizeexponent to the utility:- precision = 10 - await asyncio.wait_for( - check_unredeemed_tickets_value( - swarm7[relay], - statistics_before.unredeemed_value + (ticket_price + ticket_price / Decimal(win_prob)), - ), - 30.0, - ) + await asyncio.wait_for( + check_unredeemed_tickets_value( + swarm7[relay], + statistics_before.unredeemed_value + (ticket_price + ticket_price / Decimal(win_prob)), + quantize=Decimal("1e-10"), # or Decimal(f"1e-{precision}") if you keep the variable + ), + 30.0, + )🧰 Tools
🪛 Ruff (0.11.9)
116-116: Local variable
precisionis assigned to but never usedRemove assignment to unused variable
precision(F841)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_win_prob.py(1 hunks)tests/utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/utils.py (2)
sdk/python/localcluster/node.py (2)
Node(36-319)api(84-85)sdk/python/api/hopr.py (1)
get_tickets_statistics(351-357)
🪛 Ruff (0.11.9)
tests/utils.py
114-114: f-string without any placeholders
Remove extraneous f prefix
(F541)
tests/test_win_prob.py
116-116: Local variable precision is assigned to but never used
Remove assignment to unused variable precision
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: hopli / docker
- GitHub Check: zizmor
- GitHub Check: Build Candidate Binaries (hoprd)
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: Linter
- GitHub Check: Cargo Audit
7e190b3 to
d2634b8
Compare
candidatebuild to 20min timeout