Simplify node reload logic in tests#4142
Simplify node reload logic in tests#4142joostjager wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
|
👋 Hi! I see this is a draft PR. |
joostjager
force-pushed
the
real-persist
branch
from
a5c6ff1 to
ccd72e0
Compare
joostjager
force-pushed
the
real-persist
branch
from
ccd72e0 to
5d9d8d0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4142 +/- ##
==========================================
+ Coverage 88.63% 88.74% +0.11%
==========================================
Files 180 180
Lines 135230 135507 +277
Branches 135230 135507 +277
==========================================
+ Hits 119865 120261 +396
+ Misses 12594 12482 -112
+ Partials 2771 2764 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Store the latest persisted monitor in TestChainMonitor so it no longer needs to be passed into reload. This change also makes the test more realistic. If the monitor wasn't actually persisted, it won't reload properly.
joostjager
force-pushed
the
real-persist
branch
from
5d9d8d0 to
6688991
Compare
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
LGTM, I think we can land with 1 ack
|
|
||
| let mut deserialized_fwd_htlcs = nodes[0].node.forward_htlcs.lock().unwrap(); | ||
| for scid in [scid_1, scid_2].iter() { | ||
| let mut deserialized_fwd_htlcs = nodes[0].node.forward_htlcs.lock().unwrap(); for scid in [scid_1, scid_2].iter() { |
There was a problem hiding this comment.
nit: needs newline
| } | ||
|
|
||
| #[cfg(test)] | ||
| macro_rules! reload_node_and_monitors { |
There was a problem hiding this comment.
Split into two macros? 😛
There was a problem hiding this comment.
Hm why am I even adding new macros. Maybe I should fn'ify it.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
This means we can't write async-persist tests that load from a ChannelMonitor that never finished its persistence, afaict?
My thought was to use the new macro only when possible and with scenarios that are simple reloads. With the way the tests are currently set up (manual monitor serialization, without a connection to persister), I don't think it would be possible to test storing channels alongside channel monitors? Because for that, somehow the call to persister needs to store it's arguments, so that they can be used when reloading chan mgr. I guess that could also be done manually though...? |
Not sure I understand this? If we're storing channels in monitors (+ in channelmonitorupdates) it should be easy to test? |
I just wanted to avoid manually extracting the serialized channels in the test code and passing them in when reloading the node. But it seems that there are too many special cases to make this nice and clean. Closing this and going to try modifying the tests to get the serialized channels from TestPersister and passing them to reload node. |
|
I guess I'm still confused why channels are being serialized/handled separately at all, vs just being a field in |
4 participants
Store the latest persisted monitor in TestChainMonitor so it no longer needs to be passed into reload. This change also makes the test more realistic. If the monitor wasn't actually persisted, it won't reload properly.
This is a preparation for the larger channel manager refactor project.