-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 |
||
mod test { | ||
#[cfg(target_arch = "wasm32")] | ||
use wasm_bindgen_test::wasm_bindgen_test as test; | ||
|
||
use super::*; | ||
|
||
#[cfg(feature = "serde")] | ||
#[test] | ||
fn aggregated_signature_fixture() { | ||
use base64ct::{Base64, Encoding}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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 |
||
mod test { | ||
use test_strategy::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.
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