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

Cleanup Testing code #471

wants to merge 3 commits into from

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Jun 10, 2024

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 josephlr requested a review from newpavlov June 10, 2024 11:38
@josephlr
Copy link
Member Author

CC @briansmith who is doing other testing improvements.

@josephlr
Copy link
Member Author

Haiku failure is unrelated, fixed by from rust-lang/rust#126212

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

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.

src/rdrand.rs Outdated Show resolved Hide resolved
src/tests.rs Outdated Show resolved Hide resolved
src/tests.rs Outdated Show resolved Hide resolved
@josephlr
Copy link
Member Author

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.

josephlr added a commit that referenced this pull request Jun 12, 2024
This simplifies lib.rs and allows for our testing code to be less
convoluted. Split out from #471. CC @briansmith

The MSRV for rand 0.9 is 1.61 and Debian stable is 1.63, so this is a
fine MSRV bump.

Signed-off-by: Joe Richey <[email protected]>
@josephlr josephlr force-pushed the testing branch 5 times, most recently from bae28dd to 14ba262 Compare June 19, 2024 10:26
@newpavlov
Copy link
Member

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.

Copy link
Contributor

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

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.

src/lib.rs Outdated
test,
not(all(target_family = "wasm", target_os = "unknown", feature = "custom"))
))]
pub(crate) mod tests;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "normal" tests should still be defined in tests/normal.rs (See comment below).

I think we're all keen to avoid duplicating complex cfg all over. I suggest we add a new api:

/// true if the implementation defined by `register_custom_getrandom!` would be used; false if it will be ignored.
pub const CUSTOM_USED: bool;

Then we can condition all the entropy checks in the common tests on that new API.

I think this API would also be useful to external users.

I also expected to see here something like:

cfg_if::cfg_if! {
    if #[cfg(any(target_arch = "x86", target_arch = "x86_64")] {
        mod rdrand;
        #[cfg(test)]
        mod rdrand_tests {
            tests::define_tests!(rdrand::getrandom_uninit);
        }
    }
}

This way, tests/rdrand.rs is properly replaced instead of just being lost.

Then presumably in future PRs we would do the same for use_file, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're all keen to avoid duplicating complex cfg all over. I suggest we add a new api:

After rearranging some stuff, this cfg is no longer needed. We can just do #[cfg(test)] and everything works as expected. Note that the CI tests are updated to run wasm-pack test --node --features=custom --test=custom when testing on an unsupported platform. This makes more sense to me, as the in-library unit tests will not build if you just enable the custom feature on an unsupported platform (you have to register a custom handler, which is only done in our integration test).

I also expected to see here something like:

cfg_if::cfg_if! {
    if #[cfg(any(target_arch = "x86", target_arch = "x86_64")] {
        mod rdrand;
        #[cfg(test)]
        mod rdrand_tests {
            tests::define_tests!(rdrand::getrandom_uninit);
        }
    }
}

This way, tests/rdrand.rs is properly replaced instead of just being lost.

I changed this PR to just unconditionally have mod rdrand present if the "rdrand" feature is enabled. This is similar to what we do for "custom", and allows us to just have normal unit tests inside rdrand.rs. We can't put such things inside the cfg_if, but having the mod rdrand outside of that macro ends up being more readable anyway.

src/tests.rs Outdated
}

// A function which fills a buffer with random bytes.
type FillFn<B> = fn(&mut [B]) -> Result<(), Error>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be type FillFn<B, R> = fn(&mut [B]) -> Result<R, Error> so that all of getrandom::getrandom_uninit, getrandom::getrandom, and the internal getrandom_impl functions can all be used with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, this doesn't work. Due to lifetime-related problems, there's no way to have a type alias which refers to the type of all 3 of those functions. Specifically, there's no R for which:

getrandom_uninit: FillFn<B,R>

as the full type is:

getrandom_uninit: for<'a> fn(&'a mut [MaybeUninit<u8>]) -> Result<&'a mut [u8], Error>

Instead, I changed things to have FillFn be a trait (implemented by the relevant Fn objects) which defines a make_vec method. The only downside is that it requires a dummy generic parameter to avoid conflicting implementations. Let me know what you think.

src/tests.rs Outdated
}
pub(crate) use define_tests;

define_tests!(crate::getrandom);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect that instead of having this line here, we could have it in tests/normal.rs:

#[path = "../src/tests.rs"]
mod tests;

mod init {
    define_tests!(getrandom::getrandom);
}
mod uninit {
    define_tests!(getrandom::getrandom_uninit);
}

This way we're defining integration tests in the normal way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Conceptually, I consider the tests created by define_tests! to be unit tests rather than integration tests (they just happen to test the public API), so it makes more sense to me for them to use the unit testing framework. Having them as unit tests also avoids #[path = "..."] shenanigans (which can be brittle and annoying at times).

@josephlr josephlr force-pushed the testing branch 2 times, most recently from 79f3444 to a04b6f3 Compare June 20, 2024 08:31
Signed-off-by: Joe Richey <[email protected]>
@josephlr
Copy link
Member Author

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:

  • Rename/Delete files
  • Common tests for getrandom and getrandom_uninit
  • Restoring RDRAND testing

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

@josephlr josephlr requested a review from briansmith June 21, 2024 01:45
Copy link
Contributor

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

The main objection I still have is moving all these tests from integration tests to unit tests. The #[path] stuff is suboptimal and we should reduce our usage of it. But, I think we should also still stick to preferring having integration tests for getrandom::getrandom and getrandom::getrandom_uninit and more generally the whole public API.


#[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.

fn zero() {
$fill.make_vec(0);
}
#[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.

}
#[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

@newpavlov
Copy link
Member

I think #504 has resolved most of the issues targeted by this PR.

@newpavlov newpavlov closed this Oct 7, 2024
@newpavlov newpavlov deleted the testing branch October 11, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants