YASL Request

[Patrick Venture]

On Tue, Apr 10, 2018 at 8:52 PM, Brad Bishop
<bradleyb at fuzziesquirrel.com> wrote:
>
>> On Apr 9, 2018, at 10:45 PM, Lei YU <mine260309 at gmail.com> wrote:
>>
>> On Tue, Apr 10, 2018 at 6:27 AM, Patrick Venture <venture at google.com> wrote:
>>> Everyone,
>>>
>>> I'm working on unit-testing in openbmc, and have cracked most of
>>> sdbusplus into mockable pieces and verified I can in fact test against
>>> those mocks with a downstream daemon.  I'll be grabbing an upstream
>>
>> Great! I have tried to make sdbusplus mockable previously, by changing its
>> interfaces virtual, and find out that it is somehow complicated because some
>> of the interfaces return the objects and it's kind of hard to mock things like
>> bus::new_method_call().
>> At that time I discussed this with Patrick Williams and he suggested sdbusplus
>> should be compact and fast, so it's not a good idea to make it virtual.
>
> FWIW, I agree with Patrick W’s sentiment here.  I’m all for unit tests, but
> un-optimizing existing code so we can use a specific framework in a specific
> way doesn’t sit well with me.
>
>> Later it's found that Brad has some good example of mocking sdbusplus in
>> https://github.com/openbmc/phosphor-dbus-monitor/tree/master/src/test
>
> Yes, I was able to mock sdbusplus with GMock without having to change sdbusplus
> at all.  Patrick, is something like that on the table?  It seems like instead of
> the typical:
>
> class iface
> {
>     virtual void sdbusplus_foo() = 0;
> };
>
> clase real : public iface
> {
>     void sdbusplus_foo() {}
> };
>
> class fake : public face
> {
>     MOCKME(sdbusplus_foo);
> };
>
> int main()
> {
>     real r;
>     r.sdbusplus_foo();
> }
>
> void testcase()
> {
>     fake f;
>     EXPECT_BLAH_BLAH();
>     f.sdbusplus_foo();
> }
>
> ————————————————
> we could do:
>
> class real
> {
>     void sdbusplus_foo() {}
> };
>
> class fake
> {
>     MOCKME(sdbusplus_foo);
> };
>
> template <typename T>
> MyClass
> {
>     MyClass(T& interface)
>     {
>         interface.sdbusplus_foo();
>     }
> };
>
> int main()
> {
>     real r;
>     MyClass<real> m(r);
> }
>
> void testcase()
> {
>     fake f;
>     MyClass<fake> m(f);
> }
> ————————————————

We have both models down here.  One where the interface is a
template'd argument, as you've indicated, and the kind where it's
virtual.  I was hoping to avoid more and more templates, templates on
templates, and the like.  However, this is a case where you can do
either depending on the library.  I don't really feel strongly about
it in that regard and Gmock IIRC, does support both options.  My goal
was to be less disruptive where possible.  With templating, you need
to specify all the interfaces all the time, whereas with the injection
model you only need to specify them in the non-default case.  See
sdbuspluss::message::message, where by default, it works as before,
but you can make it use a mock interface, or even another library that
shares signatures.  To me, this was a win.  Imagine the following:

class Handler {
  public:
      arbitraryMethod(void *args) {
          otherThing(j);
      }
      receiveDataCallback(size_t len, void *p) {
          sendData(len, p);
      }
};

Such that sendData and otherThing are from different libraries, then
it'd end up being multiple templates all the time, everywhere.  Now,
conceivably if your object interacts with lots of different things it
might be a bad design, or it might be just the way it is.

> Granted this requires _all_ of our code to be templated, obfuscates what is
> really going on and is all just a bit silly,

Yes.  I was thinking, one template for everything isn't that bad, but it grows.

>  but from where I sit it isn’t
> any more obfuscated than wrapping every shared library function with an abstract
> interface class _and_ you don’t incur the overhead of doing that.  Am I overlooking
> anything?

One way to handle this would be to actually make the interface an
optional parameter in that, it's only used when set.

void doThing(size_t len, void *p) {
    if (intf) {
        return intf->sendData(len, p);
    }
    return sendData(len, p);
}

However, I think that's uglier than the other approach.  With simple
objects, a virtual lookup is trivial to the point it's basically
equivalent to a function pointer in a struct.

At Google, we're also seeing some serious CPU usage and wastes of
time, but a big culprit of this is that nothing caches anything ever.
If you want to talk to a dbus object, you get the service, then you
get it.  Every time.  So, basically anything talks to the mapper.  And
the mapper is just a python script getting hammered all the time.  We
lose a lot of time talking to it.  Hence, the recent efforts to start
caching sdbus addresses since they only change in rare cases.  I know
on the 2400, sometimes it's perfectly responsive and then it can get a
bit wedged and then frees up again with some regularity, so to me,
it's already slow.  And I don't think this'll make it slower as this
isn't the slowest path.  This might make the fastest path slightly
slower but with the trade off of having trust in the code.

>
> I’m only half serious with the proposal.  My real point is I feel like we are
> jumping through hoops to make a unit test framework work despite there being other
> ways of writing unit tests.
>
> Lei - thank you for weighing in.  If this was a poll it would be:
>
> 2 in favor of becoming a GMock project at the core.
> 1 against
>
> If no-one else speaks up, then I lose :-).  But it would be nice to improve the
> margin of error.
>
>
>>
>> Hopefully we can get a mockable sdbusplus as a shared library as well!
>>
>>> daemon (or providing a piece of one) to demonstrate how to leverage
>>> the mocks to test OpenBMC.  If one designs with testing in mind, the
>>> designs come out very differently if not, and so getting unit-tests
>>> throughout OpenBMC will be a lot of breaking things apart into
>>> testable pieces.  Anyways, where I'm going with this email is that
>>> everything we do within a daemon needs to be something that can be
>>> mocked -- basically.
>>>
>>> ***
>>> What do I mean specifically?  Consider, file access.  If a daemon
>>> routes all file accesses throug a common object implementation
>>> provided by a shared library, that shared library can easily also
>>> provide a mock interface for those accesses, such that one can easily
>>> verify behaviors based on file contents without implementing local
>>> files or trying to inject errors.  With a mock's file system
>>> interface, you can simply say that a file was unable to be read, or
>>> written, or opened, etc.  Another example is mocking ctime.  If you
>>> want to test whether something happens after some sleep or period, if
>>> your code can receive a mock version of that library, one can
>>> deliberately control the results of 'time' or 'difftime', etc.  I have
>>> to build these interfaces for some of our downstream daemons and
>>> likely for other parts of OpenBMC, and to avoid code duplication it'll
>>> help to have them in some library.
>>>
>>> YASL (yet-another-shared-library) Request.
>>>
>>> Previous conversations along these lines lead to the idea that we need
>>> multiple new libraries for various things.  So, this is yet another
>>> use case.  The library itself should be written in such a way that it
>>> can be tested via unit-tests, but depending on how thin of a shim it
>>> is, that isn't always practical.  See:
>>>
>>> class FileInterface {
>>>  public:
>>>     virtual int open(const char *filename, int flags) = 0;
>>> };
>>>
>>> class FileImplementation : public FileInterface {
>>>  public:
>>>    int open(const char *filename, int flags) override {
>>>        return ::open(filename, flags);
>>>    }
>>> };
>>>
>>> class FileMock : public FileInterface {
>>>   public:
>>>     MOCK_METHOD2(open, int(const char *, int));
>>> };
>>>
>>> .... then one just uses the FileInterface for their normally direct
>>> posix-style file access.  This can easily wrap iostream, or fstream,
>>> or anything.  And then if we provide some libraries for use by
>>> daemons, they can transition over to them over time, and then they get
>>> mocks for free :D  For a daemon downstream, I've written a ctime
>>> wrapper, I'll submit it for consideration later tonight along with a
>>> few other things, and then I'll reply to this email with links.
>>>
>>
>> So this library would contain several interfaces classes (e.g. FileInterface,
>> TimeInterface, and hopefully SdbusplusInterface etc) all together, right?
>> I vote for it!
>>
>>> ***Not meant as a unit-test primer, just trying to provide some real examples.
>>>
>>> Patrick