We need mock LCM interfaces for unit testing

[david-german-tri]

Right now, there are two prevailing approaches for unit-testing code that sends or receives LCM:

1. Actually put the bytes on the network. This is bad because it has side effects on any other program that happens to be listening.
2. Break encapsulation to inspect the data the code under test believes it put on the network, or received from the network. This is bad because test-only API features are clutter at best, and hazards to the unwary at worst.

A better solution would be to use a mock and dependency injection.

[david-german-tri]

Cleaning up #3494 would be a good first use case (but this feature should not block #3494).

[liangfok]

Current Post

I'm trying to understand how #3494 could be modified to address this issue. Here's my first guess:

1. Create a new abstract pure virtual parent class called DrakeLcmInterface that contains the following methods:
   • publish()
   • subscribe()
2. Create two sub-classes of DrakeLcmInterface, one that contains a real lcm::LCM object and another one that contains a mock drake::LcmMock object. The mock object contains the following additional method:
   • get_last_message_sent()
3. Modify RigidBodyTreeVisualizerLcm's constructor to take as input a DrakeLcmSubsystem instead of a lcm::LCM.

Is the above what's meant by "use a mock and dependency injection"?

Original Post

I'm trying to understand how #3494 could be modified to address this issue. Here's my first guess:

1. Create a new abstract parent class called DrakeLcmSubsystem that contains the following methods:
   • get_last_message_sent()
   • publish()
   • subscribe()
2. Create two sub-classes of DrakeLcmSubsystem, one that contains a real lcm::LCM object and another one that contains a mock drake::LcmMock object.
3. Modify RigidBodyTreeVisualizerLcm's constructor to take as input a DrakeLcmSubsystem instead of a lcm::LCM.

Is the above what's meant by "use a mock and dependency injection"?

[jwnimmer-tri]

Nope.

The basic premise of mocking is that (essentially) no Drake code ever says lcm::LCM as a type. Instead, we program against DrakeLcmInterface, which offers comparable APIs to lcm::LCM. In real simulations, class DrakeLcm : public DrakeLcmInterface has-a lcm::LCM and proxies the methods over to it; in unit tests, class StubLcm : public DrakeLcmInterface mocks up its behavior, for example remembering all messages that are sent.

[liangfok]

@jwnimmer-tri: I'm having trouble figuring out what's the difference between your description and my proposal. Is the only difference the name DrakeLcmSubsystem vs. DrakeLcmInterface? I would be happy to use DrakeLcmInterface.

[jwnimmer-tri]

Ah yes. The name confused me -- they are indeed similar. It's important to use Interface -- it should really be entirely pure virtual.

Also typically the get_last_message_sent would not be on the interface, just the mock. Also, the mock would have storage directly, not has-a some other mock class.

[liangfok]

Got it. Thanks. I'll go ahead and assign this to myself since I plan on implementing it after #3494 is merged.

[liangfok]

I'm going to start work on this now.

[liangfok]

Must the mock LCM instance be so decoupled from LCM that it should be usable even if LCM is not present on the local machine? I ask because the lcm::LCM::subscribe() API includes things like lcm::ReceiveBuffer and lcm::Subscription that will be undefined if LCM is not present. Do I need to mock up all of these classes as well?

[jwnimmer-tri]

I suspect any reasonable testing with an LCM mock will be still using non-mocked Encode and Decode methods, so LCM-as-a-build-dependency is likely fine (though perhaps nice to remove, if we can).

However, perhaps you can offline explain your mock design, because I am surprised that such types would still be needed in mock-land.

[liangfok]

From F2F meeting with @jwnimmer-tri, the mock LCM::Subscription API will look something like:

  template <class MessageType, class MessageHandlerClass>
  void subscribe(const std::string& channel,
      void (MessageHandlerClass::*handlerMethod)(const uint8_t* rbuf,
          const std::string& channel, const MessageType* msg),
      MessageHandlerClass* handler) = 0;

Note that lcm::Subscription and lcm::ReceiveBuffer will be pushed into a child class that uses the real LCM instance.

[liangfok]

WIP branch is here: https://github.com/liangfok/drake/tree/feature/lcm_mock. I have the basic framework completed and the real LCM-encapsulating class implemented and unit tested. I still need to implement the mock version and to integrate the framework with all of the System 2.0 systems that use LCM.

[david-german-tri]

🎉 very nice! thanks again!

[liangfok]

You're welcome!