p2p: tighten up Router and add tests#6044
Conversation
|
The race detector is triggering on Testify mocks accessing Will look into workarounds tomorrow. EDIT: Nevermind, the race happens when there is a mock failure and it's trying to display details, specifically this line: m.fail("\nassert: mock: The method has been called over %d times.\n\tEither do one more Mock.On(\"%s\").Return(...), or remove extra call.\n\tThis call was unexpected:\n\t\t%s\n\tat: %s", call.totalCalls, methodName, callString(methodName, arguments, true), assert.CallerInfo()) |
Codecov Report
@@ Coverage Diff @@
## master #6044 +/- ##
==========================================
+ Coverage 59.85% 60.88% +1.02%
==========================================
Files 274 275 +1
Lines 25248 25334 +86
==========================================
+ Hits 15112 15424 +312
+ Misses 8520 8317 -203
+ Partials 1616 1593 -23
|
tessr
left a comment
There was a problem hiding this comment.
This (huge) PR was (still) very readable! Nice work.
| // Validate validates the options. | ||
| // Validate validates router options. | ||
| func (o *RouterOptions) Validate() error { | ||
| return nil |
There was a problem hiding this comment.
Are there anticipated future RouterOptions that might need to be validated?
There was a problem hiding this comment.
No idea, but possible. Could just remove it until it's needed too.
Thanks! Should ideally have split out some of the changes, but there were a bunch of fairly minor changes and I was short on time. Thanks for reviewing! |
This cleans up the
Routercode and adds a bunch of tests. These sorts of systems are a real pain to test, since they have a bunch of asynchronous goroutines living their own lives, so the test coverage is decent but not fantastic. Luckily we've been able to move all of the complex peer management and transport logic outside of the router, as synchronous components that are much easier to test, so the core router logic is fairly small and simple.This also provides some initial test tooling in
p2p/p2ptestthat automatically sets up in-memory networks and channels for use in integration tests. It also includes channel-oriented test asserters inp2p/p2ptest/require.go, but these have primarily been written for router testing and should probably be adapted or extended for reactor testing.