Fix test failure on Rust v1.81#6407
Conversation
| attestation_net_bitfield, | ||
| sync_committee_net_bitfield, | ||
| score: f64::arbitrary(g), | ||
| score: rand_f64(), |
There was a problem hiding this comment.
Quickcheck (and proptest) can do a clever thing called shrinking where they simplify (shrink) failing test cases to be as small as possible. If we mix in rand we are taking control from quickcheck which will make the test non-deterministic based on quickcheck's seed and will disable shrinking.
I think it might be better to use f64::arbitrary in a loop until it returns a value other than NaN. An even cleaner option might be generating floats in a range, which would also avoid other weird values like infinity. From a quick skim of the docs I'm not sure if quickcheck's arbitrary has a function for this though
There was a problem hiding this comment.
We could do the NaN loop for now and then come back to this if/when we rewrite our quickcheck tests using proptest. Proptest is a bit better maintained
There was a problem hiding this comment.
thanks for the additional context!
There was a problem hiding this comment.
can instead we do something like
diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs
index c8425fc10..b36bccfed 100644
--- a/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs
+++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs
@@ -335,9 +335,14 @@ impl PartialOrd for Score {
impl Ord for Score {
fn cmp(&self, other: &Score) -> std::cmp::Ordering {
- self.score()
- .partial_cmp(&other.score())
- .unwrap_or(std::cmp::Ordering::Equal)
+ match (self.score(), other.score()) {
+ (s, o) if o.is_nan() || s.is_nan() => std::cmp::Ordering::Equal,
+ (_s, o) if o.is_nan() => std::cmp::Ordering::Greater,
+ (s, _o) if s.is_nan() => std::cmp::Ordering::Less,
+ (s, o) => s
+ .partial_cmp(&o)
+ .expect("all possible panic cases are covered"),
+ }
}
}and address the broken Ord impl?
|
the custom ordering implementation you suggested doesn't seem to resolve the issue. Not 100% sure why, I'll have some time this evening to dig into this a bit further |
|
@eserilev @jxs That There's also a typo in the first I'm going to merge this PR as-is because CI is now blocked on it since the runners were updated to v1.81. |
|
@mergify queue |
🛑 The pull request has been removed from the queue
|
|
@mergify refresh |
✅ Pull request refreshed |
|
@mergify queue |
✅ The pull request has been merged automaticallyDetailsThe pull request has been merged automatically at 8a085fc |
This reverts commit 8a085fc.
This reverts commit 8a085fc.
This reverts commit 8a085fc.
This reverts commit 8a085fc.
This reverts commit 8a085fc.
* generate rand f64 instead of arbitrary to prevent NaN vals * reintroduce quickcheck arbitrary but prevet NaN
Issue Addressed
#6405
Proposed Changes
prevent
NaNf64 by looping throughf64::arbitrarycalls until we get a valid number