Trust/cross compiled test targets #271
Conversation
This a test adding partial Trust tests as in dhardy/master branch
|
This increases Travis CPU-time from 15-18 min to 25-31 min (double the tests, new ones are a little slower), but is worth it I believe (I also realise Travis is running all my PRs twice, for my branches plus for PRs against this repo). I have another patch to make vs os.rs but need to figure out: (I'm sure I've seen it before. @japaric would you mind giving a hint?) |
There was a problem hiding this comment.
Great to have big-endian testing!
https://travis-ci.org/rust-lang-nursery/rand/builds/346420053?utm_source=github_status&utm_medium=notification shows two build with Linux x86_64 nightly, it seems?
We could reduce the number of builds a little by making creative combinations of rust versions, OS and architectures. For example, test nightly on MacOS, beta on Linux x86, stable on Linux x86_64, rust 1.22 with Linux MIPS64. Would that be smart, not worth it, or the start of endless problems?
src/prng/xorshift.rs
Outdated
| @@ -136,7 +136,8 @@ mod tests { | |||
| assert_eq!(rng1.next_u64(), 4325440999699518727); | |||
|
|
|||
| let mut rng2 = XorShiftRng::from_rng(&mut rng1).unwrap(); | |||
| assert_eq!(rng2.next_u64(), 15614385950550801700); | |||
| // Note: we cannot test the state of rng2 because from_rng does not | |||
| // fix Endianness. This is allowed in the trait specification. | |||
There was a problem hiding this comment.
I was a little surprised that this change is only necessary for Xorshift. With HC-128 the test is (somehow) missing? And ChaCha passes because from_rng has a default implementation with from_seed, which does the endian swapping.
Does it make sense to still do the comparison, but make it dependent on the architecture?
(also, we now have an unused variable warning)
There was a problem hiding this comment.
My view is that if the documentation says from_rng isn't required to be consistent across platforms, there's no point testing consistency. Which means the ChaCha test is also kind-of wrong, but it doesn't hurt (currently).
Yeah, that explains the warning.
There was a problem hiding this comment.
I thought of the test as a way to also catch changes in the implementation. But don't care strongly.
| cross test --all --benches --target $TARGET | ||
| else | ||
| cross test --all --tests --no-default-features --target $TARGET | ||
| cross test --all --features serde-1,log --target $TARGET |
There was a problem hiding this comment.
Is/should this list be the same as in .travis.yml?
There was a problem hiding this comment.
I don't see why that's required; e.g. I don't see much point testing docs on multiple platforms.
There was a problem hiding this comment.
To be honest I did not check the list completely. And you are right, not everything is interesting to test on all platforms. The --all flag is mostly there for workspaces, which we don't use yet, right? But I see no problem in keeping it.
There was a problem hiding this comment.
I revised the tests for non-cross builds and combined the two nightly targets we've had for ages, as well as a bit more clean-up. Yes, --all does nothing helpful now. But I'm not sure it will do quite the right thing in the future either because I don't believe passed --features apply to sub-targets.
|
Ah, found my old question about thumb* targets. Hopefully that fixes it. |
| @@ -424,10 +424,10 @@ mod imp { | |||
| } | |||
| pub fn try_fill_bytes(&mut self, v: &mut [u8]) -> Result<(), Error> { | |||
| let mib = [libc::CTL_KERN, libc::KERN_ARND]; | |||
| trace!("OsRng: reading {} bytes via kern.arandom", v.len()); | |||
Based on dhardy#44 and later modifications, but without forcing all tests to use docker.
Motivations:
Seems to work though sometimes builds don't appear to start.