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

[Kieran Bingham]

Hi Laurent,

On 02/01/2019 09:23, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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?


> 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
--
Kieran