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 Custom Tests #473

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Cleanup Custom Tests #473

merged 1 commit into from
Jun 19, 2024

Conversation

josephlr
Copy link
Member

This moves the tests for the custom RNG registration into custom.rs. It also:

  • Simplifies the registered custom RNG
  • Makes sure the custom RNG is not used on supported platforms
  • Makes sure the custom RNG is used on unsupported platforms

Split out from #471 CC @briansmith

src/custom.rs Outdated Show resolved Hide resolved
src/custom.rs Outdated Show resolved Hide resolved
src/custom.rs Outdated Show resolved Hide resolved
tests/custom.rs Show resolved Hide resolved
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

This looks fine to me, but I am not sure we need to specifically test that custom implementations do not overwrite the default backends.

Also, I have an alternative proposal. I think we can do the following:

// in lib.rs
cfg_if! {
    if #[cfg(all(test, test_rdrand))] {
        mod lazy;
        #[path = "rdrand.rs"] mod imp;
    } else if #[cfg(all(test, test_custom, feature = "custom"))] {
        use custom as imp;
    } else if #[cfg( ... ))] {
        // ...
    }
}

// in tests module:
register_custom_getrandom!(mock_rng);

#[cfg(not(all(feature = "custom", test_custom)))]
#[test]
fn mock_rng_is_not_used() {
    // ..
}

This will eliminate the weird juggling around RDRAND and will allow us to test a custom implementation on standard Linux targets. Using cfg(test) will ensure that the testing branches will not escape outside of this crate.

@josephlr
Copy link
Member Author

josephlr commented Jun 19, 2024

@newpavlov @briansmith PTAL. I moved the custom.rs tests back to being an integration test, so both supported and unsupported platforms are now tested without changing the cfg_if! in lib.rs.

This looks fine to me, but I am not sure we need to specifically test that custom implementations do not overwrite the default backends.

Also, I have an alternative proposal. I think we can do the following:

I think its important to test that:

  • The custom backend works
  • The custom backend won't override existing implementations

My concern with changing the lib.rs logic is that by adding too many test cfgs, we might end up not actually testing what we want.

This will eliminate the weird juggling around RDRAND and will allow us to test a custom implementation on standard Linux targets. Using cfg(test) will ensure that the testing branches will not escape outside of this crate.

I agree that RDRAND has some weird juggling, but that's probably best left for discussion in #476

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 makes a lot more sense to me. Just a couple comments.

tests/custom.rs Outdated Show resolved Hide resolved
tests/custom.rs Outdated Show resolved Hide resolved
tests/custom.rs Show resolved Hide resolved
This marks the `tests/custom.rs` test as requiring the `"custom"`
feature. It also:
  - Simplifies the registered custom RNG
  - Makes sure the custom RNG _is not_ used on supported platforms
  - Makes sure the custom RNG _is_ used on unsupported platforms

Signed-off-by: Joe Richey <[email protected]>
@newpavlov newpavlov merged commit f345775 into master Jun 19, 2024
54 checks passed
@newpavlov newpavlov deleted the test_custom branch June 19, 2024 09:20
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