[vlc-devel] [PATCH 2/6] executor: add unit tests

[Romain Vimont]

On Tue, Sep 08, 2020 at 12:23:28PM +0200, Alexandre Janniaux wrote:
> Hi,
> 
> On Tue, Sep 08, 2020 at 12:11:50PM +0200, Romain Vimont wrote:
> > On Tue, Sep 08, 2020 at 12:06:23PM +0200, Alexandre Janniaux wrote:
> > > Hi,
> > >
> > > On Mon, Sep 07, 2020 at 07:16:07PM +0200, Romain Vimont wrote:
> > > > On Mon, Sep 07, 2020 at 06:27:37PM +0200, Alexandre Janniaux wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Sep 07, 2020 at 05:40:07PM +0200, Romain Vimont wrote:
> > > > > > +    /* Wait a bit (in two lines to avoid harmful_delay() warning) */
> > > > >
> > > > > To avoid harmful_delay() warnings, you can use (vlc_tick_sleep)(foo);
> > > >
> > > > Thank you for the tip.
> > > >
> > > > However, I'm not fan of this hack, I find it confusing and it "breaks"
> > > > tools like cscope/ctags.
> > > >
> > > > > > +    vlc_tick_t delay = VLC_TICK_FROM_MS(150);
> > > > > > +    vlc_tick_sleep(delay);
> > >
> > > Yes, but it could be argued that it's not much prettier
> > > to hide the warnings by splitting the sleeping calls.
> > > It's probably better to leave the warning triggers here
> > > then.
> >
> > The warning says "use proper event handling instead of short delay":
> >
> >     ../../src/test/executor.c: In function ‘test_cancel’:
> >     ../../include/vlc_threads.h:904:14: warning: call to ‘harmful_delay’ declared with attribute warning: use proper event handling instead of short delay [-Wattribute-warning]
> >       904 |            ? harmful_delay(d) \
> >           |              ^~~~~~~~~~~~~~~~
> >     ../../include/vlc_threads.h:923:42: note: in expansion of macro ‘check_delay’
> >       923 | #define vlc_tick_sleep(d) vlc_tick_sleep(check_delay(d))
> >           |                                          ^~~~~~~~~~~
> >     ../../src/test/executor.c:176:5: note: in expansion of macro ‘vlc_tick_sleep’
> >       176 |     vlc_tick_sleep(VLC_TICK_FROM_MS(150));
> >           |     ^~~~~~~~~~~~~~
> >
> > This is a good advice for real code, but not for unit tests (at least
> > not in this specific case).
> 
> Well, it does say “if you base your test on waiting, you can always have
> instances in which the test will fail to execute in due time“ so the
> warning still feels legitimate in the current situation

In that case, the sleep is just to make the test "more interesting", but
I took care that the test should never fail spuriously due to
(arbitrarily) bad timing. (unless I did mistakes and there are bugs in
the test code, of course)

Regards