The PR branch HEAD was f98a4e8d89 at the time of this review club meeting.
Notes
PR 25515 is a refactoring
change that allows unit-testing of the
PeerManager
class, by making CConnman mockable. CConnman is made mockable by defining
an abstract interface class, ConnectionsInterface, that CConnman then
inherits from.
Currently, PeerManager holds a reference to CConnman
(PeerManagerImpl::m_connman)
and the PR changes the type of that reference to ConnectionsInterface.
Additionally, when a PeerManager instance is created, we now pass a
ConnectionsInterface instead of a CConnman. With that, any unit tests can
create a mocked version of the connection manager to test the internals of
PeerManager.
To demonstrate the added benefit, the PR also adds two initial PeerManager
unit tests for the version and ping/pong messages (see
peerman_tests.cpp).
PeerManager is currently almost exclusively tested through our functional
tests (See the test/functional/p2p_*.py tests). These tests spin up entire
nodes and test functionality through the interfaces that nodes provide (p2p,
rpc, rest, etc.).
Why do we need unit tests? Aren’t functional tests sufficient?
Why do we need to mock CConnman to unit test PeerManager?
Do mock interfaces need to implement all of the real interfaces?
Should we mock other components as well? (e.g.: CTxMempool,
ChainstateManager)
Roughly, what is the overhead of using an interface class?
How can you tell that a class is an interface class?
What do the = 0 indicate at the definition of each ConnectionsInterface
method? What would happen if they weren’t there?
CConnMan and ConnectionsInterfaceMock both are classes derived from ConnectionsInterface.
Why are their methods specified with the override keyword? (For example,
PushMessage)
What is the general approach of the
version_handshake
test? Why does it contain the series of calls to BOOST_CHECK_EQUAL()?
What is the general approach of the
ping
test? Why does it mock the current time?
<LarryRuane> don't hesitate to ask questions! Even if we've already moved on to the next topic, feel free to ask questions about something we've already covered
<raj-> My understanding is its useful for unit situations where we need to mimic some behaviour but we can't directly instantiate a concrete class.. Here we can't have real CConnMan as there is no real networking in unit tests..
<LarryRuane> amovfx: Yes excellent ... Here's how I think of it ... software, at least if it's well-written, is implemented in layers, and if you want to test a given layer, it will need the layers below it (call on its methods) ... but it may not be possible or practical to include all of that lower layer's functionality in the test setup
<LarryRuane> raj-: yes, good idea.. I think of it as, peer manager decides to send certain messages, and processes received ones ... connection manager is the low-level moving-bytes-over-the-wire kind of stuff
<LarryRuane> it's just the PeerManager is high-level things (like sequencing the multiple messages in a version handshake), and connection manager is dealing with low level stuff (like the addresses of peers)
<glozow> functional tests have a ton of overhead that makes them ill-equipped to target specific pieces of internal logic, and really really slow. we can't test eviction logic in isolation; we have to spin up a bunch of nodes, wait for evictions to be triggered, mine real blocks, to feed to the node, etc. etc.
<stratospher[m]> functional tests are more time consuming and resource intensive to set up. unit tests would be easier to debug since behaviour is compartmentalised.
<LarryRuane> brunoerg: yes I like that, glozow: good points, also functional tests can't test as deeply because they're limited to the RPC and P2P interfaces
<LarryRuane> raj-: lightlike: Exactly, that's what I was getting (question not worded well) .. All must be defined, but no, only the ones that the test causes the production code-under-test to invoke need to be (non-trivially) implemented
<LarryRuane> see how (at that link) `ForNode()` is just an empty function (`{}`) ... it must be defined but doesn't need to do anything because the test code doesn't cause the peer manager to call it!
<raj-> Question: What guides the separation between interface functions and *additional* function of the derived type, while creating an interface like this?
<LarryRuane> lightlike: +1 raj-: I think what's in the interface are at least those functions that need to currently be mocked, but more can be added later (as more tests are written)
<raj-> LarryRuane, yes that makes sense in my head.. So we can say that this interface is "dynamic" and can be modified later depending upon test requirements?
<LarryRuane> Hope that answers things, speak up if we missed anything ... question 5: Should we mock other components as well? (e.g.: `CTxMempool`, `ChainstateManager`)?
<LarryRuane> raj-: I'd say ideally it should decrease code complexity by separating the layers more clearly, instead of everything being spaghettied (?) together
<LarryRuane> So for question 5, I think we're on the same page that other classes should be mocked, but doing so would have to be driven by the tests that people are writing
<LarryRuane> amovfx: correct, the new interface class `ConnectionsInterface` must be used in production code (by the peer manager), but the interface class *allows* unit tests
<LarryRuane> raj-: I hadn't noticed that, good question! pablomartin: That's my understanding, it would let the friend class's methods (the mock functions in this case) to access the private members of CConnman (for example)
<LarryRuane> I think the idea is that test code needs to be able to dig into data structures, violate the layering so to speak, that you don't want production code to be able to do
<kouloumos> Regarding a change like this that maybe results to the compiler not be able to inline some functions. How could someone measure the performance of such a change?
<LarryRuane> Ok we're about out of time, let me skip ahead to question 10: What is the general approach of the version_handshake test? Why does it contain the series of calls to BOOST_CHECK_EQUAL()?
<LarryRuane> Yes those are good answers, I was thinking it could maybe test that the p2p messages were being sent in the correct order (rather than just the number of times they were sent)
<LarryRuane> Well we're out of time, sorry we didn't get to every question, thank you all for coming! feel free to stay here and continue the discussion!