Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup Testing code #471

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ jobs:
toolchain: ${{ matrix.toolchain }}
- uses: Swatinem/rust-cache@v2
- run: cargo test
# Make sure enabling the std and custom features don't break anything
- run: cargo test --features=std,custom
# Ensure enabling features works, and run feature-specific tests.
- run: cargo test --features=std,custom,rdrand
- run: cargo test --features=linux_disable_fallback
- if: ${{ matrix.toolchain == 'nightly' }}
run: cargo test --benches
Expand Down Expand Up @@ -258,8 +258,8 @@ jobs:
- name: Test (Safari)
if: runner.os == 'macOS'
run: wasm-pack test --headless --safari --features=js,test-in-browser
- name: Test (custom getrandom)
run: wasm-pack test --node --features=custom
- name: Test (custom getrandom, no unit tests)
run: wasm-pack test --node --features=custom --test=custom
- name: Test (JS overrides custom)
run: wasm-pack test --node --features=custom,js

Expand Down
14 changes: 12 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,13 @@ mod util;
mod custom;
#[cfg(feature = "std")]
mod error_impls;
// If the rdrand feature is enabled, always bring in the rdrand module, so
// that the RDRAND implementation can be tested.
#[cfg(all(
any(target_env = "sgx", feature = "rdrand"),
any(target_arch = "x86_64", target_arch = "x86"),
))]
mod rdrand;

pub use crate::error::Error;

Expand Down Expand Up @@ -330,10 +337,10 @@ cfg_if! {
} else if #[cfg(windows)] {
#[path = "windows.rs"] mod imp;
} else if #[cfg(all(target_arch = "x86_64", target_env = "sgx"))] {
#[path = "rdrand.rs"] mod imp;
use rdrand as imp;
} else if #[cfg(all(feature = "rdrand",
any(target_arch = "x86_64", target_arch = "x86")))] {
#[path = "rdrand.rs"] mod imp;
use rdrand as imp;
} else if #[cfg(all(feature = "js",
any(target_arch = "wasm32", target_arch = "wasm64"),
target_os = "unknown"))] {
Expand Down Expand Up @@ -404,3 +411,6 @@ pub fn getrandom_uninit(dest: &mut [MaybeUninit<u8>]) -> Result<&mut [u8], Error
// since it returned `Ok`.
Ok(unsafe { slice_assume_init_mut(dest) })
}

#[cfg(test)]
mod tests;
6 changes: 6 additions & 0 deletions src/rdrand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ fn is_rdrand_good() -> bool {
unsafe { self_test() }
}

#[allow(dead_code)]
pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
static RDRAND_GOOD: LazyBool = LazyBool::new();
if !RDRAND_GOOD.unsync_init(is_rdrand_good) {
Expand Down Expand Up @@ -121,3 +122,8 @@ unsafe fn rdrand_exact(dest: &mut [MaybeUninit<u8>]) -> Option<()> {
}
Some(())
}

#[cfg(test)]
mod tests {
crate::tests::define_tests!(super::getrandom_inner);
}
139 changes: 139 additions & 0 deletions src/tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
use crate::Error;

extern crate std;
use std::{mem::MaybeUninit, sync::mpsc::channel, thread, vec, vec::Vec};

#[cfg(feature = "test-in-browser")]
wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser);

// Return the number of bits in which s1 and s2 differ
fn num_diff_bits(s1: &[u8], s2: &[u8]) -> usize {
assert_eq!(s1.len(), s2.len());
s1.iter()
.zip(s2.iter())
.map(|(a, b)| (a ^ b).count_ones() as usize)
.sum()
}

// Tests the quality of calling getrandom on two large buffers
pub(crate) fn check_diff_large(make_vec: fn(usize) -> Vec<u8>) {
let v1 = make_vec(1000);
let v2 = make_vec(1000);

// Between 3.5 and 4.5 bits per byte should differ. Probability of failure:
// ~ 2^(-94) = 2 * CDF[BinomialDistribution[8000, 0.5], 3500]
let d = num_diff_bits(&v1, &v2);
assert!(d > 3500);
assert!(d < 4500);
}

// Tests the quality of calling getrandom repeatedly on small buffers
pub(crate) fn check_small(make_vec: fn(usize) -> Vec<u8>) {
// For each buffer size, get at least 256 bytes and check that between
// 3 and 5 bits per byte differ. Probability of failure:
// ~ 2^(-91) = 64 * 2 * CDF[BinomialDistribution[8*256, 0.5], 3*256]
for size in 1..=64 {
let mut num_bytes = 0;
let mut diff_bits = 0;
while num_bytes < 256 {
let s1 = make_vec(size);
let s2 = make_vec(size);
num_bytes += size;
diff_bits += num_diff_bits(&s1, &s2);
}
assert!(diff_bits > 3 * num_bytes);
assert!(diff_bits < 5 * num_bytes);
}
}

pub(crate) fn check_multithreading(make_vec: fn(usize) -> Vec<u8>) {
let mut txs = vec![];
for _ in 0..20 {
let (tx, rx) = channel();
txs.push(tx);

thread::spawn(move || {
// wait until all the tasks are ready to go.
rx.recv().unwrap();
for _ in 0..100 {
make_vec(1000);
thread::yield_now();
}
});
}

// start all the tasks
for tx in txs.iter() {
tx.send(()).unwrap();
}
}

// Helper trait for testing different kinds of functions.
// DUMMY generic parameter is needed to avoid conflicting implementations.
pub(crate) trait FillFn<const DUMMY: usize> {
fn make_vec(self, len: usize) -> Vec<u8>;
}
impl<F: Fn(&mut [u8]) -> Result<(), Error>> FillFn<0> for F {
fn make_vec(self, len: usize) -> Vec<u8> {
let mut v = vec![0; len];
self(&mut v).unwrap();
v
}
}
impl<F: Fn(&mut [MaybeUninit<u8>]) -> Result<(), Error>> FillFn<1> for F {
fn make_vec(self, len: usize) -> Vec<u8> {
let mut v = Vec::with_capacity(len);
self(v.spare_capacity_mut()).unwrap();
unsafe { v.set_len(len) };
v
}
}
impl<F: Fn(&mut [MaybeUninit<u8>]) -> Result<&mut [u8], Error>> FillFn<2> for F {
fn make_vec(self, len: usize) -> Vec<u8> {
let mut v = Vec::with_capacity(len);
let ret = self(v.spare_capacity_mut()).unwrap();
assert_eq!(ret.len(), len);
assert_eq!(ret.as_ptr(), v.as_ptr());
unsafe { v.set_len(len) };
v
}
}

macro_rules! define_tests {
($fill:path) => {
use crate::tests::FillFn;
#[cfg(all(target_family = "wasm", target_os = "unknown"))]
use wasm_bindgen_test::wasm_bindgen_test as test;

#[test]
fn zero() {
$fill.make_vec(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getrandom::getrandom_uninit() does the !dest.is_empty() check before to avoid calling getrandom_inner with an empty dest, custom::getrandom_inner() doesn't have a similar check, but also we guarantee that the custom implementation will never be called with a zero-length dest. But, having this test here implies to me that we do expect every getrandom_inner() to correctly handle an empty dest. It seems contradictory. Perhaps instead zero() should be an integration test that tests specifically/only getrandom::getrandom() and getrandom::getrandom_uninit()? Or, perhaps we should move that check out of getrandom_uninit and into the getrandom_inner implementations since that would be more robust.

Regardless of how we address this, we should have a comment here explaining what we're expecting this test to be testing.

}
#[test]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's separate the functions inside the macro body with blank lines.

fn diff_large() {
crate::tests::check_diff_large(|len| $fill.make_vec(len));
}
#[test]
fn small() {
crate::tests::check_small(|len| $fill.make_vec(len));
}
#[test]
fn huge() {
$fill.make_vec(100_000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a comment here about why were not using check_diff_large

}
// On WASM, the thread API always fails/panics.
#[test]
#[cfg_attr(target_family = "wasm", ignore)]
fn multithreading() {
crate::tests::check_multithreading(|len| $fill.make_vec(len));
}
};
}
pub(crate) use define_tests;

mod init {
super::define_tests!(crate::getrandom);
}
mod uninit {
super::define_tests!(crate::getrandom_uninit);
}
100 changes: 0 additions & 100 deletions tests/common/mod.rs

This file was deleted.

11 changes: 0 additions & 11 deletions tests/normal.rs

This file was deleted.

22 changes: 0 additions & 22 deletions tests/rdrand.rs

This file was deleted.