BurntSushi / quickcheck

Automated property based testing for Rust (with shrinking).
The Unlicense
2.32k stars 144 forks source link

Durations's Arbitrary instance is dependant on Gen's size #321

Open avandecreme opened 9 months ago

avandecreme commented 9 months ago

According to Gen::new doc:

The size parameter controls the size of random values generated. For example, it specifies the maximum length of a randomly generated vector, but is and should not be used to control the range of a randomly generated number. (Unless that number is used to control the size of a data structure.)

However Duration's Arbitrary is defined like this: https://github.com/BurntSushi/quickcheck/blob/aa968a94650b5d4d572c4ef581a7f5eb259aa0d2/src/arbitrary.rs#L1068-L1072

The way seconds is generated seems in violation of the above rule to me and has the consequence of generating very short duration only (with the default size of 100).

According to https://doc.rust-lang.org/nightly/core/time/struct.Duration.html#method.new it seems that we should be able to use the full u64 range without issue for seconds since we are already capping the nano seconds to one billion.

However from my testing this break SystemTime's instance because we can get overflows in the following implementation: https://github.com/BurntSushi/quickcheck/blob/aa968a94650b5d4d572c4ef581a7f5eb259aa0d2/src/arbitrary.rs#L1104-L1112

Any idea how we should fix this?

neithernut commented 9 months ago

We could use SystemTime::checked_add and checked_sub, and divide the duration by two if that fails. Maybe just do that in a loop until we end up with a valid SystemTime.