-
Notifications
You must be signed in to change notification settings - Fork 2
fix: feature flags for tests #44
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
base: sdk-bindings
Are you sure you want to change the base?
Conversation
89ee32d
to
517224a
Compare
29e3ad0
to
b584ded
Compare
21bd720
to
e60ec91
Compare
741084a
to
9d0081c
Compare
What exactly is this PR doing? |
Adjust the feature gates so
|
test, | ||
feature = "ed25519", | ||
feature = "secp256k1", | ||
feature = "secp256r1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could have put the specific features on the specific tests, otherwise tests are only run together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this initially, but it ends up in a big mess and at the end also doesn't really test much anymore, because all the tests contain parts of the other types
@@ -161,14 +161,13 @@ pub struct ValidatorSignature { | |||
pub signature: Bls12381Signature, | |||
} | |||
|
|||
#[cfg(test)] | |||
#[cfg(all(feature = "serde", test))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this actually failing? Seems a bit annoying if we ever add more tests, we may forget to remove the serde feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not failing, but without the serde feature there are warnings for unused imports
@@ -435,7 +435,7 @@ impl crate::ObjectId { | |||
} | |||
} | |||
|
|||
#[cfg(test)] | |||
#[cfg(all(test, feature = "proptest"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not asking you to change but I guess I would have preferred putting them on individual tests, even if redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why though if all the tests inside need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, you also need the features for the individual imports then if you don't want the warnings for unused imports
Fix feature flags so
cargo test
without any additional features doesn't fail to compile anymoreTested by running following commands in crates/iota-sdk-types