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

fix(identity): fix test when no features are enabled #5137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stormshield-gt
Copy link

Description

On master branch the test for identity when they are no features enabled are currently broken. See

cargo test --package libp2p-identity
# or
cd identity/
cargo test

This PR add the required #[cfg()] to make the test pass.

Change checklist

Not sure if we need a changelog entry for something this small

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

The danger here is that you are accidentally excluding tests with bad feature flags so the never actually run despite being there in the code.

@stormshield-gt
Copy link
Author

The danger here is that you are accidentally excluding tests with bad feature flags so the never actually run despite being there in the code.

So those test should be fixed if they have bad features flags? I can do so in this PR if you want.
If in the future somebody add a test with bad features flags, it will quickly see if it's not running

The silver bullet here will be to go with a tool like cargo hack to test the combination of features, but while that's happening, the proposed solution + a pass to ensure tests have not bad feature flags, should do the job, in my opinion.

@thomaseizinger
Copy link
Contributor

If in the future somebody add a test with bad features flags, it will quickly see if it's not running

How? I can write a test that is feature-gated on #[cfg(feature = "ed2519")] (notice the typo?) and the test will never run if the crate doesn't provide an ed2519 feature.

Cargo does not warn about the use of features which are not defined in the manifest because they are just passed down to rustc and rustc has no knowledge of Cargo.toml files.

To be on the safe side, we shouldn't feature flag tests I think.

@stormshield-gt
Copy link
Author

How? I can write a test that is feature-gated on #[cfg(feature = "ed2519")] (notice the typo?) and the test will never run if the crate doesn't provide an ed2519 feature.

Intuitively I would have said that the next thing people do when writing a test is to run it, so they will detect the typo.

Anyway, maybe we should wait for rust-lang/rfcs#3013 to be stabilized then?

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.

None yet

2 participants