tikv / fail-rs

Fail points for rust
Apache License 2.0
332 stars 39 forks source link

cargo: add a "failpoints" feature #37

Closed lucab closed 5 years ago

lucab commented 5 years ago

Breaking: this changes the library behavior, disabling code generation (in macro) by default.

It introduces a failpoints Cargo feature that consumers should only enable where relevant (e.g. in testing environments).

Transitive consumers of this library can enable this feature too, with the special name fail/failpoints (this requires a recent cargo binary).

Closes: https://github.com/pingcap/fail-rs/issues/35

lucab commented 5 years ago

/cc @brson @BusyJay for review

BusyJay commented 5 years ago

You need to update all the CI configuration files to run tests with failpoints feature.

lucab commented 5 years ago

An overall update:

A middle-ground solution could be to make this cfg(any(debug_assertions, feature = "failpoints")), so that code is always generated in debug/testing mode (i.e. cargo test), but it's strictly opt-in in release mode (i.e. cargo build --release). I'm not huge a fan of this as it introduces more complexity and troublesome sharp edges (think of cargo test --release, see https://github.com/pingcap/rust-prometheus/pull/246), but at least it wouldn't need changes in most consumers' CI. Another drawback is that it won't be anymore possible to disable for debug builds (i.e. cargo build).

BusyJay commented 5 years ago

I'm OK with overall changes. Well done @lucab! The only thing I concerns about is the feature name, but I don't have strong opinions.

lucab commented 5 years ago

@BusyJay @overvenus thanks for reviewing and merging this! Would you mind a git tag and cargo publish for 0.3.0? That would also close https://github.com/pingcap/fail-rs/issues/31.

BusyJay commented 5 years ago

Yes, I will publish this once #32 is merged. Maybe in 24 hours.