[{"id":17618,"web_url":"https://patchwork.libcamera.org/comment/17618/","msgid":"<20210617215604.nhyqkoraptxmekdx@uno.localdomain>","date":"2021-06-17T21:56:04","subject":"Re: [libcamera-devel] [PATCH v8 4/5] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas,\n\nOn Wed, Jun 16, 2021 at 10:25:34AM -0300, Nícolas F. R. A. Prado wrote:\n> Refactor lc-compliance using Googletest as the test framework.\n>\n> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n> Changes in v8:\n> - Thanks to Laurent:\n>   - Fixed lc-compliance's meson.build to disable when gtest is not installed\n>   - Fixed compiling error in Clang\n>\n> Changes in v7:\n> - Thanks to Jacopo:\n>   - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n>\n> No changes in v6\n>\n> Changes in v5:\n> - Thanks to Laurent:\n>   - Fixed style issues\n>   - Added SetUp and TearDown functions to acquire and release the camera for\n>     each test\n>\n> Changes in v4:\n> - Removed old lc-compliance results classes\n> - Thanks to Niklas:\n>   - Improved naming of tests\n>\n> Changes in v3:\n> - Thanks to Niklas:\n>   - Went back to static test registration, and created a Singleton Environment\n>     class to store the camera global variable\n> - Added a nameParameters() function to give meaningful names to the test\n>   parameters, removing the need to register each test suite for every role\n>\n>  src/lc-compliance/main.cpp           |  73 ++++++------\n>  src/lc-compliance/meson.build        |   5 +-\n>  src/lc-compliance/results.cpp        |  75 ------------\n>  src/lc-compliance/results.h          |  47 --------\n>  src/lc-compliance/simple_capture.cpp |  74 +++++-------\n>  src/lc-compliance/simple_capture.h   |   9 +-\n>  src/lc-compliance/single_stream.cpp  | 172 ++++++++++++++++-----------\n>  src/lc-compliance/tests.h            |  16 ---\n>  8 files changed, 174 insertions(+), 297 deletions(-)\n>  delete mode 100644 src/lc-compliance/results.cpp\n>  delete mode 100644 src/lc-compliance/results.h\n>  delete mode 100644 src/lc-compliance/tests.h\n>\n> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> index 54cee54aa978..8c18845141de 100644\n> --- a/src/lc-compliance/main.cpp\n> +++ b/src/lc-compliance/main.cpp\n> @@ -9,10 +9,12 @@\n>  #include <iostream>\n>  #include <string.h>\n>\n> +#include <gtest/gtest.h>\n> +\n>  #include <libcamera/libcamera.h>\n>\n> +#include \"environment.h\"\n>  #include \"../cam/options.h\"\n> -#include \"tests.h\"\n>\n>  using namespace libcamera;\n>\n> @@ -21,21 +23,35 @@ enum {\n>  \tOptHelp = 'h',\n>  };\n>\n> +/*\n> + * Make asserts act like exceptions, otherwise they only fail (or skip) the\n> + * current function. From gtest documentation:\n> + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception\n> + */\n> +class ThrowListener : public testing::EmptyTestEventListener\n> +{\n> +\tvoid OnTestPartResult(const testing::TestPartResult &result) override\n> +\t{\n> +\t\tif (result.type() == testing::TestPartResult::kFatalFailure ||\n> +\t\t    result.type() == testing::TestPartResult::kSkip)\n> +\t\t\tthrow testing::AssertionException(result);\n> +\t}\n> +};\n> +\n>  class Harness\n\nWhat would you think of dropping this class ? It's only used in\nmain.cpp and it basically just wraps the CameraManager.\n\nI would keep the cm local, initalize the Environment initialize gtest\nand call RUN_ALL_TESTS(). listCameras() can be made a static helper\nfunction. Maybe I'm missing some perspective, but once we're on gtest\nnothing more should be add to main.cpp\n\n>  {\n>  public:\n>  \tHarness(const OptionsParser::Options &options);\n>  \t~Harness();\n>\n> -\tint exec();\n> +\tint init();\n> +\tint run(int argc, char **argv);\n>\n>  private:\n> -\tint init();\n>  \tvoid listCameras();\n>\n>  \tOptionsParser::Options options_;\n>  \tstd::unique_ptr<CameraManager> cm_;\n> -\tstd::shared_ptr<Camera> camera_;\n>  };\n>\n>  Harness::Harness(const OptionsParser::Options &options)\n> @@ -46,35 +62,13 @@ Harness::Harness(const OptionsParser::Options &options)\n>\n>  Harness::~Harness()\n>  {\n> -\tif (camera_) {\n> -\t\tcamera_->release();\n> -\t\tcamera_.reset();\n> -\t}\n> -\n>  \tcm_->stop();\n>  }\n>\n> -int Harness::exec()\n> -{\n> -\tint ret = init();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tstd::vector<Results> results;\n> -\n> -\tresults.push_back(testSingleStream(camera_));\n> -\n> -\tfor (const Results &result : results) {\n> -\t\tret = result.summary();\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n>  int Harness::init()\n>  {\n> +\tstd::shared_ptr<Camera> camera;\n> +\n>  \tint ret = cm_->start();\n>  \tif (ret) {\n>  \t\tstd::cout << \"Failed to start camera manager: \"\n> @@ -89,23 +83,29 @@ int Harness::init()\n>  \t}\n>\n>  \tconst std::string &cameraId = options_[OptCamera];\n> -\tcamera_ = cm_->get(cameraId);\n> -\tif (!camera_) {\n> +\tcamera = cm_->get(cameraId);\n> +\tif (!camera) {\n>  \t\tstd::cout << \"Camera \" << cameraId << \" not found, available cameras:\" << std::endl;\n>  \t\tlistCameras();\n>  \t\treturn -ENODEV;\n>  \t}\n>\n> -\tif (camera_->acquire()) {\n> -\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tEnvironment::get()->setup(cm_.get(), cameraId);\n>\n>  \tstd::cout << \"Using camera \" << cameraId << std::endl;\n>\n>  \treturn 0;\n>  }\n>\n> +int Harness::run(int argc, char **argv)\n> +{\n> +\t::testing::InitGoogleTest(&argc, argv);\n> +\n> +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> +\n> +\treturn RUN_ALL_TESTS();\n> +}\n> +\n>  void Harness::listCameras()\n>  {\n>  \tfor (const std::shared_ptr<Camera> &cam : cm_->cameras())\n> @@ -143,6 +143,9 @@ int main(int argc, char **argv)\n>  \t\treturn EXIT_FAILURE;\n>\n>  \tHarness harness(options);\n> +\tret = harness.init();\n> +\tif (ret)\n> +\t\treturn ret;\n>\n> -\treturn harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;\n> +\treturn harness.run(argc, argv);\n>  }\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index 6dd49085569f..097804c7aeda 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -1,8 +1,9 @@\n>  # SPDX-License-Identifier: CC0-1.0\n>\n>  libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))\n> +libgtest = dependency('gtest', required : get_option('lc-compliance'))\n>\n> -if not libevent.found()\n> +if not (libevent.found() and libgtest.found())\n>      lc_compliance_enabled = false\n>      subdir_done()\n>  endif\n> @@ -14,7 +15,6 @@ lc_compliance_sources = files([\n>      '../cam/options.cpp',\n>      'environment.cpp',\n>      'main.cpp',\n> -    'results.cpp',\n>      'simple_capture.cpp',\n>      'single_stream.cpp',\n>  ])\n> @@ -24,5 +24,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,\n>                                  libatomic,\n>                                  libcamera_dep,\n>                                  libevent,\n> +                                libgtest,\n>                              ],\n>                              install : true)\n> diff --git a/src/lc-compliance/results.cpp b/src/lc-compliance/results.cpp\n> deleted file mode 100644\n> index f149f7859e28..000000000000\n> --- a/src/lc-compliance/results.cpp\n> +++ /dev/null\n> @@ -1,75 +0,0 @@\n> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * results.cpp - Test result aggregator\n> - */\n> -\n> -#include \"results.h\"\n> -\n> -#include <iostream>\n> -\n> -void Results::add(const Result &result)\n> -{\n> -\tif (result.first == Pass)\n> -\t\tpassed_++;\n> -\telse if (result.first == Fail)\n> -\t\tfailed_++;\n> -\telse if (result.first == Skip)\n> -\t\tskipped_++;\n> -\n> -\tprintResult(result);\n> -}\n> -\n> -void Results::add(Status status, const std::string &message)\n> -{\n> -\tadd({ status, message });\n> -}\n> -\n> -void Results::fail(const std::string &message)\n> -{\n> -\tadd(Fail, message);\n> -}\n> -\n> -void Results::pass(const std::string &message)\n> -{\n> -\tadd(Pass, message);\n> -}\n> -\n> -void Results::skip(const std::string &message)\n> -{\n> -\tadd(Skip, message);\n> -}\n> -\n> -int Results::summary() const\n> -{\n> -\tif (failed_ + passed_ + skipped_ != planned_) {\n> -\t\tstd::cout << \"Planned and executed number of tests differ \"\n> -\t\t\t  << failed_ + passed_ + skipped_ << \" executed \"\n> -\t\t\t  << planned_ << \" planned\" << std::endl;\n> -\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tstd::cout << planned_ << \" tests executed, \"\n> -\t\t  << passed_ << \" tests passed, \"\n> -\t\t  << skipped_ << \" tests skipped and \"\n> -\t\t  << failed_ << \" tests failed \" << std::endl;\n> -\n> -\treturn 0;\n> -}\n> -\n> -void Results::printResult(const Result &result)\n> -{\n> -\tstd::string prefix;\n> -\n> -\t/* \\todo Make parsable as TAP. */\n> -\tif (result.first == Pass)\n> -\t\tprefix = \"PASS\";\n> -\telse if (result.first == Fail)\n> -\t\tprefix = \"FAIL\";\n> -\telse if (result.first == Skip)\n> -\t\tprefix = \"SKIP\";\n> -\n> -\tstd::cout << \"- \" << prefix << \": \" << result.second << std::endl;\n> -}\n> diff --git a/src/lc-compliance/results.h b/src/lc-compliance/results.h\n> deleted file mode 100644\n> index 2a3722b841a6..000000000000\n> --- a/src/lc-compliance/results.h\n> +++ /dev/null\n> @@ -1,47 +0,0 @@\n> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * results.h - Test result aggregator\n> - */\n> -#ifndef __LC_COMPLIANCE_RESULTS_H__\n> -#define __LC_COMPLIANCE_RESULTS_H__\n> -\n> -#include <string>\n> -#include <utility>\n> -\n> -/* \\todo Check if result aggregator can be shared with self tests in test/ */\n> -class Results\n> -{\n> -public:\n> -\tenum Status {\n> -\t\tFail,\n> -\t\tPass,\n> -\t\tSkip,\n> -\t};\n> -\n> -\tusing Result = std::pair<Status, std::string>;\n> -\n> -\tResults(unsigned int planned)\n> -\t\t: planned_(planned), passed_(0), failed_(0), skipped_(0)\n> -\t{\n> -\t}\n> -\n> -\tvoid add(const Result &result);\n> -\tvoid add(Status status, const std::string &message);\n> -\tvoid fail(const std::string &message);\n> -\tvoid pass(const std::string &message);\n> -\tvoid skip(const std::string &message);\n> -\n> -\tint summary() const;\n> -\n> -private:\n> -\tvoid printResult(const Result &result);\n> -\n> -\tunsigned int planned_;\n> -\tunsigned int passed_;\n> -\tunsigned int failed_;\n> -\tunsigned int skipped_;\n> -};\n> -\n> -#endif /* __LC_COMPLIANCE_RESULTS_H__ */\n> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> index bfc91b26444e..d5a9d9e8cebe 100644\n> --- a/src/lc-compliance/simple_capture.cpp\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -5,6 +5,8 @@\n>   * simple_capture.cpp - Simple capture helper\n>   */\n>\n> +#include <gtest/gtest.h>\n> +\n>  #include \"simple_capture.h\"\n>\n>  using namespace libcamera;\n> @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture()\n>  \tstop();\n>  }\n>\n> -Results::Result SimpleCapture::configure(StreamRole role)\n> +void SimpleCapture::configure(StreamRole role)\n>  {\n>  \tconfig_ = camera_->generateConfiguration({ role });\n>\n> -\tif (!config_)\n> -\t\treturn { Results::Skip, \"Role not supported by camera\" };\n> +\tif (!config_) {\n> +\t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> +\t\tGTEST_SKIP();\n\nDoesn't GTEST_SKIP() support redirecting strings there ?\n\n> +\t}\n>\n>  \tif (config_->validate() != CameraConfiguration::Valid) {\n>  \t\tconfig_.reset();\n> -\t\treturn { Results::Fail, \"Configuration not valid\" };\n> +\t\tFAIL() << \"Configuration not valid\";\n>  \t}\n>\n>  \tif (camera_->configure(config_.get())) {\n>  \t\tconfig_.reset();\n> -\t\treturn { Results::Fail, \"Failed to configure camera\" };\n> +\t\tFAIL() << \"Failed to configure camera\";\n>  \t}\n> -\n> -\treturn { Results::Pass, \"Configure camera\" };\n>  }\n>\n> -Results::Result SimpleCapture::start()\n> +void SimpleCapture::start()\n>  {\n>  \tStream *stream = config_->at(0).stream();\n> -\tif (allocator_->allocate(stream) < 0)\n> -\t\treturn { Results::Fail, \"Failed to allocate buffers\" };\n> +\tASSERT_GE(allocator_->allocate(stream), 0) << \"Failed to allocate buffers\";\n\nThis should actually be equal to the StreamConfig.bufferCount, doesn't\nit ?\n>\n> -\tif (camera_->start())\n> -\t\treturn { Results::Fail, \"Failed to start camera\" };\n> +\tASSERT_TRUE(!camera_->start()) << \"Failed to start camera\";\n>\n>  \tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> -\n> -\treturn { Results::Pass, \"Started camera\" };\n>  }\n>\n>  void SimpleCapture::stop()\n> @@ -74,22 +72,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)\n>  {\n>  }\n>\n> -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> +void SimpleCaptureBalanced::capture(unsigned int numRequests)\n>  {\n> -\tResults::Result ret = start();\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> +\tstart();\n>\n>  \tStream *stream = config_->at(0).stream();\n>  \tconst std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);\n>\n>  \t/* No point in testing less requests then the camera depth. */\n>  \tif (buffers.size() > numRequests) {\n> -\t\t/* Cache buffers.size() before we destroy it in stop() */\n> -\t\tint buffers_size = buffers.size();\n> -\n> -\t\treturn { Results::Skip, \"Camera needs \" + std::to_string(buffers_size)\n> -\t\t\t+ \" requests, can't test only \" + std::to_string(numRequests) };\n> +\t\tstd::cout << \"Camera needs \" + std::to_string(buffers.size())\n> +\t\t\t+ \" requests, can't test only \"\n> +\t\t\t+ std::to_string(numRequests) << std::endl;\n> +\t\tGTEST_SKIP();\n>  \t}\n>\n>  \tqueueCount_ = 0;\n> @@ -100,14 +95,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n>  \tstd::vector<std::unique_ptr<libcamera::Request>> requests;\n>  \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers) {\n>  \t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> -\t\tif (!request)\n> -\t\t\treturn { Results::Fail, \"Can't create request\" };\n> +\t\tASSERT_TRUE(request) << \"Can't create request\";\n>\n> -\t\tif (request->addBuffer(stream, buffer.get()))\n> -\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> +\t\tASSERT_FALSE(request->addBuffer(stream, buffer.get())) << \"Can't set buffer for request\";\n>\n> -\t\tif (queueRequest(request.get()) < 0)\n> -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> +\t\tASSERT_GE(queueRequest(request.get()), 0) << \"Failed to queue request\";\n>\n>  \t\trequests.push_back(std::move(request));\n>  \t}\n> @@ -118,12 +110,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n>  \tstop();\n>  \tdelete loop_;\n>\n> -\tif (captureCount_ != captureLimit_)\n> -\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) +\n> -\t\t\t\" request, wanted \" + std::to_string(captureLimit_) };\n> -\n> -\treturn { Results::Pass, \"Balanced capture of \" +\n> -\t\tstd::to_string(numRequests) + \" requests\" };\n> +\tASSERT_EQ(captureCount_, captureLimit_);\n>  }\n>\n>  int SimpleCaptureBalanced::queueRequest(Request *request)\n> @@ -155,11 +142,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera)\n>  {\n>  }\n>\n> -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> +void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n>  {\n> -\tResults::Result ret = start();\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> +\tstart();\n>\n>  \tStream *stream = config_->at(0).stream();\n>  \tconst std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);\n> @@ -171,14 +156,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n>  \tstd::vector<std::unique_ptr<libcamera::Request>> requests;\n>  \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers) {\n>  \t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> -\t\tif (!request)\n> -\t\t\treturn { Results::Fail, \"Can't create request\" };\n> +\t\tASSERT_TRUE(request) << \"Can't create request\";\n>\n> -\t\tif (request->addBuffer(stream, buffer.get()))\n> -\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> +\t\tASSERT_FALSE(request->addBuffer(stream, buffer.get())) << \"Can't set buffer for request\";\n>\n> -\t\tif (camera_->queueRequest(request.get()) < 0)\n> -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> +\t\tASSERT_GE(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n>\n>  \t\trequests.push_back(std::move(request));\n>  \t}\n> @@ -189,7 +171,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n>  \tstop();\n>  \tdelete loop_;\n>\n> -\treturn { status ? Results::Fail : Results::Pass, \"Unbalanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> +\tASSERT_FALSE(status);\n>  }\n>\n>  void SimpleCaptureUnbalanced::requestComplete(Request *request)\n> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> index d9de53fb63a3..62984243a1fa 100644\n> --- a/src/lc-compliance/simple_capture.h\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -12,18 +12,17 @@\n>  #include <libcamera/libcamera.h>\n>\n>  #include \"../cam/event_loop.h\"\n> -#include \"results.h\"\n>\n>  class SimpleCapture\n>  {\n>  public:\n> -\tResults::Result configure(libcamera::StreamRole role);\n> +\tvoid configure(libcamera::StreamRole role);\n>\n>  protected:\n>  \tSimpleCapture(std::shared_ptr<libcamera::Camera> camera);\n>  \tvirtual ~SimpleCapture();\n>\n> -\tResults::Result start();\n> +\tvoid start();\n>  \tvoid stop();\n>\n>  \tvirtual void requestComplete(libcamera::Request *request) = 0;\n> @@ -40,7 +39,7 @@ class SimpleCaptureBalanced : public SimpleCapture\n>  public:\n>  \tSimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera);\n>\n> -\tResults::Result capture(unsigned int numRequests);\n> +\tvoid capture(unsigned int numRequests);\n>\n>  private:\n>  \tint queueRequest(libcamera::Request *request);\n> @@ -56,7 +55,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture\n>  public:\n>  \tSimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera);\n>\n> -\tResults::Result capture(unsigned int numRequests);\n> +\tvoid capture(unsigned int numRequests);\n>\n>  private:\n>  \tvoid requestComplete(libcamera::Request *request) override;\n> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp\n> index 8318b42f42d6..fec42981d0e9 100644\n> --- a/src/lc-compliance/single_stream.cpp\n> +++ b/src/lc-compliance/single_stream.cpp\n> @@ -7,91 +7,121 @@\n>\n>  #include <iostream>\n>\n> +#include <gtest/gtest.h>\n> +\n> +#include \"environment.h\"\n>  #include \"simple_capture.h\"\n> -#include \"tests.h\"\n>\n>  using namespace libcamera;\n>\n> -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,\n> -\t\t\t\t   StreamRole role, unsigned int startCycles,\n> -\t\t\t\t   unsigned int numRequests)\n> +const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> +const std::vector<StreamRole> ROLES = { Raw, StillCapture, VideoRecording, Viewfinder };\n> +\n> +class FixedRequestsTest : public testing::TestWithParam<std::tuple<StreamRole, int>>\n> +{\n> +public:\n> +\tstatic std::string nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType> &info);\n> +\n> +protected:\n> +\tvoid SetUp() override;\n> +\tvoid TearDown() override;\n> +\n> +\tstd::shared_ptr<Camera> camera_;\n> +};\n> +\n> +/*\n> + * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> + * in order to be able to assert on them.\n> + */\n> +void FixedRequestsTest::SetUp()\n> +{\n> +\tEnvironment *env = Environment::get();\n> +\n> +\tcamera_ = env->cm()->get(env->cameraId());\n> +\n> +\tASSERT_FALSE(camera_->acquire());\n\nASSERT_EQ(..., 0) ?\n\n> +}\n> +\n> +void FixedRequestsTest::TearDown()\n> +{\n> +\tif (!camera_)\n> +\t\treturn;\n> +\n> +\tcamera_->release();\n> +\tcamera_.reset();\n> +}\n> +\n> +std::string FixedRequestsTest::nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType> &info)\n> +{\n> +\tstd::map<StreamRole, std::string> rolesMap = { { Raw, \"Raw\" },\n> +\t\t\t\t\t\t       { StillCapture, \"StillCapture\" },\n> +\t\t\t\t\t\t       { VideoRecording, \"VideoRecording\" },\n> +\t\t\t\t\t\t       { Viewfinder, \"Viewfinder\" } };\n> +\n> +\tstd::string roleName = rolesMap[std::get<0>(info.param)];\n> +\tstd::string numRequestsName = std::to_string(std::get<1>(info.param));\n> +\n> +\treturn roleName + \"_\" + numRequestsName;\n> +}\n\nNeat!\n\n> +\n> +/*\n> + * Test single capture cycles\n> + *\n> + * Makes sure the camera completes the exact number of requests queued. Example\n> + * failure is a camera that needs N+M requests queued to complete N requests to\n> + * the application.\n> + */\n> +TEST_P(FixedRequestsTest, BalancedSingleCycle)\n\nOn test naming, we now have\nRolesAndRequests/FixedRequestsTest.BalancedSingleCycle/$role_$reqnum\nRolesAndRequests/FixedRequestsTest.BalancedMultiCycle/$role_$reqnum\nRolesAndRequests/FixedRequestsTest.Unalanced/$role_$reqnum\n\nI would make them\n\nCaptureTests/SingleStream.CaptureTest/$role_$reqnum\nCaptureTests/SingleStream.CaptureStartStop/$role_$reqnum\nCaptureTests/SingleStream.UnbalancedStop/$role_$reqnum\n(I admit I didn't get what unbalanced does differently than balanced :)\n\nI would in the first test test all roles like you're doing, and reduce\nthe number of requests we test. The number are pretty arbitrary and\nit's hard to establish which ones are really sensible. Furthermore,\nrobustness tests have to be run for a much higer number of requests,\nso testing 89 or 30 does not really make a difference. I would test\nwith the queue depth as min number of requests and an arbitrary number\nlarger than 2 times the queue depth to make sure we cycle it.\nThis implies the number of requests to test would have to be moved to\nthe simple_capture.cpp as it cannot be declared statically).\n\nIn the second test I would pick the higher number of requests used in\nthe first and test a start-stop sequence. Testing 3 times like it's\ndone now I think it's fine, even just one sequence would be ok\n\nUnbalanced it's not really clear to me what it does so I can't really\ncomment :)\n\nI would also rename this file in capture_test.cpp or similar\n\nI know it sound like a lot of work, and this patch already goes in the\nright direction so it's not a strict requirement to do so in this\nseries.\n\nThanks\n   j\n\n>  {\n> -\tSimpleCaptureBalanced capture(camera);\n> +\tauto [role, numRequests] = GetParam();\n>\n> -\tResults::Result ret = capture.configure(role);\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> +\tSimpleCaptureBalanced capture(camera_);\n>\n> -\tfor (unsigned int starts = 0; starts < startCycles; starts++) {\n> -\t\tret = capture.capture(numRequests);\n> -\t\tif (ret.first != Results::Pass)\n> -\t\t\treturn ret;\n> -\t}\n> +\tcapture.configure(role);\n>\n> -\treturn { Results::Pass, \"Balanced capture of \" +\n> -\t\tstd::to_string(numRequests) + \" requests with \" +\n> -\t\tstd::to_string(startCycles) + \" start cycles\" };\n> +\tcapture.capture(numRequests);\n>  }\n>\n> -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,\n> -\t\t\t\t     StreamRole role, unsigned int numRequests)\n> +/*\n> + * Test multiple start/stop cycles\n> + *\n> + * Makes sure the camera supports multiple start/stop cycles. Example failure is\n> + * a camera that does not clean up correctly in its error path but is only\n> + * tested by single-capture applications.\n> + */\n> +TEST_P(FixedRequestsTest, BalancedMultiCycle)\n>  {\n> -\tSimpleCaptureUnbalanced capture(camera);\n> +\tauto [role, numRequests] = GetParam();\n> +\tunsigned int numRepeats = 3;\n> +\n> +\tSimpleCaptureBalanced capture(camera_);\n>\n> -\tResults::Result ret = capture.configure(role);\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> +\tcapture.configure(role);\n>\n> -\treturn capture.capture(numRequests);\n> +\tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> +\t\tcapture.capture(numRequests);\n>  }\n>\n> -Results testSingleStream(std::shared_ptr<Camera> camera)\n> +/*\n> + * Test unbalanced stop\n> + *\n> + * Makes sure the camera supports a stop with requests queued. Example failure\n> + * is a camera that does not handle cancelation of buffers coming back from the\n> + * video device while stopping.\n> + */\n> +TEST_P(FixedRequestsTest, Unbalanced)\n>  {\n> -\tstatic const std::vector<std::pair<std::string, StreamRole>> roles = {\n> -\t\t{ \"raw\", Raw },\n> -\t\t{ \"still\", StillCapture },\n> -\t\t{ \"video\", VideoRecording },\n> -\t\t{ \"viewfinder\", Viewfinder },\n> -\t};\n> -\tstatic const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> -\n> -\tResults results(numRequests.size() * roles.size() * 3);\n> -\n> -\tfor (const auto &role : roles) {\n> -\t\tstd::cout << \"= Test role \" << role.first << std::endl;\n> -\t\t/*\n> -\t\t * Test single capture cycles\n> -\t\t *\n> -\t\t * Makes sure the camera completes the exact number of requests queued.\n> -\t\t * Example failure is a camera that needs N+M requests queued to\n> -\t\t * complete N requests to the application.\n> -\t\t */\n> -\t\tstd::cout << \"* Test single capture cycles\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestBalance(camera, role.second, 1, num));\n> -\n> -\t\t/*\n> -\t\t * Test multiple start/stop cycles\n> -\t\t *\n> -\t\t * Makes sure the camera supports multiple start/stop cycles.\n> -\t\t * Example failure is a camera that does not clean up correctly in its\n> -\t\t * error path but is only tested by single-capture applications.\n> -\t\t */\n> -\t\tstd::cout << \"* Test multiple start/stop cycles\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestBalance(camera, role.second, 3, num));\n> -\n> -\t\t/*\n> -\t\t * Test unbalanced stop\n> -\t\t *\n> -\t\t * Makes sure the camera supports a stop with requests queued.\n> -\t\t * Example failure is a camera that does not handle cancelation\n> -\t\t * of buffers coming back from the video device while stopping.\n> -\t\t */\n> -\t\tstd::cout << \"* Test unbalanced stop\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestUnbalance(camera, role.second, num));\n> -\t}\n> -\n> -\treturn results;\n> +\tauto [role, numRequests] = GetParam();\n> +\n> +\tSimpleCaptureUnbalanced capture(camera_);\n> +\n> +\tcapture.configure(role);\n> +\n> +\tcapture.capture(numRequests);\n>  }\n> +\n> +INSTANTIATE_TEST_SUITE_P(RolesAndRequests,\n> +\t\t\t FixedRequestsTest,\n> +\t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> +\t\t\t\t\t  testing::ValuesIn(NUMREQUESTS)),\n> +\t\t\t FixedRequestsTest::nameParameters);\n> diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h\n> deleted file mode 100644\n> index 396605214e4b..000000000000\n> --- a/src/lc-compliance/tests.h\n> +++ /dev/null\n> @@ -1,16 +0,0 @@\n> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * tests.h - Test modules\n> - */\n> -#ifndef __LC_COMPLIANCE_TESTS_H__\n> -#define __LC_COMPLIANCE_TESTS_H__\n> -\n> -#include <libcamera/libcamera.h>\n> -\n> -#include \"results.h\"\n> -\n> -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera);\n> -\n> -#endif /* __LC_COMPLIANCE_TESTS_H__ */\n> --\n> 2.32.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5C4ACBD78E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 17 Jun 2021 21:55:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E89E068946;\n\tThu, 17 Jun 2021 23:55:19 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 18DB36050C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 17 Jun 2021 23:55:18 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 159DF200003;\n\tThu, 17 Jun 2021 21:55:16 +0000 (UTC)"],"Date":"Thu, 17 Jun 2021 23:56:04 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado <nfraprado@collabora.com>","Message-ID":"<20210617215604.nhyqkoraptxmekdx@uno.localdomain>","References":"<20210616132535.453411-1-nfraprado@collabora.com>\n\t<20210616132535.453411-5-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210616132535.453411-5-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v8 4/5] lc-compliance: Refactor using\n\tGoogletest","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida <andrealmeid@collabora.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17666,"web_url":"https://patchwork.libcamera.org/comment/17666/","msgid":"<20210621153355.keqlmw72nkxgpx7u@notapiano>","date":"2021-06-21T15:33:55","subject":"Re: [libcamera-devel] [PATCH v8 4/5] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Jacopo,\n\nOn Thu, Jun 17, 2021 at 11:56:04PM +0200, Jacopo Mondi wrote:\n> Hi Nicolas,\n> \n> On Wed, Jun 16, 2021 at 10:25:34AM -0300, Nícolas F. R. A. Prado wrote:\n> > Refactor lc-compliance using Googletest as the test framework.\n> >\n> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> > Changes in v8:\n> > - Thanks to Laurent:\n> >   - Fixed lc-compliance's meson.build to disable when gtest is not installed\n> >   - Fixed compiling error in Clang\n> >\n> > Changes in v7:\n> > - Thanks to Jacopo:\n> >   - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n> >\n> > No changes in v6\n> >\n> > Changes in v5:\n> > - Thanks to Laurent:\n> >   - Fixed style issues\n> >   - Added SetUp and TearDown functions to acquire and release the camera for\n> >     each test\n> >\n> > Changes in v4:\n> > - Removed old lc-compliance results classes\n> > - Thanks to Niklas:\n> >   - Improved naming of tests\n> >\n> > Changes in v3:\n> > - Thanks to Niklas:\n> >   - Went back to static test registration, and created a Singleton Environment\n> >     class to store the camera global variable\n> > - Added a nameParameters() function to give meaningful names to the test\n> >   parameters, removing the need to register each test suite for every role\n> >\n> >  src/lc-compliance/main.cpp           |  73 ++++++------\n> >  src/lc-compliance/meson.build        |   5 +-\n> >  src/lc-compliance/results.cpp        |  75 ------------\n> >  src/lc-compliance/results.h          |  47 --------\n> >  src/lc-compliance/simple_capture.cpp |  74 +++++-------\n> >  src/lc-compliance/simple_capture.h   |   9 +-\n> >  src/lc-compliance/single_stream.cpp  | 172 ++++++++++++++++-----------\n> >  src/lc-compliance/tests.h            |  16 ---\n> >  8 files changed, 174 insertions(+), 297 deletions(-)\n> >  delete mode 100644 src/lc-compliance/results.cpp\n> >  delete mode 100644 src/lc-compliance/results.h\n> >  delete mode 100644 src/lc-compliance/tests.h\n> >\n> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > index 54cee54aa978..8c18845141de 100644\n> > --- a/src/lc-compliance/main.cpp\n> > +++ b/src/lc-compliance/main.cpp\n> > @@ -9,10 +9,12 @@\n> >  #include <iostream>\n> >  #include <string.h>\n> >\n> > +#include <gtest/gtest.h>\n> > +\n> >  #include <libcamera/libcamera.h>\n> >\n> > +#include \"environment.h\"\n> >  #include \"../cam/options.h\"\n> > -#include \"tests.h\"\n> >\n> >  using namespace libcamera;\n> >\n> > @@ -21,21 +23,35 @@ enum {\n> >  \tOptHelp = 'h',\n> >  };\n> >\n> > +/*\n> > + * Make asserts act like exceptions, otherwise they only fail (or skip) the\n> > + * current function. From gtest documentation:\n> > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception\n> > + */\n> > +class ThrowListener : public testing::EmptyTestEventListener\n> > +{\n> > +\tvoid OnTestPartResult(const testing::TestPartResult &result) override\n> > +\t{\n> > +\t\tif (result.type() == testing::TestPartResult::kFatalFailure ||\n> > +\t\t    result.type() == testing::TestPartResult::kSkip)\n> > +\t\t\tthrow testing::AssertionException(result);\n> > +\t}\n> > +};\n> > +\n> >  class Harness\n> \n> What would you think of dropping this class ? It's only used in\n> main.cpp and it basically just wraps the CameraManager.\n> \n> I would keep the cm local, initalize the Environment initialize gtest\n> and call RUN_ALL_TESTS(). listCameras() can be made a static helper\n> function. Maybe I'm missing some perspective, but once we're on gtest\n> nothing more should be add to main.cpp\n\nSounds reasonable. I made that change to see how it would look like and we end\nup with the following helper functions: listCameras, initCamera,\ninitGtestParameters, initGtest, parseOptions. There is more parameter passing of\ncourse but the result seems good, so I'll add it as part of v9.\n\n> \n> >  {\n> >  public:\n> >  \tHarness(const OptionsParser::Options &options);\n> >  \t~Harness();\n> >\n> > -\tint exec();\n> > +\tint init();\n> > +\tint run(int argc, char **argv);\n> >\n> >  private:\n> > -\tint init();\n> >  \tvoid listCameras();\n> >\n> >  \tOptionsParser::Options options_;\n> >  \tstd::unique_ptr<CameraManager> cm_;\n> > -\tstd::shared_ptr<Camera> camera_;\n> >  };\n> >\n> >  Harness::Harness(const OptionsParser::Options &options)\n> > @@ -46,35 +62,13 @@ Harness::Harness(const OptionsParser::Options &options)\n> >\n> >  Harness::~Harness()\n> >  {\n> > -\tif (camera_) {\n> > -\t\tcamera_->release();\n> > -\t\tcamera_.reset();\n> > -\t}\n> > -\n> >  \tcm_->stop();\n> >  }\n> >\n> > -int Harness::exec()\n> > -{\n> > -\tint ret = init();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\tstd::vector<Results> results;\n> > -\n> > -\tresults.push_back(testSingleStream(camera_));\n> > -\n> > -\tfor (const Results &result : results) {\n> > -\t\tret = result.summary();\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> > -\t}\n> > -\n> > -\treturn 0;\n> > -}\n> > -\n> >  int Harness::init()\n> >  {\n> > +\tstd::shared_ptr<Camera> camera;\n> > +\n> >  \tint ret = cm_->start();\n> >  \tif (ret) {\n> >  \t\tstd::cout << \"Failed to start camera manager: \"\n> > @@ -89,23 +83,29 @@ int Harness::init()\n> >  \t}\n> >\n> >  \tconst std::string &cameraId = options_[OptCamera];\n> > -\tcamera_ = cm_->get(cameraId);\n> > -\tif (!camera_) {\n> > +\tcamera = cm_->get(cameraId);\n> > +\tif (!camera) {\n> >  \t\tstd::cout << \"Camera \" << cameraId << \" not found, available cameras:\" << std::endl;\n> >  \t\tlistCameras();\n> >  \t\treturn -ENODEV;\n> >  \t}\n> >\n> > -\tif (camera_->acquire()) {\n> > -\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > +\tEnvironment::get()->setup(cm_.get(), cameraId);\n> >\n> >  \tstd::cout << \"Using camera \" << cameraId << std::endl;\n> >\n> >  \treturn 0;\n> >  }\n> >\n> > +int Harness::run(int argc, char **argv)\n> > +{\n> > +\t::testing::InitGoogleTest(&argc, argv);\n> > +\n> > +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> > +\n> > +\treturn RUN_ALL_TESTS();\n> > +}\n> > +\n> >  void Harness::listCameras()\n> >  {\n> >  \tfor (const std::shared_ptr<Camera> &cam : cm_->cameras())\n> > @@ -143,6 +143,9 @@ int main(int argc, char **argv)\n> >  \t\treturn EXIT_FAILURE;\n> >\n> >  \tHarness harness(options);\n> > +\tret = harness.init();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> >\n> > -\treturn harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;\n> > +\treturn harness.run(argc, argv);\n> >  }\n> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> > index 6dd49085569f..097804c7aeda 100644\n> > --- a/src/lc-compliance/meson.build\n> > +++ b/src/lc-compliance/meson.build\n> > @@ -1,8 +1,9 @@\n> >  # SPDX-License-Identifier: CC0-1.0\n> >\n> >  libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))\n> > +libgtest = dependency('gtest', required : get_option('lc-compliance'))\n> >\n> > -if not libevent.found()\n> > +if not (libevent.found() and libgtest.found())\n> >      lc_compliance_enabled = false\n> >      subdir_done()\n> >  endif\n> > @@ -14,7 +15,6 @@ lc_compliance_sources = files([\n> >      '../cam/options.cpp',\n> >      'environment.cpp',\n> >      'main.cpp',\n> > -    'results.cpp',\n> >      'simple_capture.cpp',\n> >      'single_stream.cpp',\n> >  ])\n> > @@ -24,5 +24,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,\n> >                                  libatomic,\n> >                                  libcamera_dep,\n> >                                  libevent,\n> > +                                libgtest,\n> >                              ],\n> >                              install : true)\n> > diff --git a/src/lc-compliance/results.cpp b/src/lc-compliance/results.cpp\n> > deleted file mode 100644\n> > index f149f7859e28..000000000000\n> > --- a/src/lc-compliance/results.cpp\n> > +++ /dev/null\n> > @@ -1,75 +0,0 @@\n> > -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Google Inc.\n> > - *\n> > - * results.cpp - Test result aggregator\n> > - */\n> > -\n> > -#include \"results.h\"\n> > -\n> > -#include <iostream>\n> > -\n> > -void Results::add(const Result &result)\n> > -{\n> > -\tif (result.first == Pass)\n> > -\t\tpassed_++;\n> > -\telse if (result.first == Fail)\n> > -\t\tfailed_++;\n> > -\telse if (result.first == Skip)\n> > -\t\tskipped_++;\n> > -\n> > -\tprintResult(result);\n> > -}\n> > -\n> > -void Results::add(Status status, const std::string &message)\n> > -{\n> > -\tadd({ status, message });\n> > -}\n> > -\n> > -void Results::fail(const std::string &message)\n> > -{\n> > -\tadd(Fail, message);\n> > -}\n> > -\n> > -void Results::pass(const std::string &message)\n> > -{\n> > -\tadd(Pass, message);\n> > -}\n> > -\n> > -void Results::skip(const std::string &message)\n> > -{\n> > -\tadd(Skip, message);\n> > -}\n> > -\n> > -int Results::summary() const\n> > -{\n> > -\tif (failed_ + passed_ + skipped_ != planned_) {\n> > -\t\tstd::cout << \"Planned and executed number of tests differ \"\n> > -\t\t\t  << failed_ + passed_ + skipped_ << \" executed \"\n> > -\t\t\t  << planned_ << \" planned\" << std::endl;\n> > -\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > -\n> > -\tstd::cout << planned_ << \" tests executed, \"\n> > -\t\t  << passed_ << \" tests passed, \"\n> > -\t\t  << skipped_ << \" tests skipped and \"\n> > -\t\t  << failed_ << \" tests failed \" << std::endl;\n> > -\n> > -\treturn 0;\n> > -}\n> > -\n> > -void Results::printResult(const Result &result)\n> > -{\n> > -\tstd::string prefix;\n> > -\n> > -\t/* \\todo Make parsable as TAP. */\n> > -\tif (result.first == Pass)\n> > -\t\tprefix = \"PASS\";\n> > -\telse if (result.first == Fail)\n> > -\t\tprefix = \"FAIL\";\n> > -\telse if (result.first == Skip)\n> > -\t\tprefix = \"SKIP\";\n> > -\n> > -\tstd::cout << \"- \" << prefix << \": \" << result.second << std::endl;\n> > -}\n> > diff --git a/src/lc-compliance/results.h b/src/lc-compliance/results.h\n> > deleted file mode 100644\n> > index 2a3722b841a6..000000000000\n> > --- a/src/lc-compliance/results.h\n> > +++ /dev/null\n> > @@ -1,47 +0,0 @@\n> > -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Google Inc.\n> > - *\n> > - * results.h - Test result aggregator\n> > - */\n> > -#ifndef __LC_COMPLIANCE_RESULTS_H__\n> > -#define __LC_COMPLIANCE_RESULTS_H__\n> > -\n> > -#include <string>\n> > -#include <utility>\n> > -\n> > -/* \\todo Check if result aggregator can be shared with self tests in test/ */\n> > -class Results\n> > -{\n> > -public:\n> > -\tenum Status {\n> > -\t\tFail,\n> > -\t\tPass,\n> > -\t\tSkip,\n> > -\t};\n> > -\n> > -\tusing Result = std::pair<Status, std::string>;\n> > -\n> > -\tResults(unsigned int planned)\n> > -\t\t: planned_(planned), passed_(0), failed_(0), skipped_(0)\n> > -\t{\n> > -\t}\n> > -\n> > -\tvoid add(const Result &result);\n> > -\tvoid add(Status status, const std::string &message);\n> > -\tvoid fail(const std::string &message);\n> > -\tvoid pass(const std::string &message);\n> > -\tvoid skip(const std::string &message);\n> > -\n> > -\tint summary() const;\n> > -\n> > -private:\n> > -\tvoid printResult(const Result &result);\n> > -\n> > -\tunsigned int planned_;\n> > -\tunsigned int passed_;\n> > -\tunsigned int failed_;\n> > -\tunsigned int skipped_;\n> > -};\n> > -\n> > -#endif /* __LC_COMPLIANCE_RESULTS_H__ */\n> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> > index bfc91b26444e..d5a9d9e8cebe 100644\n> > --- a/src/lc-compliance/simple_capture.cpp\n> > +++ b/src/lc-compliance/simple_capture.cpp\n> > @@ -5,6 +5,8 @@\n> >   * simple_capture.cpp - Simple capture helper\n> >   */\n> >\n> > +#include <gtest/gtest.h>\n> > +\n> >  #include \"simple_capture.h\"\n> >\n> >  using namespace libcamera;\n> > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture()\n> >  \tstop();\n> >  }\n> >\n> > -Results::Result SimpleCapture::configure(StreamRole role)\n> > +void SimpleCapture::configure(StreamRole role)\n> >  {\n> >  \tconfig_ = camera_->generateConfiguration({ role });\n> >\n> > -\tif (!config_)\n> > -\t\treturn { Results::Skip, \"Role not supported by camera\" };\n> > +\tif (!config_) {\n> > +\t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> > +\t\tGTEST_SKIP();\n> \n> Doesn't GTEST_SKIP() support redirecting strings there ?\n\nUntil very recently, no. That bug was fixed two years ago [1] but there hadn't\nbeen a new version release of gtest since. Luckily a new version was released a\nbit over a week ago [2].\n\nI run a rolling release distro so I just tested the latest version and it's\nindeed fixed there, but that's not the case for a lot of people (including\nKernelCI, which is on Debian Buster). In those cases the skip message would be\ninvisible, so I'm thinking we should keep this hack for the time being\nunfortunately...\n\n[1] https://github.com/google/googletest/pull/2517\n[2] https://github.com/google/googletest/releases\n\n> \n> > +\t}\n> >\n> >  \tif (config_->validate() != CameraConfiguration::Valid) {\n> >  \t\tconfig_.reset();\n> > -\t\treturn { Results::Fail, \"Configuration not valid\" };\n> > +\t\tFAIL() << \"Configuration not valid\";\n> >  \t}\n> >\n> >  \tif (camera_->configure(config_.get())) {\n> >  \t\tconfig_.reset();\n> > -\t\treturn { Results::Fail, \"Failed to configure camera\" };\n> > +\t\tFAIL() << \"Failed to configure camera\";\n> >  \t}\n> > -\n> > -\treturn { Results::Pass, \"Configure camera\" };\n> >  }\n> >\n> > -Results::Result SimpleCapture::start()\n> > +void SimpleCapture::start()\n> >  {\n> >  \tStream *stream = config_->at(0).stream();\n> > -\tif (allocator_->allocate(stream) < 0)\n> > -\t\treturn { Results::Fail, \"Failed to allocate buffers\" };\n> > +\tASSERT_GE(allocator_->allocate(stream), 0) << \"Failed to allocate buffers\";\n> \n> This should actually be equal to the StreamConfig.bufferCount, doesn't\n> it ?\n\nTechnically yes, but on closer inspection, V4L2VideoDevice::createBuffers()\neither returns the total count or an error value: it errors out even if a single\nbuffer fails to allocate.\n\nSince the capture loop works regardless of if less buffers than requested have\nbeen allocated, I suggest adding after that ASSERT_GE line:\n\n\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n\nThis would let the test continue running through the end, but mark it as failed.\nIt should cover us if the allocate() API ever changes to let allocations of less\nbuffers than requested be considered successful.\n\n> >\n> > -\tif (camera_->start())\n> > -\t\treturn { Results::Fail, \"Failed to start camera\" };\n> > +\tASSERT_TRUE(!camera_->start()) << \"Failed to start camera\";\n> >\n> >  \tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > -\n> > -\treturn { Results::Pass, \"Started camera\" };\n> >  }\n> >\n> >  void SimpleCapture::stop()\n> > @@ -74,22 +72,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)\n> >  {\n> >  }\n> >\n> > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > +void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> >  {\n> > -\tResults::Result ret = start();\n> > -\tif (ret.first != Results::Pass)\n> > -\t\treturn ret;\n> > +\tstart();\n> >\n> >  \tStream *stream = config_->at(0).stream();\n> >  \tconst std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);\n> >\n> >  \t/* No point in testing less requests then the camera depth. */\n> >  \tif (buffers.size() > numRequests) {\n> > -\t\t/* Cache buffers.size() before we destroy it in stop() */\n> > -\t\tint buffers_size = buffers.size();\n> > -\n> > -\t\treturn { Results::Skip, \"Camera needs \" + std::to_string(buffers_size)\n> > -\t\t\t+ \" requests, can't test only \" + std::to_string(numRequests) };\n> > +\t\tstd::cout << \"Camera needs \" + std::to_string(buffers.size())\n> > +\t\t\t+ \" requests, can't test only \"\n> > +\t\t\t+ std::to_string(numRequests) << std::endl;\n> > +\t\tGTEST_SKIP();\n> >  \t}\n> >\n> >  \tqueueCount_ = 0;\n> > @@ -100,14 +95,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> >  \tstd::vector<std::unique_ptr<libcamera::Request>> requests;\n> >  \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers) {\n> >  \t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> > -\t\tif (!request)\n> > -\t\t\treturn { Results::Fail, \"Can't create request\" };\n> > +\t\tASSERT_TRUE(request) << \"Can't create request\";\n> >\n> > -\t\tif (request->addBuffer(stream, buffer.get()))\n> > -\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> > +\t\tASSERT_FALSE(request->addBuffer(stream, buffer.get())) << \"Can't set buffer for request\";\n> >\n> > -\t\tif (queueRequest(request.get()) < 0)\n> > -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> > +\t\tASSERT_GE(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> >\n> >  \t\trequests.push_back(std::move(request));\n> >  \t}\n> > @@ -118,12 +110,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> >  \tstop();\n> >  \tdelete loop_;\n> >\n> > -\tif (captureCount_ != captureLimit_)\n> > -\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) +\n> > -\t\t\t\" request, wanted \" + std::to_string(captureLimit_) };\n> > -\n> > -\treturn { Results::Pass, \"Balanced capture of \" +\n> > -\t\tstd::to_string(numRequests) + \" requests\" };\n> > +\tASSERT_EQ(captureCount_, captureLimit_);\n> >  }\n> >\n> >  int SimpleCaptureBalanced::queueRequest(Request *request)\n> > @@ -155,11 +142,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera)\n> >  {\n> >  }\n> >\n> > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> >  {\n> > -\tResults::Result ret = start();\n> > -\tif (ret.first != Results::Pass)\n> > -\t\treturn ret;\n> > +\tstart();\n> >\n> >  \tStream *stream = config_->at(0).stream();\n> >  \tconst std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);\n> > @@ -171,14 +156,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> >  \tstd::vector<std::unique_ptr<libcamera::Request>> requests;\n> >  \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers) {\n> >  \t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> > -\t\tif (!request)\n> > -\t\t\treturn { Results::Fail, \"Can't create request\" };\n> > +\t\tASSERT_TRUE(request) << \"Can't create request\";\n> >\n> > -\t\tif (request->addBuffer(stream, buffer.get()))\n> > -\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> > +\t\tASSERT_FALSE(request->addBuffer(stream, buffer.get())) << \"Can't set buffer for request\";\n> >\n> > -\t\tif (camera_->queueRequest(request.get()) < 0)\n> > -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> > +\t\tASSERT_GE(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> >\n> >  \t\trequests.push_back(std::move(request));\n> >  \t}\n> > @@ -189,7 +171,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> >  \tstop();\n> >  \tdelete loop_;\n> >\n> > -\treturn { status ? Results::Fail : Results::Pass, \"Unbalanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> > +\tASSERT_FALSE(status);\n> >  }\n> >\n> >  void SimpleCaptureUnbalanced::requestComplete(Request *request)\n> > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> > index d9de53fb63a3..62984243a1fa 100644\n> > --- a/src/lc-compliance/simple_capture.h\n> > +++ b/src/lc-compliance/simple_capture.h\n> > @@ -12,18 +12,17 @@\n> >  #include <libcamera/libcamera.h>\n> >\n> >  #include \"../cam/event_loop.h\"\n> > -#include \"results.h\"\n> >\n> >  class SimpleCapture\n> >  {\n> >  public:\n> > -\tResults::Result configure(libcamera::StreamRole role);\n> > +\tvoid configure(libcamera::StreamRole role);\n> >\n> >  protected:\n> >  \tSimpleCapture(std::shared_ptr<libcamera::Camera> camera);\n> >  \tvirtual ~SimpleCapture();\n> >\n> > -\tResults::Result start();\n> > +\tvoid start();\n> >  \tvoid stop();\n> >\n> >  \tvirtual void requestComplete(libcamera::Request *request) = 0;\n> > @@ -40,7 +39,7 @@ class SimpleCaptureBalanced : public SimpleCapture\n> >  public:\n> >  \tSimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera);\n> >\n> > -\tResults::Result capture(unsigned int numRequests);\n> > +\tvoid capture(unsigned int numRequests);\n> >\n> >  private:\n> >  \tint queueRequest(libcamera::Request *request);\n> > @@ -56,7 +55,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture\n> >  public:\n> >  \tSimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera);\n> >\n> > -\tResults::Result capture(unsigned int numRequests);\n> > +\tvoid capture(unsigned int numRequests);\n> >\n> >  private:\n> >  \tvoid requestComplete(libcamera::Request *request) override;\n> > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp\n> > index 8318b42f42d6..fec42981d0e9 100644\n> > --- a/src/lc-compliance/single_stream.cpp\n> > +++ b/src/lc-compliance/single_stream.cpp\n> > @@ -7,91 +7,121 @@\n> >\n> >  #include <iostream>\n> >\n> > +#include <gtest/gtest.h>\n> > +\n> > +#include \"environment.h\"\n> >  #include \"simple_capture.h\"\n> > -#include \"tests.h\"\n> >\n> >  using namespace libcamera;\n> >\n> > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,\n> > -\t\t\t\t   StreamRole role, unsigned int startCycles,\n> > -\t\t\t\t   unsigned int numRequests)\n> > +const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> > +const std::vector<StreamRole> ROLES = { Raw, StillCapture, VideoRecording, Viewfinder };\n> > +\n> > +class FixedRequestsTest : public testing::TestWithParam<std::tuple<StreamRole, int>>\n> > +{\n> > +public:\n> > +\tstatic std::string nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType> &info);\n> > +\n> > +protected:\n> > +\tvoid SetUp() override;\n> > +\tvoid TearDown() override;\n> > +\n> > +\tstd::shared_ptr<Camera> camera_;\n> > +};\n> > +\n> > +/*\n> > + * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> > + * in order to be able to assert on them.\n> > + */\n> > +void FixedRequestsTest::SetUp()\n> > +{\n> > +\tEnvironment *env = Environment::get();\n> > +\n> > +\tcamera_ = env->cm()->get(env->cameraId());\n> > +\n> > +\tASSERT_FALSE(camera_->acquire());\n> \n> ASSERT_EQ(..., 0) ?\n\nYeah, fixed.\n\n> \n> > +}\n> > +\n> > +void FixedRequestsTest::TearDown()\n> > +{\n> > +\tif (!camera_)\n> > +\t\treturn;\n> > +\n> > +\tcamera_->release();\n> > +\tcamera_.reset();\n> > +}\n> > +\n> > +std::string FixedRequestsTest::nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType> &info)\n> > +{\n> > +\tstd::map<StreamRole, std::string> rolesMap = { { Raw, \"Raw\" },\n> > +\t\t\t\t\t\t       { StillCapture, \"StillCapture\" },\n> > +\t\t\t\t\t\t       { VideoRecording, \"VideoRecording\" },\n> > +\t\t\t\t\t\t       { Viewfinder, \"Viewfinder\" } };\n> > +\n> > +\tstd::string roleName = rolesMap[std::get<0>(info.param)];\n> > +\tstd::string numRequestsName = std::to_string(std::get<1>(info.param));\n> > +\n> > +\treturn roleName + \"_\" + numRequestsName;\n> > +}\n> \n> Neat!\n> \n> > +\n> > +/*\n> > + * Test single capture cycles\n> > + *\n> > + * Makes sure the camera completes the exact number of requests queued. Example\n> > + * failure is a camera that needs N+M requests queued to complete N requests to\n> > + * the application.\n> > + */\n> > +TEST_P(FixedRequestsTest, BalancedSingleCycle)\n> \n> On test naming, we now have\n> RolesAndRequests/FixedRequestsTest.BalancedSingleCycle/$role_$reqnum\n> RolesAndRequests/FixedRequestsTest.BalancedMultiCycle/$role_$reqnum\n> RolesAndRequests/FixedRequestsTest.Unalanced/$role_$reqnum\n> \n> I would make them\n> \n> CaptureTests/SingleStream.CaptureTest/$role_$reqnum\n> CaptureTests/SingleStream.CaptureStartStop/$role_$reqnum\n> CaptureTests/SingleStream.UnbalancedStop/$role_$reqnum\n> (I admit I didn't get what unbalanced does differently than balanced :)\n\nSure. I think the naming convention will probably change anyway as we add more\ntests and notice inconsistencies. Examples:\n\n* the \"CaptureTests/\" there at the beginning won't present if the test isn't\n  parametrized, and instead just run once.\n\n* the \"SingleStream.\" test fixture class is specific to that combination of\n  parameters: StreamRole and int for the number of requests. So if we have tests\n  that need other parameters they won't be called \"SingleStream.\" anymore. (I\n  did make one such test [3], which I'll resend after this series is merged)\n\n[3] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020754.html\n\nSo I'm fine with those names for now even though I don't think they'll make much\nsense after more tests are added, but it's easy enough to change them again\nwhen needed.\n\n> \n> I would in the first test test all roles like you're doing, and reduce\n> the number of requests we test. The number are pretty arbitrary and\n> it's hard to establish which ones are really sensible. Furthermore,\n> robustness tests have to be run for a much higer number of requests,\n> so testing 89 or 30 does not really make a difference. I would test\n> with the queue depth as min number of requests and an arbitrary number\n> larger than 2 times the queue depth to make sure we cycle it.\n\nSince this is changing the way the tests are done, I think it would be best\ndiscussed and addressed after this series.\n\n> This implies the number of requests to test would have to be moved to\n> the simple_capture.cpp as it cannot be declared statically).\n\nNeeding to move the number of requests would actually be a big problem.\nBasically when using gtest all test cases should be defined statically (it is\nactually possible to define dynamically but it's overly complicated and we\ndecided to keep it static [4]).\n\n[4] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020330.html\n\n> \n> In the second test I would pick the higher number of requests used in\n> the first and test a start-stop sequence. Testing 3 times like it's\n> done now I think it's fine, even just one sequence would be ok\n> \n> Unbalanced it's not really clear to me what it does so I can't really\n> comment :)\n\nBasically what unbalanced does differently from balanced is that it doesn't\ncheck if the needed amount of requests were already queued before queuing a new\none. So if it just needs one more request but has 8 free buffers, it might queue\nall 8, and after the first one completes and makes it exit the loop and call\nstop(), the other 7 need to be cancelled, exposing bugs if the cancellation\nisn't done well by the camera.\n\n> \n> I would also rename this file in capture_test.cpp or similar\n\nSure.\n\nThanks,\nNícolas\n\n> \n> I know it sound like a lot of work, and this patch already goes in the\n> right direction so it's not a strict requirement to do so in this\n> series.\n> \n> Thanks\n>    j\n> \n> >  {\n> > -\tSimpleCaptureBalanced capture(camera);\n> > +\tauto [role, numRequests] = GetParam();\n> >\n> > -\tResults::Result ret = capture.configure(role);\n> > -\tif (ret.first != Results::Pass)\n> > -\t\treturn ret;\n> > +\tSimpleCaptureBalanced capture(camera_);\n> >\n> > -\tfor (unsigned int starts = 0; starts < startCycles; starts++) {\n> > -\t\tret = capture.capture(numRequests);\n> > -\t\tif (ret.first != Results::Pass)\n> > -\t\t\treturn ret;\n> > -\t}\n> > +\tcapture.configure(role);\n> >\n> > -\treturn { Results::Pass, \"Balanced capture of \" +\n> > -\t\tstd::to_string(numRequests) + \" requests with \" +\n> > -\t\tstd::to_string(startCycles) + \" start cycles\" };\n> > +\tcapture.capture(numRequests);\n> >  }\n> >\n> > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,\n> > -\t\t\t\t     StreamRole role, unsigned int numRequests)\n> > +/*\n> > + * Test multiple start/stop cycles\n> > + *\n> > + * Makes sure the camera supports multiple start/stop cycles. Example failure is\n> > + * a camera that does not clean up correctly in its error path but is only\n> > + * tested by single-capture applications.\n> > + */\n> > +TEST_P(FixedRequestsTest, BalancedMultiCycle)\n> >  {\n> > -\tSimpleCaptureUnbalanced capture(camera);\n> > +\tauto [role, numRequests] = GetParam();\n> > +\tunsigned int numRepeats = 3;\n> > +\n> > +\tSimpleCaptureBalanced capture(camera_);\n> >\n> > -\tResults::Result ret = capture.configure(role);\n> > -\tif (ret.first != Results::Pass)\n> > -\t\treturn ret;\n> > +\tcapture.configure(role);\n> >\n> > -\treturn capture.capture(numRequests);\n> > +\tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> > +\t\tcapture.capture(numRequests);\n> >  }\n> >\n> > -Results testSingleStream(std::shared_ptr<Camera> camera)\n> > +/*\n> > + * Test unbalanced stop\n> > + *\n> > + * Makes sure the camera supports a stop with requests queued. Example failure\n> > + * is a camera that does not handle cancelation of buffers coming back from the\n> > + * video device while stopping.\n> > + */\n> > +TEST_P(FixedRequestsTest, Unbalanced)\n> >  {\n> > -\tstatic const std::vector<std::pair<std::string, StreamRole>> roles = {\n> > -\t\t{ \"raw\", Raw },\n> > -\t\t{ \"still\", StillCapture },\n> > -\t\t{ \"video\", VideoRecording },\n> > -\t\t{ \"viewfinder\", Viewfinder },\n> > -\t};\n> > -\tstatic const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> > -\n> > -\tResults results(numRequests.size() * roles.size() * 3);\n> > -\n> > -\tfor (const auto &role : roles) {\n> > -\t\tstd::cout << \"= Test role \" << role.first << std::endl;\n> > -\t\t/*\n> > -\t\t * Test single capture cycles\n> > -\t\t *\n> > -\t\t * Makes sure the camera completes the exact number of requests queued.\n> > -\t\t * Example failure is a camera that needs N+M requests queued to\n> > -\t\t * complete N requests to the application.\n> > -\t\t */\n> > -\t\tstd::cout << \"* Test single capture cycles\" << std::endl;\n> > -\t\tfor (unsigned int num : numRequests)\n> > -\t\t\tresults.add(testRequestBalance(camera, role.second, 1, num));\n> > -\n> > -\t\t/*\n> > -\t\t * Test multiple start/stop cycles\n> > -\t\t *\n> > -\t\t * Makes sure the camera supports multiple start/stop cycles.\n> > -\t\t * Example failure is a camera that does not clean up correctly in its\n> > -\t\t * error path but is only tested by single-capture applications.\n> > -\t\t */\n> > -\t\tstd::cout << \"* Test multiple start/stop cycles\" << std::endl;\n> > -\t\tfor (unsigned int num : numRequests)\n> > -\t\t\tresults.add(testRequestBalance(camera, role.second, 3, num));\n> > -\n> > -\t\t/*\n> > -\t\t * Test unbalanced stop\n> > -\t\t *\n> > -\t\t * Makes sure the camera supports a stop with requests queued.\n> > -\t\t * Example failure is a camera that does not handle cancelation\n> > -\t\t * of buffers coming back from the video device while stopping.\n> > -\t\t */\n> > -\t\tstd::cout << \"* Test unbalanced stop\" << std::endl;\n> > -\t\tfor (unsigned int num : numRequests)\n> > -\t\t\tresults.add(testRequestUnbalance(camera, role.second, num));\n> > -\t}\n> > -\n> > -\treturn results;\n> > +\tauto [role, numRequests] = GetParam();\n> > +\n> > +\tSimpleCaptureUnbalanced capture(camera_);\n> > +\n> > +\tcapture.configure(role);\n> > +\n> > +\tcapture.capture(numRequests);\n> >  }\n> > +\n> > +INSTANTIATE_TEST_SUITE_P(RolesAndRequests,\n> > +\t\t\t FixedRequestsTest,\n> > +\t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > +\t\t\t\t\t  testing::ValuesIn(NUMREQUESTS)),\n> > +\t\t\t FixedRequestsTest::nameParameters);\n> > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h\n> > deleted file mode 100644\n> > index 396605214e4b..000000000000\n> > --- a/src/lc-compliance/tests.h\n> > +++ /dev/null\n> > @@ -1,16 +0,0 @@\n> > -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Google Inc.\n> > - *\n> > - * tests.h - Test modules\n> > - */\n> > -#ifndef __LC_COMPLIANCE_TESTS_H__\n> > -#define __LC_COMPLIANCE_TESTS_H__\n> > -\n> > -#include <libcamera/libcamera.h>\n> > -\n> > -#include \"results.h\"\n> > -\n> > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera);\n> > -\n> > -#endif /* __LC_COMPLIANCE_TESTS_H__ */\n> > --\n> > 2.32.0\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F4145BE58C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Jun 2021 15:34:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72B086893A;\n\tMon, 21 Jun 2021 17:34:45 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 990A360295\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Jun 2021 17:34:44 +0200 (CEST)","from notapiano (unknown\n\t[IPv6:2804:14c:1a9:2434:73c2:f0a9:af4a:cee])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\t(Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id D314C1F4234D;\n\tMon, 21 Jun 2021 16:34:42 +0100 (BST)"],"Date":"Mon, 21 Jun 2021 12:33:55 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado <nfraprado@collabora.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20210621153355.keqlmw72nkxgpx7u@notapiano>","References":"<20210616132535.453411-1-nfraprado@collabora.com>\n\t<20210616132535.453411-5-nfraprado@collabora.com>\n\t<20210617215604.nhyqkoraptxmekdx@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210617215604.nhyqkoraptxmekdx@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v8 4/5] lc-compliance: Refactor using\n\tGoogletest","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida <andrealmeid@collabora.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]