Re: [PATCH v2 3/3] unit: dbus: Add test for disconnect watch API

[Jukka Rissanen <jukka.rissanen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2102 bytes --]

On pe, 2015-02-13 at 08:10 -0600, Denis Kenzior wrote:
> Hi Jukka,
> 
> On 02/13/2015 02:40 AM, Jukka Rissanen wrote:
> > On to, 2015-02-12 at 21:36 -0600, Denis Kenzior wrote:
> >> On 02/12/2015 05:48 AM, Jukka Rissanen wrote:
> >>> Test the dbus disconnect watch support.
> >>> ---
> >>>    Makefile.am            |   3 +
> >>>    unit/test-dbus-watch.c | 388 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>    2 files changed, 391 insertions(+)
> >>>    create mode 100644 unit/test-dbus-watch.c
> >>>
> >>
> >> That seems like an awful lot of copy-paste. Can we isolate the actual
> >> signal dispatcher and unit test it properly?  For example, you can
> >> always make the message_filter function private (e.g. without
> >> LIB_EXPORT) instead of static and feed it signals created using e.g.
> >> l_dbus_message_new_signal or dbus_message_from_blob
> >
> > I did not quite understand your comment about signal dispatcher and
> > message_filter here, please elaborate a bit more?
> >
> 
> Look at how unit/test-dbus-service.c is done.  Almost every part of the 
> dbus-service code can be tested standalone.  You need to think about how 
> to do the same.
> 
> For example:
> 	static void message_filter(...) -> void _dbus_message_filter(...)
> 	static void format_rule(...) -> void _dbus1_format_rule()
> 
> Or maybe even allow the entire handle_signal() path to be tested.
> 
> Then test this functionality in the unit test.  If you're relying on 
> semi-manual tests for this stuff, your test coverage will be pretty small.
> 

But don't we want to test this against dbus-daemon? Testing this watch
API without dbus-daemon interaction seems pointless to me.

> Regards,
> -Denis
> 
> >>
> >> Alternatively, fold this into test-dbus.c if possible.
> >
> > I thought about that but test-dbus.c was already quite big. But if you
> > are ok with that I can certainly do that too.
> >
> 
> This is the least preferred option.  Only do this for stuff that can't 
> be tested via the unit test.
> 
> Regards,
> -Denis
> 


Jukka