rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
275 stars 180 forks source link

Cleanup Testing code #471

Open josephlr opened 3 months ago

josephlr commented 3 months ago

Depends on #473

This stops using weird include hacks to reuse testing code. Instead, we move the code to a normal tests.rs file and uses helper functions to avoid code duplication. Now when running cargo test the tests appear as normal unit tests:

   Compiling getrandom v0.2.15 (/home/joe/dev/rust/getrandom)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.23s
     Running unittests src/lib.rs (target/debug/deps/getrandom-1d050696e8dd96df)

running 6 tests
test error::tests::test_size ... ok
test tests::fill_large ... ok
test tests::fill_zero ... ok
test tests::multithreading ... ok
test tests::fill_small ... ok
test tests::fill_huge ... ok

Follow up PRs (which explain some of the design logic in this PR):

josephlr commented 3 months ago

CC @briansmith who is doing other testing improvements.

josephlr commented 3 months ago

Haiku failure is unrelated, fixed by from https://github.com/rust-lang/rust/pull/126212

josephlr commented 3 months ago

This PR tries to do three things, AFAICT:

* Refactoring how common logic is shared between tests.

* Add support for testing the `_uninit()` API directly.

* Change how we test custom implementations and what is being tested in those cases.

Each of these types of changes should be its own PR with its own discussion. I think if I had submitted this PR, nobody would want to review it because it mixes all of these things.

This is a very good point. I've reduced the scope of this PR (see the updated description), and I've split out some of the prerequisites into their own PRs (#473, #472). I've also moved testing of RDRAND / getrandom_uninit / file-fallback to followup PRs (#476 , #475 , #474).

@newpavlov @briansmith let me know if you thing the Byte-based generic code makes sense. Having a single method which is "give me a random Vec<u8>" seemed like the easiest way to express the differences between the u8 and MaybeUninit<u8> code.

newpavlov commented 3 months ago

I would prefer to keep bulk of our tests in the tests/ folder. I will try to experiment with the testing code a bit later.

josephlr commented 3 months ago

This PR is hard to reivew in the GitHub UI because the move of tests/common/mod.rs to src/tests.rs is combined with extensive edits of the file. If you could, please split moving the unmodified file into a separate commit and then rebase this commit over it, so that the PR consists of two commits. Then each commit in the PR can be reviewed in the GitHub UI individually.

Split into 3 commits:

I also closed #474 and #476 as those changes seem to work better as part of this PR (#475 is still its own PR).