fail: add test-mutex pattern directly to the library #40
Conversation
|
/cc @BusyJay @brson @overvenus for review.
|
lucab
force-pushed
the
ups/scenario
branch
2 times, most recently
from
eddfd5f
to
15898ce
Compare
lucab
changed the title
|
I updated |
|
This is so cool! @brson would you like to take a look? |
There was a problem hiding this comment.
Thanks for taking an interest in this crate @lucab. I think I'm happy with this approach. Though it does force users to use the guard pattern, as you point out in the README it's simple to just apply the guard to the entire program. I did make a note asking to move cfg from a free function to a method.
src/lib.rs
Outdated
| //! let _gaurd = setup(); | ||
| //! fail::cfg("read-dir", "panic").unwrap(); | ||
| //! let scenario = FailScenario::setup(); | ||
| //! scenario.cfg("read-dir", "panic").unwrap(); |
There was a problem hiding this comment.
It looks to me like in this patch cfg is still a free function. Can you make it a method on FailScenario as depicted here?
There was a problem hiding this comment.
That was my initial approach, but I had to revert back to a free function. This doc error is a leftover I didn't catch when undoing.
The reason why this needs to be a free function is that there is a case for consumers like this one in tikv: https://github.com/tikv/tikv/blob/82de1403905cb12ed988d85fa8da2ab99fb0adde/src/server/service/debug.rs#L269.
There, fail::cfg is used in project logic to override fail-points configuration, if any. That is decoupled from the scenario, thus we can't use FailScenario::cfg there.
There was a problem hiding this comment.
If both fail::cfg and fail_point! can be made as methods of FailScenario, maybe we don't need the global lock at all. And tests can be run in parallel.
But I'm happy with current approach as it makes life easier at least.
There was a problem hiding this comment.
Maybe this could be another PR? :)
There was a problem hiding this comment.
Yes, of course.
Changing them into methods also loses the flexibility of macros, which can be put to anywhere you want without any context. However, it maybe worthy to speedup tests when failpoint are used heavily like TiKV.
There was a problem hiding this comment.
I think this is the same topic as #24. I guess, we can brainstorm that design and its implication there a bit more. From what I can see, it will be a major/invasive change anyway.
|
Seem like after this we should finally bump to 0.3, and integrate this pattern into tikv per @luca's patch. |
|
The issue to bump to 0.3 is #31. I know I don't have the permissions to tag and publish. So probably @BusyJay or @overvenus will need to do that. |
lucab
force-pushed
the
ups/scenario
branch
2 times, most recently
from
153122c
to
9ca990f
Compare
|
@brson I've updated the docs to fix the example, and explained the reason for keeping @BusyJay after #32, the appveyor configuration should be removed. It is still triggering (and failing) on PRs. I think it is under your account. |
|
Gentle bump. |
|
Thanks for this Luca! :) |
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.
|
Rebased on latest master to get rid of appveyor hanging status. (EDIT: that seems to have dropped the two LGTMs too, sorry) |
|
Thanks and sorry for the delay @lucab |
|
@brson no prob. Actually, thanks for the reviews! |
This adds a
FailScenarioto the library, exposing the usual idiomof 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: #23