| Message ID | 20190101212947.28098-5-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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 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'])], +] 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@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
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@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 >
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@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
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
Prevent duplication of boilerplate code as the suite grows by establishing the foreach pattern in the media_device test suite. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- test/media_device/meson.build | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)