[libcamera-devel] [PATCH 4/5] test: media_device: Convert to foreach

[Laurent Pinchart]

Hi Kieran,

On Wednesday, 2 January 2019 15:52:31 EET Kieran Bingham wrote:
> On 02/01/2019 09:23, Laurent Pinchart wrote:
> > On Tuesday, 1 January 2019 23:29:46 EET Kieran Bingham wrote:
> >> Prevent duplication of boilerplate code as the suite grows by
> >> establishing the foreach pattern in the media_device test suite.
> > 
> > This results in duplicating the code for the foreach pattern though. I
> > wonder whether we could do better. How about something like
> 
> I appreciate there is duplication of the foreach pattern ... however
> this should be constant per 'test suite', and allows tests to be added
> with a single line as opposed to duplicating the 4/5 lines of
> executable()/test() specifications.
> 
> > diff --git a/test/media_device/meson.build b/test/media_device/meson.build
> > index a7ebed102e24..5f14197b005f 100644
> > --- a/test/media_device/meson.build
> > +++ b/test/media_device/meson.build
> > @@ -1,5 +1,3 @@
> > -media_device_test = executable('media_device_test',
> > 'media_device_test.cpp',
> > -                               link_with : test_libraries,
> > -                               include_directories :
> > test_includes_internal)
> > -
> > -test('Media Device Test', media_device_test)
> > +internal_tests += [
> > +    ['media_device_test',   files(['media_device_test.cpp'])],
> > +]
> 
> IMO this isn't better.
> 
> Either, the media_device_test is a single file test, in which case it
> can live in the test/ directory and be added directly to the
> internal_tests in test/meson.build, without the need for the
> test/media_device directory or the media_device will have multiple tests
> which will be part of a suite, and should have the
>   "suite: 'media_device'" parameter added to test().
> 
> Looking at the media_device_test.cpp I think it should already be split
> up into unit tests, as at the moment it just 'does a bunch of arbitrary
> stuff' - so while it exercises the code - it doesn't really 'unit-test'.
> 
> We could add this to the array, perhaps - but I expect 'suite specific'
> helpers would be added (please see my kbingham/v4l2 branch for how I
> have added extra tests to the suite there).
> 
> So either we have every parameter in the array, or we have suite
> specific implementations.
> 
> At the moment I prefer the suite specific ... what is your take?

We could do something like

internal_tests += [
    ['media_device_test', 'media_device_test', 
files(['media_device_test.cpp'])],
]

where arg[0] is the suite name, arg[1] the test name, and arg[2] the files(). 
This would allow having a single foreach loop in test/meson.build.

On the other hand it wouldn't work if we have to link with a suite-specific 
library.

I'd really prefer if all tests from a single suite were compiled as a single 
binary. This would avoid duplicating instances of helpers in all test 
binaries. That's something that could be fixed later though, we're still 
missing proper infrastructure for this. We should however not delay this 
development for too long otherwise we'll have too many "legacy" tests to port.

> > diff --git a/test/meson.build b/test/meson.build
> > index 184a7eeb5e27..69e4855013ec 100644
> > --- a/test/meson.build
> > +++ b/test/meson.build
> > @@ -1,7 +1,5 @@
> > 
> >  subdir('libtest')
> > 
> > -subdir('media_device')
> > -
> >  public_tests = [
> >      ['list',            'list.cpp'],
> >  ]
> > @@ -9,6 +7,8 @@ public_tests = [
> >  internal_tests = [
> >  ]
> > 
> > +subdir('media_device')
> > +
> >  foreach t : public_tests
> >      exe = executable(t[0], t[1],
> >                       link_with : test_libraries,
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> 
> >>  test/media_device/meson.build | 14 ++++++++++----
> >>  1 file changed, 10 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/test/media_device/meson.build
> >> b/test/media_device/meson.build
> >> index a7ebed102e24..d9394b0545d8 100644
> >> --- a/test/media_device/meson.build
> >> +++ b/test/media_device/meson.build
> >> @@ -1,5 +1,11 @@
> >> -media_device_test = executable('media_device_test',
> >> 'media_device_test.cpp',
> >> -                               link_with : test_libraries,
> >> -                               include_directories :
> >> test_includes_internal)
> >> +media_device_tests = [
> >> +    ['media_device_test',   'media_device_test.cpp'],
> >> +]
> >> 
> >> -test('Media Device Test', media_device_test)
> >> +foreach t : media_device_tests
> >> +    exe = executable(t[0], t[1],
> >> +                     link_with : test_libraries,
> >> +                     include_directories : test_includes_internal)
> >> +
> >> +    test(t[0], exe, suite: 'media_device', is_parallel: false)
> >> +endforeach

-- 
Regards,

Laurent Pinchart