Add test coverage of async BP and fix minor bug #2145
Conversation
TheBlueMatt
force-pushed
the
2023-04-async-bp-cleanups
branch
from
0618fbe
to
a1c1b02
Compare
|
Feel free to squash |
Instead of asserting a `Result` `is_ok`, we should always simply `unwrap` to get a backgrace, and we should avoid doing so if the thread is already panicking.
If `ChannelManager` is persistable before the async background processor even starts, it may not even get around to overwriting the `should_exit` flag before testing it, and the default value is (incorrectly) true, causing an immediate unconditional exit. The default value should simply be false. Fixes lightningdevkit#2140
If the user's sleep future passed to an async background processor only returns true for exiting once and then reverts back to false, we should exit anyway when we get a chance to. We do to this here by always ensuring we check the exit flag even when only polling sleep futures with no intent to (yet) exit. This is utilized in the tests added in the coming commit(s).
This finally gives us a bit of test coverage of the async BP, which was embarrassingly missing until now.
This gives us coverage of an async BP returning an error.
TheBlueMatt
force-pushed
the
2023-04-async-bp-cleanups
branch
from
a1c1b02
to
068d2c6
Compare
|
Squashed without further changes. |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2145 +/- ##
==========================================
+ Coverage 91.35% 91.38% +0.03%
==========================================
Files 102 102
Lines 49909 49852 -57
Branches 49909 49852 -57
==========================================
- Hits 45592 45556 -36
+ Misses 4317 4296 -21
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| @@ -1186,7 +1186,9 @@ mod tests { | |||
| let filepath = get_full_filepath("test_background_processor_persister_0".to_string(), "scorer".to_string()); | |||
| check_persisted_data!(nodes[0].scorer, filepath.clone()); | |||
|
|
|||
| assert!(bg_processor.stop().is_ok()); | |||
| if !std::thread::panicking() { | |||
| bg_processor.stop().unwrap(); | |||
There was a problem hiding this comment.
non-blocking nit: Since we're touching them, all of these could also be expect.
There was a problem hiding this comment.
I don't think it matters? Its a test and I think the expectation here is clear, we don't really need to document why we shouldnt get an error?
Also may help debug #2144, though now that I look at it I don't see how async-specific changes could have broken tests - the tests didn't test async at all lol.