search

tikv / fail-rs

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

fail: add test-mutex pattern directly to the library #40

Closed lucab closed 5 years ago

lucab commented 5 years ago

This adds a FailScenario to the library, exposing the usual idiom of a global testing mutex and individual guards owned by each test.

The global mutex is kept as an internal detail and not exposed, consumers can only borrow a guard via the scenario.

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

lucab commented 5 years ago

/cc @BusyJay @brson @overvenus for review.

Do Not Merge (DNM): I haven't tested this on any consumer yet. It would be good if somebody familiar with tikv test-base could try to drop this one in place there and confirm I haven't missed/overlooked anything, before merging.

I'm not in a hurry to land this, I can wait till after 0.3 release (see https://github.com/pingcap/fail-rs/pull/37#issuecomment-501606095).

lucab commented 5 years ago

I updated tikv testsuite to consume this: https://github.com/tikv/tikv/pull/4903. CI is green. This is ready for review and fine to land before 0.3, if you like.

BusyJay commented 5 years ago

This is so cool! @brson would you like to take a look?

brson commented 5 years ago

Seem like after this we should finally bump to 0.3, and integrate this pattern into tikv per @luca's patch.

brson commented 5 years ago

The issue to bump to 0.3 is https://github.com/pingcap/fail-rs/issues/31. I know I don't have the permissions to tag and publish. So probably @BusyJay or @overvenus will need to do that.

lucab commented 5 years ago

@brson I've updated the docs to fix the example, and explained the reason for keeping cfg as a free-function: https://github.com/pingcap/fail-rs/pull/40#discussion_r294685926.

@BusyJay after https://github.com/pingcap/fail-rs/pull/32, the appveyor configuration should be removed. It is still triggering (and failing) on PRs. I think it is under your account.

lucab commented 5 years ago

Gentle bump.

Hoverbear commented 5 years ago

Thanks for this Luca! :)

lucab commented 5 years ago

Rebased on latest master to get rid of appveyor hanging status.

(EDIT: that seems to have dropped the two LGTMs too, sorry)

brson commented 5 years ago

Thanks and sorry for the delay @lucab

lucab commented 5 years ago

@brson no prob. Actually, thanks for the reviews!