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

[Alexandre Janniaux]

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, though the
design of the executor might not be able to test it without an intrusive
method.

We have such warning silencing in the vlc_tick_sleep cancellation tests,
but well, at some point you need to actually call it to test it. :D

Regards,
--
Alexandre Janniaux
Videolabs