[libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using Googletest

[Nícolas F. R. A. Prado]

Hi Niklas,

thanks for the feedback.

Em 2021-04-29 08:48, Niklas Söderlund escreveu:

> Hello Nícolas,
>
> Thanks for your work.
>
> On 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote:
> > Refactor lc-compliance using Googletest as the test framework.
>
> I really like the gtest rework from a first reading of this patch.
> Unfortunately it no longer applies to master so I have not been able to
> take it for a test drive.
>
> I like that we get most if not all features we discussed such as skip
> and test suite/case selection form the CLI more or less out-for-the box
> for free.
>
> Some random comments below.
>
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > ---
> >  src/lc-compliance/main.cpp           |  60 +++++------
> >  src/lc-compliance/meson.build        |   3 +
> >  src/lc-compliance/simple_capture.cpp |  73 +++++---------
> >  src/lc-compliance/simple_capture.h   |   8 +-
> >  src/lc-compliance/single_stream.cpp  | 142 ++++++++++++++-------------
> >  5 files changed, 132 insertions(+), 154 deletions(-)
> > 
> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > index 54cee54aa978..04cd0732ee3d 100644
> > --- a/src/lc-compliance/main.cpp
> > +++ b/src/lc-compliance/main.cpp
> > @@ -14,8 +14,12 @@
> >  #include "../cam/options.h"
> >  #include "tests.h"
> >  
> > +#include "gtest/gtest.h"
>
> As gtest is pulled in as an dependably and not added to the libcamera
> tee should this not be <gtest/gtest.h> ?

It should indeed. Fixed for v2.

>
> > +
> >  using namespace libcamera;
> >  
> > +std::shared_ptr<Camera> cam;
> > +
>
> Would it be possible to factor the code so that the Camera instance is
> not a global variable? I'm thinking that this will not scale when tests
> requiring more then one camera are needed.

So... This is something that annoyed me as well. The short answer is I think it
could, but the test registration code wouldn't be as clean.

The not so short answer: The only way I think this could be done is by adding a
parameter to every test case with the camera instance. But I'm currently
registering the test suites statically:

INSTANTIATE_TEST_SUITE_P(Raw,
                         FixedRequestsTest,
                         testing::Combine(testing::Values(Raw),
					testing::ValuesIn(NUMREQUESTS)));

So in order to pass the camera instance, they would need to be registered
dynamically, as the example in [1] shows, after the camera to use is parsed from
the command line.

[1] https://google.github.io/googletest/advanced.html#registering-tests-programmatically

But indeed, this seems to be needed to support working with multiple cameras.
The other solutions I was thinking about: making the global variable a vector of
cameras and/or running gtest multiple times and each time with the camera
variable set differently, wouldn't give a single report at the end with the
results from all cameras which would make it harder to parse the results.

I'm not sure which tests would need multiple cameras (if you think of examples
let me know), but at least being able to run each of the tests for each of the
cameras on a system in a single lc-compliance run would be important I think.

So I'll take a stab at it and see how things go.

Regarding the other issues, I'll have them fixed for v2.

Thanks,
Nícolas