[RFC,v1,12/12] apps: lc-compliance: Add multi-stream tests

22438 diff mbox series

Rename the `SingleStream` test to `SimpleCapture`, and
extend it to support using multiple roles. And instantiate
another test suite from the `SimpleCapture` test that
tests multiple streams in one capture session.

Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/apps/lc-compliance/tests/capture_test.cpp | 85 +++++++++++--------
 1 file changed, 48 insertions(+), 37 deletions(-)

Quoting Barnabás Pőcze (2024-12-20 15:08:58)
> Rename the `SingleStream` test to `SimpleCapture`, and
> extend it to support using multiple roles. And instantiate
> another test suite from the `SimpleCapture` test that
> tests multiple streams in one capture session.
> 
> Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/apps/lc-compliance/tests/capture_test.cpp | 85 +++++++++++--------
>  1 file changed, 48 insertions(+), 37 deletions(-)
> 
> diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> index c382fcf47..8ea422f0d 100644
> --- a/src/apps/lc-compliance/tests/capture_test.cpp
> +++ b/src/apps/lc-compliance/tests/capture_test.cpp
> @@ -8,7 +8,7 @@
>  
>  #include "capture.h"
>  
> -#include <iostream>
> +#include <sstream>
>  
>  #include <gtest/gtest.h>
>  
> @@ -18,19 +18,10 @@ namespace {
>  
>  using namespace libcamera;
>  
> -const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
> -
> -const StreamRole ROLES[] = {
> -       StreamRole::Raw,
> -       StreamRole::StillCapture,
> -       StreamRole::VideoRecording,
> -       StreamRole::Viewfinder
> -};
> -
> -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> +class SimpleCapture : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>>
>  {
>  public:
> -       static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> +       static std::string nameParameters(const testing::TestParamInfo<SimpleCapture::ParamType> &info);
>  
>  protected:
>         void SetUp() override;
> @@ -43,7 +34,7 @@ protected:
>   * We use gtest's SetUp() and TearDown() instead of constructor and destructor
>   * in order to be able to assert on them.
>   */
> -void SingleStream::SetUp()
> +void SimpleCapture::SetUp()
>  {
>         Environment *env = Environment::get();
>  
> @@ -52,7 +43,7 @@ void SingleStream::SetUp()
>         ASSERT_EQ(camera_->acquire(), 0);
>  }
>  
> -void SingleStream::TearDown()
> +void SimpleCapture::TearDown()
>  {
>         if (!camera_)
>                 return;
> @@ -61,19 +52,17 @@ void SingleStream::TearDown()
>         camera_.reset();
>  }
>  
> -std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info)
> +std::string SimpleCapture::nameParameters(const testing::TestParamInfo<SimpleCapture::ParamType> &info)
>  {
> -       std::map<StreamRole, std::string> rolesMap = {
> -               { StreamRole::Raw, "Raw" },
> -               { StreamRole::StillCapture, "StillCapture" },
> -               { StreamRole::VideoRecording, "VideoRecording" },
> -               { StreamRole::Viewfinder, "Viewfinder" }
> -       };
> +       const auto &[roles, numRequests] = info.param;
> +       std::ostringstream ss;
>  
> -       std::string roleName = rolesMap[std::get<0>(info.param)];
> -       std::string numRequestsName = std::to_string(std::get<1>(info.param));
> +       for (StreamRole r : roles)
> +               ss << r << '_';
>  
> -       return roleName + "_" + numRequestsName;
> +       ss << '_' << numRequests;
> +
> +       return std::move(ss).str();
>  }
>  
>  /*
> @@ -83,13 +72,13 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre
>   * failure is a camera that completes less requests than the number of requests
>   * queued.
>   */
> -TEST_P(SingleStream, Capture)
> +TEST_P(SimpleCapture, Capture)
>  {
> -       auto [role, numRequests] = GetParam();
> +       const auto &[roles, numRequests] = GetParam();
>  
>         CaptureBalanced capture(camera_);
>  
> -       capture.configure(std::array{ role });
> +       capture.configure(roles);
>  
>         capture.capture(numRequests);
>  }
> @@ -101,14 +90,14 @@ TEST_P(SingleStream, Capture)
>   * a camera that does not clean up correctly in its error path but is only
>   * tested by single-capture applications.
>   */
> -TEST_P(SingleStream, CaptureStartStop)
> +TEST_P(SimpleCapture, CaptureStartStop)
>  {
> -       auto [role, numRequests] = GetParam();
> +       const auto &[roles, numRequests] = GetParam();
>         unsigned int numRepeats = 3;
>  
>         CaptureBalanced capture(camera_);
>  
> -       capture.configure(std::array{ role });
> +       capture.configure(roles);
>  
>         for (unsigned int starts = 0; starts < numRepeats; starts++)
>                 capture.capture(numRequests);
> @@ -121,21 +110,43 @@ TEST_P(SingleStream, CaptureStartStop)
>   * is a camera that does not handle cancelation of buffers coming back from the
>   * video device while stopping.
>   */
> -TEST_P(SingleStream, UnbalancedStop)
> +TEST_P(SimpleCapture, UnbalancedStop)
>  {
> -       auto [role, numRequests] = GetParam();
> +       const auto &[roles, numRequests] = GetParam();
>  
>         CaptureUnbalanced capture(camera_);
>  
> -       capture.configure(std::array{ role });
> +       capture.configure(roles);
>  
>         capture.capture(numRequests);
>  }
>  
> -INSTANTIATE_TEST_SUITE_P(CaptureTests,
> -                        SingleStream,
> -                        testing::Combine(testing::ValuesIn(ROLES),
> +const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };

Not from your series, but I'm not sure I understand the full relevance
of why we use this sequence of 10 tests for each type. Do we really need
to run each test 10 times for each of these combinations? Running
lc-compliance seems to take a long time waiting for each of these
combinations and I can't see what value that really adds yet.

I could understand 1, 2, 3, 5 as variants that might or might not stream
based on minimum buffer requirements, but from there - I can't see what
difference we get from 8, 13, 21, 34, 55, 89. I'd be tempted to chop
this down to just something like { 1, 2, 3, 5, 8, 30, 90 }; or somehow
more aligned to what we actually want to test.

Especially when we get to the multi-stream role combinations, where we
might want to do different combinations such as a sequences that do not
have a buffer in one of the streams.

But aside from that which is more of a higher level discussion, I like
this patch simplifying things and handling / using testing::Combine to
create / manage the test suites.



Actions from the any discussions on the above could easily be on top.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> +
> +const std::vector<StreamRole> SINGLEROLES[] = {
> +       { StreamRole::Raw, },
> +       { StreamRole::StillCapture, },
> +       { StreamRole::VideoRecording, },
> +       { StreamRole::Viewfinder, },
> +};
> +
> +const std::vector<StreamRole> MULTIROLES[] = {
> +       { StreamRole::Raw, StreamRole::StillCapture },
> +       { StreamRole::Raw, StreamRole::VideoRecording },
> +       { StreamRole::StillCapture, StreamRole::VideoRecording },
> +       { StreamRole::VideoRecording, StreamRole::VideoRecording },
> +};
> +
> +INSTANTIATE_TEST_SUITE_P(SingleStream,
> +                        SimpleCapture,
> +                        testing::Combine(testing::ValuesIn(SINGLEROLES),
> +                                         testing::ValuesIn(NUMREQUESTS)),
> +                        SimpleCapture::nameParameters);
> +
> +INSTANTIATE_TEST_SUITE_P(MultiStream,
> +                        SimpleCapture,
> +                        testing::Combine(testing::ValuesIn(MULTIROLES),
>                                           testing::ValuesIn(NUMREQUESTS)),
> -                        SingleStream::nameParameters);
> +                        SimpleCapture::nameParameters);
>  
>  } /* namespace */
> -- 
> 2.47.1
> 
>

Hi


2024. december 24., kedd 17:42 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Barnabás Pőcze (2024-12-20 15:08:58)
> > Rename the `SingleStream` test to `SimpleCapture`, and
> > extend it to support using multiple roles. And instantiate
> > another test suite from the `SimpleCapture` test that
> > tests multiple streams in one capture session.
> >
> > Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  src/apps/lc-compliance/tests/capture_test.cpp | 85 +++++++++++--------
> >  1 file changed, 48 insertions(+), 37 deletions(-)
> >
> > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> > index c382fcf47..8ea422f0d 100644
> > --- a/src/apps/lc-compliance/tests/capture_test.cpp
> > +++ b/src/apps/lc-compliance/tests/capture_test.cpp
> > @@ -8,7 +8,7 @@
> >
> >  #include "capture.h"
> >
> > -#include <iostream>
> > +#include <sstream>
> >
> >  #include <gtest/gtest.h>
> >
> > @@ -18,19 +18,10 @@ namespace {
> >
> >  using namespace libcamera;
> >
> > -const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
> > -
> > -const StreamRole ROLES[] = {
> > -       StreamRole::Raw,
> > -       StreamRole::StillCapture,
> > -       StreamRole::VideoRecording,
> > -       StreamRole::Viewfinder
> > -};
> > -
> > -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> > +class SimpleCapture : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>>
> >  {
> >  public:
> > -       static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> > +       static std::string nameParameters(const testing::TestParamInfo<SimpleCapture::ParamType> &info);
> >
> >  protected:
> >         void SetUp() override;
> > @@ -43,7 +34,7 @@ protected:
> >   * We use gtest's SetUp() and TearDown() instead of constructor and destructor
> >   * in order to be able to assert on them.
> >   */
> > -void SingleStream::SetUp()
> > +void SimpleCapture::SetUp()
> >  {
> >         Environment *env = Environment::get();
> >
> > @@ -52,7 +43,7 @@ void SingleStream::SetUp()
> >         ASSERT_EQ(camera_->acquire(), 0);
> >  }
> >
> > -void SingleStream::TearDown()
> > +void SimpleCapture::TearDown()
> >  {
> >         if (!camera_)
> >                 return;
> > @@ -61,19 +52,17 @@ void SingleStream::TearDown()
> >         camera_.reset();
> >  }
> >
> > -std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info)
> > +std::string SimpleCapture::nameParameters(const testing::TestParamInfo<SimpleCapture::ParamType> &info)
> >  {
> > -       std::map<StreamRole, std::string> rolesMap = {
> > -               { StreamRole::Raw, "Raw" },
> > -               { StreamRole::StillCapture, "StillCapture" },
> > -               { StreamRole::VideoRecording, "VideoRecording" },
> > -               { StreamRole::Viewfinder, "Viewfinder" }
> > -       };
> > +       const auto &[roles, numRequests] = info.param;
> > +       std::ostringstream ss;
> >
> > -       std::string roleName = rolesMap[std::get<0>(info.param)];
> > -       std::string numRequestsName = std::to_string(std::get<1>(info.param));
> > +       for (StreamRole r : roles)
> > +               ss << r << '_';
> >
> > -       return roleName + "_" + numRequestsName;
> > +       ss << '_' << numRequests;
> > +
> > +       return std::move(ss).str();
> >  }
> >
> >  /*
> > @@ -83,13 +72,13 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre
> >   * failure is a camera that completes less requests than the number of requests
> >   * queued.
> >   */
> > -TEST_P(SingleStream, Capture)
> > +TEST_P(SimpleCapture, Capture)
> >  {
> > -       auto [role, numRequests] = GetParam();
> > +       const auto &[roles, numRequests] = GetParam();
> >
> >         CaptureBalanced capture(camera_);
> >
> > -       capture.configure(std::array{ role });
> > +       capture.configure(roles);
> >
> >         capture.capture(numRequests);
> >  }
> > @@ -101,14 +90,14 @@ TEST_P(SingleStream, Capture)
> >   * a camera that does not clean up correctly in its error path but is only
> >   * tested by single-capture applications.
> >   */
> > -TEST_P(SingleStream, CaptureStartStop)
> > +TEST_P(SimpleCapture, CaptureStartStop)
> >  {
> > -       auto [role, numRequests] = GetParam();
> > +       const auto &[roles, numRequests] = GetParam();
> >         unsigned int numRepeats = 3;
> >
> >         CaptureBalanced capture(camera_);
> >
> > -       capture.configure(std::array{ role });
> > +       capture.configure(roles);
> >
> >         for (unsigned int starts = 0; starts < numRepeats; starts++)
> >                 capture.capture(numRequests);
> > @@ -121,21 +110,43 @@ TEST_P(SingleStream, CaptureStartStop)
> >   * is a camera that does not handle cancelation of buffers coming back from the
> >   * video device while stopping.
> >   */
> > -TEST_P(SingleStream, UnbalancedStop)
> > +TEST_P(SimpleCapture, UnbalancedStop)
> >  {
> > -       auto [role, numRequests] = GetParam();
> > +       const auto &[roles, numRequests] = GetParam();
> >
> >         CaptureUnbalanced capture(camera_);
> >
> > -       capture.configure(std::array{ role });
> > +       capture.configure(roles);
> >
> >         capture.capture(numRequests);
> >  }
> >
> > -INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > -                        SingleStream,
> > -                        testing::Combine(testing::ValuesIn(ROLES),
> > +const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
> 
> Not from your series, but I'm not sure I understand the full relevance
> of why we use this sequence of 10 tests for each type. Do we really need
> to run each test 10 times for each of these combinations? Running
> lc-compliance seems to take a long time waiting for each of these
> combinations and I can't see what value that really adds yet.

You're right. It takes a bit of time to run, this can be a problem. To be honest
I just took these numbers from the single stream case. I believe the numbers
follow the Fibonacci sequence, so I am not sure if there is any more intent behind
them. I couldn't find anything in the git history in any case.


Regards,
Barnabás Pőcze


> 
> I could understand 1, 2, 3, 5 as variants that might or might not stream
> based on minimum buffer requirements, but from there - I can't see what
> difference we get from 8, 13, 21, 34, 55, 89. I'd be tempted to chop
> this down to just something like { 1, 2, 3, 5, 8, 30, 90 }; or somehow
> more aligned to what we actually want to test.
> 
> Especially when we get to the multi-stream role combinations, where we
> might want to do different combinations such as a sequences that do not
> have a buffer in one of the streams.
> 
> But aside from that which is more of a higher level discussion, I like
> this patch simplifying things and handling / using testing::Combine to
> create / manage the test suites.
> 
> 
> 
> Actions from the any discussions on the above could easily be on top.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > +
> > +const std::vector<StreamRole> SINGLEROLES[] = {
> > +       { StreamRole::Raw, },
> > +       { StreamRole::StillCapture, },
> > +       { StreamRole::VideoRecording, },
> > +       { StreamRole::Viewfinder, },
> > +};
> > +
> > +const std::vector<StreamRole> MULTIROLES[] = {
> > +       { StreamRole::Raw, StreamRole::StillCapture },
> > +       { StreamRole::Raw, StreamRole::VideoRecording },
> > +       { StreamRole::StillCapture, StreamRole::VideoRecording },
> > +       { StreamRole::VideoRecording, StreamRole::VideoRecording },
> > +};
> > +
> > +INSTANTIATE_TEST_SUITE_P(SingleStream,
> > +                        SimpleCapture,
> > +                        testing::Combine(testing::ValuesIn(SINGLEROLES),
> > +                                         testing::ValuesIn(NUMREQUESTS)),
> > +                        SimpleCapture::nameParameters);
> > +
> > +INSTANTIATE_TEST_SUITE_P(MultiStream,
> > +                        SimpleCapture,
> > +                        testing::Combine(testing::ValuesIn(MULTIROLES),
> >                                           testing::ValuesIn(NUMREQUESTS)),
> > -                        SingleStream::nameParameters);
> > +                        SimpleCapture::nameParameters);
> >
> >  } /* namespace */
> > --
> > 2.47.1
> >
> >
>

Hi Barnabás,

Thank you for the patch.

On Tue, Jan 07, 2025 at 08:43:13AM +0000, Barnabás Pőcze wrote:
> 2024. december 24., kedd 17:42 keltezéssel, Kieran Bingham írta:
> > Quoting Barnabás Pőcze (2024-12-20 15:08:58)
> > > Rename the `SingleStream` test to `SimpleCapture`, and
> > > extend it to support using multiple roles. And instantiate
> > > another test suite from the `SimpleCapture` test that
> > > tests multiple streams in one capture session.

Sounds like this could have been split in two patches.

> > > Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > ---
> > >  src/apps/lc-compliance/tests/capture_test.cpp | 85 +++++++++++--------
> > >  1 file changed, 48 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> > > index c382fcf47..8ea422f0d 100644
> > > --- a/src/apps/lc-compliance/tests/capture_test.cpp
> > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp
> > > @@ -8,7 +8,7 @@
> > >
> > >  #include "capture.h"
> > >
> > > -#include <iostream>
> > > +#include <sstream>
> > >
> > >  #include <gtest/gtest.h>
> > >
> > > @@ -18,19 +18,10 @@ namespace {
> > >
> > >  using namespace libcamera;
> > >
> > > -const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
> > > -
> > > -const StreamRole ROLES[] = {
> > > -       StreamRole::Raw,
> > > -       StreamRole::StillCapture,
> > > -       StreamRole::VideoRecording,
> > > -       StreamRole::Viewfinder
> > > -};
> > > -
> > > -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> > > +class SimpleCapture : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>>
> > >  {
> > >  public:
> > > -       static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> > > +       static std::string nameParameters(const testing::TestParamInfo<SimpleCapture::ParamType> &info);
> > >
> > >  protected:
> > >         void SetUp() override;
> > > @@ -43,7 +34,7 @@ protected:
> > >   * We use gtest's SetUp() and TearDown() instead of constructor and destructor
> > >   * in order to be able to assert on them.
> > >   */
> > > -void SingleStream::SetUp()
> > > +void SimpleCapture::SetUp()
> > >  {
> > >         Environment *env = Environment::get();
> > >
> > > @@ -52,7 +43,7 @@ void SingleStream::SetUp()
> > >         ASSERT_EQ(camera_->acquire(), 0);
> > >  }
> > >
> > > -void SingleStream::TearDown()
> > > +void SimpleCapture::TearDown()
> > >  {
> > >         if (!camera_)
> > >                 return;
> > > @@ -61,19 +52,17 @@ void SingleStream::TearDown()
> > >         camera_.reset();
> > >  }
> > >
> > > -std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info)
> > > +std::string SimpleCapture::nameParameters(const testing::TestParamInfo<SimpleCapture::ParamType> &info)
> > >  {
> > > -       std::map<StreamRole, std::string> rolesMap = {
> > > -               { StreamRole::Raw, "Raw" },
> > > -               { StreamRole::StillCapture, "StillCapture" },
> > > -               { StreamRole::VideoRecording, "VideoRecording" },
> > > -               { StreamRole::Viewfinder, "Viewfinder" }
> > > -       };
> > > +       const auto &[roles, numRequests] = info.param;
> > > +       std::ostringstream ss;
> > >
> > > -       std::string roleName = rolesMap[std::get<0>(info.param)];
> > > -       std::string numRequestsName = std::to_string(std::get<1>(info.param));
> > > +       for (StreamRole r : roles)
> > > +               ss << r << '_';
> > >
> > > -       return roleName + "_" + numRequestsName;
> > > +       ss << '_' << numRequests;
> > > +
> > > +       return std::move(ss).str();
> > >  }
> > >
> > >  /*
> > > @@ -83,13 +72,13 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre
> > >   * failure is a camera that completes less requests than the number of requests
> > >   * queued.
> > >   */
> > > -TEST_P(SingleStream, Capture)
> > > +TEST_P(SimpleCapture, Capture)
> > >  {
> > > -       auto [role, numRequests] = GetParam();
> > > +       const auto &[roles, numRequests] = GetParam();
> > >
> > >         CaptureBalanced capture(camera_);
> > >
> > > -       capture.configure(std::array{ role });
> > > +       capture.configure(roles);
> > >
> > >         capture.capture(numRequests);
> > >  }
> > > @@ -101,14 +90,14 @@ TEST_P(SingleStream, Capture)
> > >   * a camera that does not clean up correctly in its error path but is only
> > >   * tested by single-capture applications.
> > >   */
> > > -TEST_P(SingleStream, CaptureStartStop)
> > > +TEST_P(SimpleCapture, CaptureStartStop)
> > >  {
> > > -       auto [role, numRequests] = GetParam();
> > > +       const auto &[roles, numRequests] = GetParam();
> > >         unsigned int numRepeats = 3;
> > >
> > >         CaptureBalanced capture(camera_);
> > >
> > > -       capture.configure(std::array{ role });
> > > +       capture.configure(roles);
> > >
> > >         for (unsigned int starts = 0; starts < numRepeats; starts++)
> > >                 capture.capture(numRequests);
> > > @@ -121,21 +110,43 @@ TEST_P(SingleStream, CaptureStartStop)
> > >   * is a camera that does not handle cancelation of buffers coming back from the
> > >   * video device while stopping.
> > >   */
> > > -TEST_P(SingleStream, UnbalancedStop)
> > > +TEST_P(SimpleCapture, UnbalancedStop)
> > >  {
> > > -       auto [role, numRequests] = GetParam();
> > > +       const auto &[roles, numRequests] = GetParam();
> > >
> > >         CaptureUnbalanced capture(camera_);
> > >
> > > -       capture.configure(std::array{ role });
> > > +       capture.configure(roles);
> > >
> > >         capture.capture(numRequests);
> > >  }
> > >
> > > -INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > > -                        SingleStream,
> > > -                        testing::Combine(testing::ValuesIn(ROLES),
> > > +const int NUMREQUESTS[] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };

While at it, s/NUMREQUESTS/kNumRequests/ to match our coding style ?
Same below.

> > 
> > Not from your series, but I'm not sure I understand the full relevance
> > of why we use this sequence of 10 tests for each type. Do we really need
> > to run each test 10 times for each of these combinations? Running
> > lc-compliance seems to take a long time waiting for each of these
> > combinations and I can't see what value that really adds yet.
> 
> You're right. It takes a bit of time to run, this can be a problem. To be honest
> I just took these numbers from the single stream case. I believe the numbers
> follow the Fibonacci sequence, so I am not sure if there is any more intent behind
> them. I couldn't find anything in the git history in any case.
> 
> > I could understand 1, 2, 3, 5 as variants that might or might not stream
> > based on minimum buffer requirements, but from there - I can't see what
> > difference we get from 8, 13, 21, 34, 55, 89. I'd be tempted to chop
> > this down to just something like { 1, 2, 3, 5, 8, 30, 90 }; or somehow
> > more aligned to what we actually want to test.
> > 
> > Especially when we get to the multi-stream role combinations, where we
> > might want to do different combinations such as a sequences that do not
> > have a buffer in one of the streams.
> > 
> > But aside from that which is more of a higher level discussion, I like
> > this patch simplifying things and handling / using testing::Combine to
> > create / manage the test suites.
> > 
> > Actions from the any discussions on the above could easily be on top.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Agreed.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > +
> > > +const std::vector<StreamRole> SINGLEROLES[] = {
> > > +       { StreamRole::Raw, },
> > > +       { StreamRole::StillCapture, },
> > > +       { StreamRole::VideoRecording, },
> > > +       { StreamRole::Viewfinder, },
> > > +};
> > > +
> > > +const std::vector<StreamRole> MULTIROLES[] = {
> > > +       { StreamRole::Raw, StreamRole::StillCapture },
> > > +       { StreamRole::Raw, StreamRole::VideoRecording },
> > > +       { StreamRole::StillCapture, StreamRole::VideoRecording },
> > > +       { StreamRole::VideoRecording, StreamRole::VideoRecording },
> > > +};
> > > +
> > > +INSTANTIATE_TEST_SUITE_P(SingleStream,
> > > +                        SimpleCapture,
> > > +                        testing::Combine(testing::ValuesIn(SINGLEROLES),
> > > +                                         testing::ValuesIn(NUMREQUESTS)),
> > > +                        SimpleCapture::nameParameters);
> > > +
> > > +INSTANTIATE_TEST_SUITE_P(MultiStream,
> > > +                        SimpleCapture,
> > > +                        testing::Combine(testing::ValuesIn(MULTIROLES),
> > >                                           testing::ValuesIn(NUMREQUESTS)),
> > > -                        SingleStream::nameParameters);
> > > +                        SimpleCapture::nameParameters);
> > >
> > >  } /* namespace */