-
Notifications
You must be signed in to change notification settings - Fork 412
Add tests for routing message handler #570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for routing message handler #570
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good as-is (there's always more to test, but this is a nice start), let me know if you plan on adding more stuff (since its tagged WIP) or if you want this to be merged. An obvious additional test may be testing that excess_data results in the announcement(s) being accepted, but not relayed (ie Ok(false) as a return value).
@TheBlueMatt I want to do all routing handlers within this PR. I'll let you know once it's fully ready for review! |
@TheBlueMatt ready for review! I left some questions along my tests. Answering those would help me to get better intuition on the current state of the repo and approaches to things, so I'd be grateful. |
e807e87
to
b1bafd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad this is getting some testing, but it seems a bunch of the internal state modifications are causing invalid states, which leave your comments that panics shoulnd't occur.
b1bafd0
to
1690c40
Compare
Codecov Report
|
@TheBlueMatt I think I addressed all your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One comment, though.
1690c40
to
14d10e5
Compare
Done! |
14d10e5
to
f9f90e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The comments are mostly all nits, so you're welcome to chose to ignore some, but get_next_channel_announcements probably doesn't make sense as-is.
let network = router.network_map.write().unwrap(); | ||
assert_eq!(network.channels.len(), 0); | ||
// Nodes are also deleted because there are no associated channels anymore | ||
assert_eq!(network.nodes.len(), 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why does it end with 1? (and maybe test that it starts with 1 at the top).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is "our" node, the one we initiate a router with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add an assert at the top of the test + a comment indicating that, I think.
lightning/src/ln/router.rs
Outdated
} | ||
|
||
// No updates. | ||
let next_announcements = router.get_next_channel_announcements(channel_key, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I actually think this is a bug, and have a commit to fix it that I haven't gotten around to PR'ing yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, so should we wait when you merge that first then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. You're welcome to take the top commit from https://github.com/TheBlueMatt/rust-lightning/tree/2020-04-relay-no-node in this PR or I can do it separately and you can drop the relevant commit from this PR.
lightning/src/ln/router.rs
Outdated
}; | ||
} | ||
|
||
// Updates with empty messages are not returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
80e676e
to
82c5995
Compare
@TheBlueMatt Addressed latest suggestions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Travis found that a few spots don't compile with older rustcs that we support (I suggested changes that I think will fix it, but no guarantees).
lightning/src/ln/router.rs
Outdated
let (_, update_1, update_2) = channel_announcements; | ||
assert_ne!(update_1.clone(), None); | ||
assert_eq!(update_2.clone(), None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let (_, update_1, update_2) = channel_announcements; | |
assert_ne!(update_1.clone(), None); | |
assert_eq!(update_2.clone(), None); | |
let &(_, ref update_1, ref update_2) = channel_announcements; | |
assert_ne!(update_1, &None); | |
assert_eq!(update_2, &None); |
lightning/src/ln/router.rs
Outdated
let (_, update_1, update_2) = channel_announcements; | ||
assert_eq!(update_1.clone(), None); | ||
assert_eq!(update_2.clone(), None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let (_, update_1, update_2) = channel_announcements; | |
assert_eq!(update_1.clone(), None); | |
assert_eq!(update_2.clone(), None); | |
let &(_, ref update_1, ref update_2) = channel_announcements; | |
assert_eq!(update_1, &None); | |
assert_eq!(update_2, &None); |
lightning/src/ln/router.rs
Outdated
let (_, update_1, update_2) = channel_announcements; | ||
assert_eq!(update_1.clone(), None); | ||
assert_eq!(update_2.clone(), None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let (_, update_1, update_2) = channel_announcements; | |
assert_eq!(update_1.clone(), None); | |
assert_eq!(update_2.clone(), None); | |
let &(_, ref update_1, ref update_2) = channel_announcements; | |
assert_eq!(update_1, &None); | |
assert_eq!(update_2, &None); |
Also needs rebase on master. |
82c5995
to
79c8491
Compare
@TheBlueMatt done. Thanks for patience :) |
Oops, I had the github settings set wrong, turns out didn't need a rebase. Anyway, looks great! |
Addressing the issue #524