Add some tests for BlockRng, BlockRng64 and Xoshiro RNGs#1639
Add some tests for BlockRng, BlockRng64 and Xoshiro RNGs#1639dhardy merged 4 commits intorust-random:masterfrom
Conversation
rand_core/src/block.rs
Outdated
| fn blockrng64_index() { | ||
| let rng = BlockRng64::<DummyRng64>::from_seed([1, 2, 3, 4, 5, 6, 7, 8]); | ||
| let index = rng.index(); | ||
| assert_eq!(index, 8); | ||
| } |
There was a problem hiding this comment.
It would be more semantically accurate to assert that index equals <DummyRng64 as BlockRngCore>::Results::len().
Overall though, this is an implementation detail, and the important point is that the test self.index >= self.results.as_ref().len() used in e.g. next_u64 is true.
Does this test do anything useful in the case that the internal implementation of BlockRng64 changes? I'm not convinced that it does (or is worth adding). (This also applies to fn blockrng64_reset.)
rand_core/src/block.rs
Outdated
| #[test] | ||
| fn blockrng64_fill_bytes() { | ||
| let mut rng = BlockRng64::<DummyRng64>::from_seed([1, 2, 3, 4, 5, 6, 7, 8]); | ||
| let mut dst: [u8; 13] = [0; 13]; | ||
| let mut dst2: [u8; 13] = [0; 13]; | ||
| rng.fill_bytes(&mut dst); | ||
| rng.fill_bytes(&mut dst2); | ||
| assert_ne!(dst, dst2); | ||
| } |
There was a problem hiding this comment.
This test says more about DummyRng64 than it does about BlockRng64.
It says basically nothing about the quality of rng's output, so it feels like the test is just extra lines of code for no purpose.
This applies to other fill_bytes tests too.
rand_core/src/block.rs
Outdated
| let mut rng = BlockRng64::<DummyRng64>::from_seed([1, 2, 3, 4, 5, 6, 7, 8]); | ||
| assert_eq!(rng.index(), 8); |
There was a problem hiding this comment.
I guess we can commit to this detail, but I'd prefer the RHS written as <DummyRng64 as BlockRngCore>::Results::len().
rand_core/src/block.rs
Outdated
| let mut rng = BlockRng64::<DummyRng64>::from_seed([1, 2, 3, 4, 5, 6, 7, 8]); | ||
| assert_eq!(rng.index(), 8); | ||
|
|
||
| rng.generate_and_set(100); |
There was a problem hiding this comment.
This should probably use the smallest invalid value, i.e. <DummyRng64 as BlockRngCore>::Results::len().
rand_core/src/block.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| #[should_panic] |
There was a problem hiding this comment.
Please include an expected = "..." line; otherwise this test would pass if the assert_eq! fails.
https://doc.rust-lang.org/rust-by-example/testing/unit_testing.html
| #[test] | ||
| fn blockrng_next_u64() { | ||
| let mut rng = BlockRng::<DummyRng>::from_seed([1, 2, 3, 4]); | ||
| for i in 0..7 { | ||
| rng.next_u64(); | ||
| } | ||
| rng.next_u32(); | ||
|
|
||
| let result = rng.next_u64(); | ||
| assert_eq!(rng.index(), 1); | ||
| } |
There was a problem hiding this comment.
This test is useful; in fact we document this behaviour. It is worth pointing out that the behaviour depends on the Results size again.
src/rngs/xoshiro256plusplus.rs
Outdated
| #[test] | ||
| fn from_seed() { | ||
| let seed: [u8; 32] = [0; 32]; | ||
| let mut rng = Xoshiro256PlusPlus::from_seed(seed); | ||
| let expected = [ | ||
| 5987356902031041503, | ||
| 7051070477665621255, | ||
| 6633766593972829180, | ||
| 211316841551650330, | ||
| 9136120204379184874, | ||
| 379361710973160858, | ||
| 15813423377499357806, | ||
| 15596884590815070553, | ||
| 5439680534584881407, | ||
| 1369371744833522710, | ||
| ]; | ||
| for &e in &expected { | ||
| assert_eq!(rng.next_u64(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need a second reference vector?
Actually, maybe we do due to the special behaviour when the seed is all zeroes. This should be documented here though.
Also, can we avoid copy-pasting the reference vector please? Maybe put both tests using this vector in the same function.
Xoshiro128PlusPlus should have a similar test.
…ult and normalizing the case of from_seed(0)
rand_core/src/block.rs
Outdated
| #[should_panic(expected = "index < self.results.as_ref().len()")] | ||
| fn blockrng64_generate_and_set_panic() { | ||
| let mut rng = BlockRng64::<DummyRng64>::from_seed([1, 2, 3, 4, 5, 6, 7, 8]); | ||
| assert_eq!(rng.index(), 8); |
There was a problem hiding this comment.
Replace 8 or delete the assert (either is fine)
There was a problem hiding this comment.
Thanks for your correction and thanks for your time to review my PR!😊
…Plus
CHANGELOG.mdentrySummary
Add some unit tests.
Motivation
By examining the existing code, we found that some tests can be added to improve the repo's overall test coverage.
Details
We've added some unit tests for BlockRng and BlockRng64 in block.rs, xoshiro256plusplus.rs and
rand/src/distr/bernoulli.rs
Lines 144 to 149 in 204084a
Thanks for your time to review this PR.