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

Align tests with features #10

Merged
merged 3 commits into from
May 24, 2024

Conversation

dkg
Copy link
Contributor

@dkg dkg commented May 22, 2024

I was experimenting with variants like:

cargo test --no-default-features --features ml-kem-512

and noticed that many permutations of features will cause the test suite to fail in different ways.

This series annotates the tests (including the doctests) with the appropriate features to ensure that they should normally pass.

The one scenario where they don't pass is if none of the ml-kem-512, ml-kem-768, or ml-kem-1024 features are enabled. That is, these two ways of running still fail:

cargo test --no-default-features
cargo test --no-default-features --features default-rng

I think that's probably OK. It would be nice if Cargo.toml could somehow be annotated explicitly that at least one of the ml-kem-* features must be enabled. I don't know how to do that, though.

Note that the first commit in this series actually tunes up one of the documentation examples so that it doesn't rely on the default-rng feature, before annotating it for reliance on ml-kem-512. If you think it really should depend on both ml-kem-512 and default-rng even though it uses a *_with_rng function elsewhere, please go ahead and adjust it in that way.

dkg added 3 commits May 22, 2024 00:27
This documentation example is showing how to use a dedicated RNG.  The
example shouldn't depend on the default-rng feature being enabled.
This lets the doctests pass as at least one of the ml-kem-512,
ml-kem-768, or ml-kem-1024 features are enabled.
This lets the standard tests succeed as long as one of the ml-kem-512,
ml-kem-768, or ml-kem-1024 features are enabled.
@dkg
Copy link
Contributor Author

dkg commented May 22, 2024

I looked at the errors in the clippy test, and they look like this:

error: item has both inner and outer attributes
   --> src/lib.rs:322:1
    |
322 | / /// Functionality for the ML-KEM-512 security parameter set, which is claimed to be in security category 1, see
323 | | /// table 2 & 3 on page 33 of spec.
324 | | #[cfg(feature = "ml-kem-512")]
325 | | pub mod ml_kem_512 {
...   |
338 | |     //!
339 | |     //! **--> See [`traits`] for the keygen, encapsulation, decapsulation, and serialization/deserialization functionality.**
    | |_____________________________________________________________________________________________________________________________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mixed_attributes_style
note: the lint level is defined here
   --> src/lib.rs:2:27
    |
2   | #![deny(clippy::pedantic, warnings, missing_docs, unsafe_code)]
    |                           ^^^^^^^^
    = note: `#[deny(clippy::mixed_attributes_style)]` implied by `#[deny(warnings)]`

This series doesn't touch src/lib.rs at all, or add any inner attributes, for that matter. So i think the test failure is unrelated to the proposed changes here. Indeed, i think this might be a bug in clippy's mixed_attributes_style linter, which might be misidentifying //! …[ as an inner attribute (why else would it show lines 338 and 339 in the error message?)

@integritychain integritychain merged commit 9f7e0df into integritychain:main May 24, 2024
29 of 30 checks passed
@integritychain
Copy link
Owner

Thanks for your work here, I like it! I think I may know what's up with clippy and can fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants