[{"id":17116,"web_url":"https://patchwork.libcamera.org/comment/17116/","msgid":"","date":"2021-05-22T10:16:48","subject":"Re: [libcamera-devel] [PATCH v4 4/5] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Nícolas,\n\nThanks for your effort!\n\nOn 2021-05-21 10:30:53 -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 \n\nReviewed-by: Niklas Söderlund \n\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 | 71 +++++++------\n> src/lc-compliance/meson.build | 4 +-\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 | 150 ++++++++++++++-------------\n> src/lc-compliance/tests.h | 16 ---\n> 8 files changed, 152 insertions(+), 294 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..37884ff70a69 100644\n> --- a/src/lc-compliance/main.cpp\n> +++ b/src/lc-compliance/main.cpp\n> @@ -9,10 +9,12 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> \n> +#include \"environment.h\"\n> #include \"../cam/options.h\"\n> -#include \"tests.h\"\n> \n> using namespace libcamera;\n> \n> @@ -21,21 +23,34 @@ 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> +\tvoid OnTestPartResult(const testing::TestPartResult& result) override {\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\t}\n> +\t}\n> +};\n> +\n> class Harness\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 cm_;\n> -\tstd::shared_ptr camera_;\n> };\n> \n> Harness::Harness(const OptionsParser::Options &options)\n> @@ -46,35 +61,15 @@ 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> +\tEnvironment::instance()->destroy();\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;\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;\n> +\n> \tint ret = cm_->start();\n> \tif (ret) {\n> \t\tstd::cout << \"Failed to start camera manager: \"\n> @@ -89,23 +84,34 @@ 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> +\tif (camera->acquire()) {\n> \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tEnvironment::create(camera);\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 &cam : cm_->cameras())\n> @@ -143,6 +149,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 c287017575bd..ae68844ac9bd 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -13,16 +13,18 @@ lc_compliance_sources = files([\n> '../cam/event_loop.cpp',\n> '../cam/options.cpp',\n> 'main.cpp',\n> - 'results.cpp',\n> 'environment.cpp',\n> 'simple_capture.cpp',\n> 'single_stream.cpp',\n> ])\n> \n> +gtest_dep = dependency('gtest')\n> +\n> lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> dependencies : [\n> libatomic,\n> libcamera_dep,\n> libevent,\n> + gtest_dep,\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 \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 \n> -#include \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;\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 f90fe6d0f9aa..7731eb16f8c2 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 \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> +\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> -\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> @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr 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> &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> @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> \tstd::vector> requests;\n> \tfor (const std::unique_ptr &buffer : buffers) {\n> \t\tstd::unique_ptr 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> @@ -121,12 +113,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> @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr 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> &buffers = allocator_->buffers(stream);\n> @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> \tstd::vector> requests;\n> \tfor (const std::unique_ptr &buffer : buffers) {\n> \t\tstd::unique_ptr 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> @@ -192,7 +174,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 \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 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 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 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..a8ae2f0e355b 100644\n> --- a/src/lc-compliance/single_stream.cpp\n> +++ b/src/lc-compliance/single_stream.cpp\n> @@ -7,91 +7,95 @@\n> \n> #include \n> \n> +#include \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,\n> -\t\t\t\t StreamRole role, unsigned int startCycles,\n> -\t\t\t\t unsigned int numRequests)\n> +const std::vector NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89};\n> +const std::vector ROLES = {Raw, StillCapture, VideoRecording, Viewfinder};\n> +\n> +class FixedRequestsTest :\n> +\tpublic testing::TestWithParam> {\n> +};\n> +\n> +std::string nameParameters(const testing::TestParamInfo& info)\n> {\n> +\tstd::map 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> +/*\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> +\tauto [role, numRequests] = GetParam();\n> +\tstd::shared_ptr camera = Environment::instance()->camera();\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> -\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.capture(numRequests);\n> +};\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> -}\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> +\tauto [role, numRequests] = GetParam();\n> +\tstd::shared_ptr camera = Environment::instance()->camera();\n> +\tunsigned int numRepeats = 3;\n> +\n> +\tSimpleCaptureBalanced capture(camera);\n> \n> -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> -\t\t\t\t StreamRole role, unsigned int numRequests)\n> +\tcapture.configure(role);\n> +\n> +\tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> +\t\tcapture.capture(numRequests);\n> +};\n> +\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> +\tauto [role, numRequests] = GetParam();\n> +\tstd::shared_ptr camera = Environment::instance()->camera();\n> +\n> \tSimpleCaptureUnbalanced 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> -}\n> +\tcapture.capture(numRequests);\n> +};\n> \n> -Results testSingleStream(std::shared_ptr camera)\n> -{\n> -\tstatic const std::vector> 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 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> -}\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 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 \n> -\n> -#include \"results.h\"\n> -\n> -Results testSingleStream(std::shared_ptr camera);\n> -\n> -#endif /* __LC_COMPLIANCE_TESTS_H__ */\n> -- \n> 2.31.1\n>","headers":{"Return-Path":"","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 03CB4C3200\n\tfor ;\n\tSat, 22 May 2021 10:16:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7052F6891F;\n\tSat, 22 May 2021 12:16:51 +0200 (CEST)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74FEF60510\n\tfor ;\n\tSat, 22 May 2021 12:16:50 +0200 (CEST)","by mail-lj1-x232.google.com with SMTP id e11so26896277ljn.13\n\tfor ;\n\tSat, 22 May 2021 03:16:50 -0700 (PDT)","from localhost (h-155-4-209-203.A463.priv.bahnhof.se.\n\t[155.4.209.203]) by smtp.gmail.com with ESMTPSA id\n\tu27sm880467lfm.239.2021.05.22.03.16.48\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 22 May 2021 03:16:49 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"DKst9tA+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=4ZOnxV259pFWAJoSTWWezPYtKdmCel6qFacxPP28+BU=;\n\tb=DKst9tA+6c2f2Kk9+cX2TWCoXiZ39ot0GulMFX1IpJoQawwEf2VZg2VKIT4GA9uNu0\n\tvyttqi3r5wZP5nkVo6SXFRVYRuPJn4LtCTRXzcFJV3+jRgVUocI0UY9MJRx6jsXZZDBW\n\t0c1SOgEr5iQLpoPs0KlhXuASyPVGlcjdu3dEbezU2mYJH4T0HHgvMwLudHMc+7JW4hwV\n\thTCg2Q5B21WOURbaca4nBoE7QwE9lE/EQJ80Zgm8eS52e+9afMtm/do0n+t6Cm1HFoCJ\n\tx9ach76+Itk0AI1PWSiHiR9B4D8UJkno8a7ZoYALPReGmvxhG7jR7zKVkQkwgPNTdViL\n\tF2Sg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=4ZOnxV259pFWAJoSTWWezPYtKdmCel6qFacxPP28+BU=;\n\tb=oG0UcYGPbTrUmPB1fllBqHQJXrBXXI7GbuaSChEwooHW1FTVDoTDU1ey1Qowj+xaJj\n\trgjMCk+A+d1m+SgrZAPc4WcWVFYMQclb/wAeI4UEQEwCreypdX0YP/zfuiLZ2+SP1z1N\n\tUGJFw+bTUjAGXUq5hzaJeuVvSDKMoV6RpqZazrGhZ708HYqEr+HFLYyq/WP+YaYT+wZ8\n\t/mkR28NtVrTbefL5g9Q11PmxrYW7ADdlkuU42nqgVF5n1GkJZhuURtPujl43DBYFf5Gv\n\tT8Z6Siz1yfPsVPTCDHzOOT3TmGf3TBJqWA2cskPpXEuGhwv/rP3lY2YbUhCahYZEc9Io\n\toa6g==","X-Gm-Message-State":"AOAM532KMFOu/TOjxHo25svWiUPHv+D+4Y9V3VS9OvffG1IYmN9v3ocg\n\teKwH5l1arlx9XalSiTzi564nhWkmsbBeAA==","X-Google-Smtp-Source":"ABdhPJyqNloFDwwybdbv6h5wYU0PNNLGrNT3gk0NJsWV26WrJdCaEU73uN4VYXpgd0/3egbaI2LDGw==","X-Received":"by 2002:a05:651c:210f:: with SMTP id\n\ta15mr9441278ljq.160.1621678609703; \n\tSat, 22 May 2021 03:16:49 -0700 (PDT)","Date":"Sat, 22 May 2021 12:16:48 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado ","Message-ID":"","References":"<20210521133054.274502-1-nfraprado@collabora.com>\n\t<20210521133054.274502-5-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210521133054.274502-5-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com,\n\t=?iso-8859-1?q?Andr=E9?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17145,"web_url":"https://patchwork.libcamera.org/comment/17145/","msgid":"","date":"2021-05-23T22:01:30","subject":"Re: [libcamera-devel] [PATCH v4 4/5] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nícolas,\n\nThank you for the patch.\n\nOn Fri, May 21, 2021 at 10:30:53AM -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 \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 | 71 +++++++------\n> src/lc-compliance/meson.build | 4 +-\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 | 150 ++++++++++++++-------------\n> src/lc-compliance/tests.h | 16 ---\n> 8 files changed, 152 insertions(+), 294 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..37884ff70a69 100644\n> --- a/src/lc-compliance/main.cpp\n> +++ b/src/lc-compliance/main.cpp\n> @@ -9,10 +9,12 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> \n> +#include \"environment.h\"\n> #include \"../cam/options.h\"\n> -#include \"tests.h\"\n> \n> using namespace libcamera;\n> \n> @@ -21,21 +23,34 @@ 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\nThe { should go on the next line.\n\n> +\tvoid OnTestPartResult(const testing::TestPartResult& result) override {\n\nSame here. And s/& / &/ (as well as in another place below).\n\n> +\t\tif (result.type() == testing::TestPartResult::kFatalFailure\n> +\t\t || result.type() == testing::TestPartResult::kSkip) {\n\nAnd the || at the end of the previous line.\n\n> +\t\t\tthrow testing::AssertionException(result);\n> +\t\t}\n\nNo need for braces here.\n\n> +\t}\n> +};\n> +\n> class Harness\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 cm_;\n> -\tstd::shared_ptr camera_;\n> };\n> \n> Harness::Harness(const OptionsParser::Options &options)\n> @@ -46,35 +61,15 @@ 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> +\tEnvironment::instance()->destroy();\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;\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;\n> +\n> \tint ret = cm_->start();\n> \tif (ret) {\n> \t\tstd::cout << \"Failed to start camera manager: \"\n> @@ -89,23 +84,34 @@ 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> +\tif (camera->acquire()) {\n> \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tEnvironment::create(camera);\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\nIf we were to make it possible to select a camera through command line\narguments, how would that work ?\n\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 &cam : cm_->cameras())\n> @@ -143,6 +149,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 c287017575bd..ae68844ac9bd 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -13,16 +13,18 @@ lc_compliance_sources = files([\n> '../cam/event_loop.cpp',\n> '../cam/options.cpp',\n> 'main.cpp',\n> - 'results.cpp',\n> 'environment.cpp',\n> 'simple_capture.cpp',\n> 'single_stream.cpp',\n> ])\n> \n> +gtest_dep = dependency('gtest')\n\nI'd name this libgtest. The reason why libcamera_dep has a dep suffix is\nbecause libcamera is the shared library itself and libcamera_dep the\ndependency object. We may want to rename other dependencies with a dep\nsuffix, but for now, I'd stick to the existing naming scheme.\n\n> +\n> lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> dependencies : [\n> libatomic,\n> libcamera_dep,\n> libevent,\n> + gtest_dep,\n\nAlphabetical order please.\n\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 \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 \n> -#include \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;\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 f90fe6d0f9aa..7731eb16f8c2 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 \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> +\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> -\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> @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr 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> &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> @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> \tstd::vector> requests;\n> \tfor (const std::unique_ptr &buffer : buffers) {\n> \t\tstd::unique_ptr 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> @@ -121,12 +113,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> @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr 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> &buffers = allocator_->buffers(stream);\n> @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> \tstd::vector> requests;\n> \tfor (const std::unique_ptr &buffer : buffers) {\n> \t\tstd::unique_ptr 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> @@ -192,7 +174,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 \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 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 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 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..a8ae2f0e355b 100644\n> --- a/src/lc-compliance/single_stream.cpp\n> +++ b/src/lc-compliance/single_stream.cpp\n> @@ -7,91 +7,95 @@\n> \n> #include \n> \n> +#include \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,\n> -\t\t\t\t StreamRole role, unsigned int startCycles,\n> -\t\t\t\t unsigned int numRequests)\n> +const std::vector NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89};\n> +const std::vector ROLES = {Raw, StillCapture, VideoRecording, Viewfinder};\n> +\n> +class FixedRequestsTest :\n> +\tpublic testing::TestWithParam> {\n\nNo line break after :, but a line break before {\n\n> +};\n> +\n> +std::string nameParameters(const testing::TestParamInfo& info)\n\nCould this be a static function of the FixedRequestsTest class ?\n\n> {\n> +\tstd::map 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> +/*\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> +\tauto [role, numRequests] = GetParam();\n> +\tstd::shared_ptr camera = Environment::instance()->camera();\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> -\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.capture(numRequests);\n\nNot that it's required, but because the TEST_P macro creates a class\nthat inherits from FixedRequestsTest, you could also put some of the\ncode in the FixedRequestsTest class (either as helper functions, or even\nthe complete implementation of the tests).\n\nI'm also wondering if it the FixedRequestsTest class shouldn't be merged\nwith the SimpleCapture* classes, but that's a question for later I\nsuppose.\n\n> +};\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> -}\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> +\tauto [role, numRequests] = GetParam();\n> +\tstd::shared_ptr camera = Environment::instance()->camera();\n> +\tunsigned int numRepeats = 3;\n> +\n> +\tSimpleCaptureBalanced capture(camera);\n> \n> -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> -\t\t\t\t StreamRole role, unsigned int numRequests)\n> +\tcapture.configure(role);\n> +\n> +\tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> +\t\tcapture.capture(numRequests);\n> +};\n> +\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> +\tauto [role, numRequests] = GetParam();\n> +\tstd::shared_ptr camera = Environment::instance()->camera();\n> +\n> \tSimpleCaptureUnbalanced 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> -}\n> +\tcapture.capture(numRequests);\n> +};\n> \n> -Results testSingleStream(std::shared_ptr camera)\n> -{\n> -\tstatic const std::vector> 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 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> -}\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 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 \n> -\n> -#include \"results.h\"\n> -\n> -Results testSingleStream(std::shared_ptr camera);\n> -\n> -#endif /* __LC_COMPLIANCE_TESTS_H__ */","headers":{"Return-Path":"","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 3A97DC3201\n\tfor ;\n\tSun, 23 May 2021 22:01:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 546F468919;\n\tMon, 24 May 2021 00:01:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC729601AA\n\tfor ;\n\tMon, 24 May 2021 00:01:35 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2C08B476;\n\tMon, 24 May 2021 00:01:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mm4tLOlX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621807294;\n\tbh=Aggabm7D09rkOQvch2tJYIwKQiXwLUD4mjJLCrQPKf0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mm4tLOlXhj8hO5SDSU9HSgrBNJMV+3WloVdTIeZtrJex/ikKGRDgLR8+QjWkGxgmK\n\tpQT9ZfzwBDs+hdZIPeH3bCT9IZVjjpjLafXnKxzLz3+XwXbsaUaXengBZrIUkfWNlg\n\tROXYtc0a9ah/ldzfluOaOSUOmF3NiY45yR5ggDEI=","Date":"Mon, 24 May 2021 01:01:30 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210521133054.274502-1-nfraprado@collabora.com>\n\t<20210521133054.274502-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":"<20210521133054.274502-5-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17215,"web_url":"https://patchwork.libcamera.org/comment/17215/","msgid":"","date":"2021-05-24T17:58:25","subject":"Re: [libcamera-devel] [PATCH v4 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 Laurent,\n\nEm 2021-05-23 19:01, Laurent Pinchart escreveu:\n\n> Hi Nícolas,\n>\n> Thank you for the patch.\n>\n> On Fri, May 21, 2021 at 10:30:53AM -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 \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 | 71 +++++++------\n> > src/lc-compliance/meson.build | 4 +-\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 | 150 ++++++++++++++-------------\n> > src/lc-compliance/tests.h | 16 ---\n> > 8 files changed, 152 insertions(+), 294 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..37884ff70a69 100644\n> > --- a/src/lc-compliance/main.cpp\n> > +++ b/src/lc-compliance/main.cpp\n> > @@ -9,10 +9,12 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > +\n> > #include \n> > \n> > +#include \"environment.h\"\n> > #include \"../cam/options.h\"\n> > -#include \"tests.h\"\n> > \n> > using namespace libcamera;\n> > \n> > @@ -21,21 +23,34 @@ 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> The { should go on the next line.\n>\n> > +\tvoid OnTestPartResult(const testing::TestPartResult& result) override {\n>\n> Same here. And s/& / &/ (as well as in another place below).\n>\n> > +\t\tif (result.type() == testing::TestPartResult::kFatalFailure\n> > +\t\t || result.type() == testing::TestPartResult::kSkip) {\n>\n> And the || at the end of the previous line.\n>\n> > +\t\t\tthrow testing::AssertionException(result);\n> > +\t\t}\n>\n> No need for braces here.\n\nTurns out I wasn't using checkstyle.py, and having copied this code from Gtest's\ndocumentation, there were some style differences. I've addressed these and some\nothers pointed out by checkstyle.py. (Although there seem to be some\nfalse-positives which I'll ignore based on common sense).\n\n>\n> > +\t}\n> > +};\n> > +\n> > class Harness\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 cm_;\n> > -\tstd::shared_ptr camera_;\n> > };\n> > \n> > Harness::Harness(const OptionsParser::Options &options)\n> > @@ -46,35 +61,15 @@ 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> > +\tEnvironment::instance()->destroy();\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;\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;\n> > +\n> > \tint ret = cm_->start();\n> > \tif (ret) {\n> > \t\tstd::cout << \"Failed to start camera manager: \"\n> > @@ -89,23 +84,34 @@ 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> > +\tif (camera->acquire()) {\n> > \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > \t\treturn -EINVAL;\n> > \t}\n> > \n> > +\tEnvironment::create(camera);\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> If we were to make it possible to select a camera through command line\n> arguments, how would that work ?\n\nThat's already possible actually using the --camera flag that was already in\nplace. Gtest ignores flags that don't start with \"gtest_\". In any case, patch 5\nchanges some of these same lines to add the \"--list\" and \"--filter\" flags, and\nwhile at it, makes some changes so that only gtest flags are passed to gtest.\n\n>\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 &cam : cm_->cameras())\n> > @@ -143,6 +149,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 c287017575bd..ae68844ac9bd 100644\n> > --- a/src/lc-compliance/meson.build\n> > +++ b/src/lc-compliance/meson.build\n> > @@ -13,16 +13,18 @@ lc_compliance_sources = files([\n> > '../cam/event_loop.cpp',\n> > '../cam/options.cpp',\n> > 'main.cpp',\n> > - 'results.cpp',\n> > 'environment.cpp',\n> > 'simple_capture.cpp',\n> > 'single_stream.cpp',\n> > ])\n> > \n> > +gtest_dep = dependency('gtest')\n>\n> I'd name this libgtest. The reason why libcamera_dep has a dep suffix is\n> because libcamera is the shared library itself and libcamera_dep the\n> dependency object. We may want to rename other dependencies with a dep\n> suffix, but for now, I'd stick to the existing naming scheme.\n\nSounds good.\n\n>\n> > +\n> > lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > dependencies : [\n> > libatomic,\n> > libcamera_dep,\n> > libevent,\n> > + gtest_dep,\n>\n> Alphabetical order please.\n\nLuckily, changing it to libgtest makes it be in alphabetical order :P.\n\n>\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 \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 \n> > -#include \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;\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 f90fe6d0f9aa..7731eb16f8c2 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 \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> > +\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> > -\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> > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr 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> &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> > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > \tstd::vector> requests;\n> > \tfor (const std::unique_ptr &buffer : buffers) {\n> > \t\tstd::unique_ptr 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> > @@ -121,12 +113,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> > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr 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> &buffers = allocator_->buffers(stream);\n> > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> > \tstd::vector> requests;\n> > \tfor (const std::unique_ptr &buffer : buffers) {\n> > \t\tstd::unique_ptr 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> > @@ -192,7 +174,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 \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 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 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 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..a8ae2f0e355b 100644\n> > --- a/src/lc-compliance/single_stream.cpp\n> > +++ b/src/lc-compliance/single_stream.cpp\n> > @@ -7,91 +7,95 @@\n> > \n> > #include \n> > \n> > +#include \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,\n> > -\t\t\t\t StreamRole role, unsigned int startCycles,\n> > -\t\t\t\t unsigned int numRequests)\n> > +const std::vector NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89};\n> > +const std::vector ROLES = {Raw, StillCapture, VideoRecording, Viewfinder};\n> > +\n> > +class FixedRequestsTest :\n> > +\tpublic testing::TestWithParam> {\n>\n> No line break after :, but a line break before {\n>\n> > +};\n> > +\n> > +std::string nameParameters(const testing::TestParamInfo& info)\n>\n> Could this be a static function of the FixedRequestsTest class ?\n\nYes. Done.\n\n>\n> > {\n> > +\tstd::map 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> > +/*\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> > +\tauto [role, numRequests] = GetParam();\n> > +\tstd::shared_ptr camera = Environment::instance()->camera();\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> > -\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.capture(numRequests);\n>\n> Not that it's required, but because the TEST_P macro creates a class\n> that inherits from FixedRequestsTest, you could also put some of the\n> code in the FixedRequestsTest class (either as helper functions, or even\n> the complete implementation of the tests).\n\nI'm not sure the tests are similar or long enough yet for helper functions to be\nthat useful. But we certainly should have that in mind as more tests are added.\n\nThat said, in order to have the camera be acquired by each individual test as\nyou suggested in the review of patch 3, I'm already adding a SetUp() function\nin the base class to be used by all tests to acquire the camera automatically at\nstartup.\n\n>\n> I'm also wondering if it the FixedRequestsTest class shouldn't be merged\n> with the SimpleCapture* classes, but that's a question for later I\n> suppose.\n\nHm, my impression is that those functions are more generic and could be useful\nfor all test suites, so makes sense to be out in its own class, but I guess time\nwill tell.\n\nThanks,\nNícolas\n\n>\n> > +};\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> > -}\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> > +\tauto [role, numRequests] = GetParam();\n> > +\tstd::shared_ptr camera = Environment::instance()->camera();\n> > +\tunsigned int numRepeats = 3;\n> > +\n> > +\tSimpleCaptureBalanced capture(camera);\n> > \n> > -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> > -\t\t\t\t StreamRole role, unsigned int numRequests)\n> > +\tcapture.configure(role);\n> > +\n> > +\tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> > +\t\tcapture.capture(numRequests);\n> > +};\n> > +\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> > +\tauto [role, numRequests] = GetParam();\n> > +\tstd::shared_ptr camera = Environment::instance()->camera();\n> > +\n> > \tSimpleCaptureUnbalanced 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> > -}\n> > +\tcapture.capture(numRequests);\n> > +};\n> > \n> > -Results testSingleStream(std::shared_ptr camera)\n> > -{\n> > -\tstatic const std::vector> 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 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> > -}\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 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 \n> > -\n> > -#include \"results.h\"\n> > -\n> > -Results testSingleStream(std::shared_ptr camera);\n> > -\n> > -#endif /* __LC_COMPLIANCE_TESTS_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> --\n> To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.","headers":{"Return-Path":"","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 CB806C3200\n\tfor ;\n\tMon, 24 May 2021 17:59:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 094436891C;\n\tMon, 24 May 2021 19:59:08 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D05A0601AA\n\tfor ;\n\tMon, 24 May 2021 19:59:06 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nfraprado) with ESMTPSA id BEA8A1F41DB6"],"Mime-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=UTF-8","Date":"Mon, 24 May 2021 14:58:25 -0300","Message-Id":"","To":"\"Laurent Pinchart\" ","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","References":"<20210521133054.274502-1-nfraprado@collabora.com>\n\t<20210521133054.274502-5-nfraprado@collabora.com>\n\t","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]