[{"id":16961,"web_url":"https://patchwork.libcamera.org/comment/16961/","msgid":"","date":"2021-05-16T14:01:20","subject":"Re: [libcamera-devel] [PATCH v3 3/4] 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 work!\n\nOn 2021-05-14 10:16:51 -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 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 | 88 ++++++++++------\n> src/lc-compliance/meson.build | 3 +\n> src/lc-compliance/simple_capture.cpp | 74 +++++---------\n> src/lc-compliance/simple_capture.h | 8 +-\n> src/lc-compliance/single_stream.cpp | 148 ++++++++++++++-------------\n> src/lc-compliance/tests.h | 19 +++-\n> 6 files changed, 187 insertions(+), 153 deletions(-)\n> \n> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> index 54cee54aa978..d5ff93f514df 100644\n> --- a/src/lc-compliance/main.cpp\n> +++ b/src/lc-compliance/main.cpp\n> @@ -9,6 +9,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> \n> #include \"../cam/options.h\"\n> @@ -21,21 +23,43 @@ enum {\n> \tOptHelp = 'h',\n> };\n> \n> +bool Environment::create(std::shared_ptr camera)\n> +{\n> +\tif (instance_)\n> +\t\treturn false;\n> +\n> +\tcamera_ = camera;\n> +\tinstance_ = new Environment;\n> +\treturn true;\n> +}\n> +\n> +void Environment::destroy()\n> +{\n> +\tif (!camera_)\n> +\t\treturn;\n> +\n> +\tcamera_->release();\n> +\tcamera_.reset();\n> +}\n> +\n> +Environment *Environment::instance()\n> +{\n> +\treturn instance_;\n> +}\n\nI think this and the corresponding class declaration should be moved to \na separate environment.{cpp,h} file. It could possibly be added in a \nseparate patch just previous to this one.\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> \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 +70,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,18 +93,20 @@ 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> @@ -133,6 +139,23 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> \treturn 0;\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> +Environment *Environment::instance_ = nullptr;\n> +std::shared_ptr Environment::camera_ = nullptr;\n> +\n> int main(int argc, char **argv)\n> {\n> \tOptionsParser::Options options;\n> @@ -143,6 +166,11 @@ 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> +\t::testing::InitGoogleTest(&argc, argv);\n> +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> +\treturn RUN_ALL_TESTS();\n> }\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index a2bfcceb1259..704bc18af3e1 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -18,10 +18,13 @@ lc_compliance_sources = files([\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/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..0f8465083456 100644\n> --- a/src/lc-compliance/simple_capture.h\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -17,13 +17,13 @@\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 +40,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 +56,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..aad32ecd051e 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 \"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, \"still\"},\n> +\t\t\t\t\t\t {VideoRecording, \"video\"},\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\nAre there some guidelines or recommendations around gtest to these \nnames? I really like that you make it possible to name each one in a \nmore human readable format. While testing this series I noticed tests \nlike BalancedMultiCycle/video2 and incorrectly thought \"This is testing \nthe V4L2 device /dev/video2\" not this is testing the video recording \nrole with 2 requests.\n\nWould it make sens to name it BalancedMultiCycle/VideoRecording/2 ?\n\nI fear this could turn into a bikeshedding discussion and we can always \nwork it on-top. But if others have ideas about naming please speak up. I \nthink for example in the CrOS test suite uses '.' as separator instead \nof '/'? So maybe there are some best-practices to be found.\n\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> index 396605214e4b..076785a7185f 100644\n> --- a/src/lc-compliance/tests.h\n> +++ b/src/lc-compliance/tests.h\n> @@ -11,6 +11,23 @@\n> \n> #include \"results.h\"\n> \n> -Results testSingleStream(std::shared_ptr camera);\n> +using namespace libcamera;\n> +\n> +class Environment\n> +{\n> +public:\n> +\tstatic bool create(std::shared_ptr camera);\n> +\tvoid destroy();\n> +\n> +\tstatic Environment *instance();\n> +\n> +\tstd::shared_ptr camera() const { return camera_; }\n> +\n> +private:\n> +\tEnvironment() {};\n> +\n> +\tstatic Environment *instance_;\n> +\tstatic std::shared_ptr camera_;\n> +};\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 8C2E5C31FB\n\tfor ;\n\tSun, 16 May 2021 14:01:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E15F76891D;\n\tSun, 16 May 2021 16:01:25 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19DC768911\n\tfor ;\n\tSun, 16 May 2021 16:01:23 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id r5so4965601lfr.5\n\tfor ;\n\tSun, 16 May 2021 07:01:23 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv8sm2521182ljj.137.2021.05.16.07.01.20\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 16 May 2021 07:01:21 -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=\"Njoergh6\"; 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=ujKcup0aI58ZYf+PeIXjk98U9OJOwspWhqrtA+NjCMA=;\n\tb=Njoergh6ATymtxjhqLl/zTV0MR4fou12F2s6L44zbRtrQFiT80jp5NS4Z2FHQwDUZf\n\tQlVk9mMpKNGT07JsMm5U2UQuzXeu7NUDdef+/2Dp8kxEVcQ76veyzmey0I/n7TrI1C0Y\n\tI+47EZBdXMsU1Tjdxd+NLrKFTd4pOC2zrtoOlZbdtwDXTWKNKjJ8fL2KvkCN4BcWFBeG\n\tgV5k/3hox9L1o9T8463jv5kU2TFqMqDcopOu1nAZ399RBlRLgjMryyhvcrat0JPAg0SC\n\tcQffeWRzmlrljpQFM63O5LnpzlEAFEfQoE0EfLj6aoWMoNmktmseLYJTEw1QUrFRr7IL\n\t0RsQ==","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=ujKcup0aI58ZYf+PeIXjk98U9OJOwspWhqrtA+NjCMA=;\n\tb=KLPx6xjbT/zvfyrWC+3XjX1QMyUx3bPhU+ubtctw5Rs+jATzmtEyLRom+7BqquPi8E\n\tONau+gkXX4VrTCmlU9XSlU413/LBJTIcxlyTX4yGbvhDCudW6oOaknYB3qXTJuNxXvlM\n\tdZWEWTBgpcOE401QPZxjUWQWNkTHufNFSqkZh2EKGzch6/Q8RQBC3dMyP+9wAIhWU8HS\n\t1Esw6hnXh/AJwAU0rWnGNGWkQUAWuUTe+r6PK/MHoa0XMtyUrHHtJKIVEmdwZMI7FVjl\n\tpUi/K1UT1Ufk0YbQx7S2GtK3Iq/DtQ3pzNKIGDENVIeXN0d9cpwP1sE95xDx13cH8X/Y\n\tfnFg==","X-Gm-Message-State":"AOAM530rgYAb42QJQ61MpwYrairZdEgY6tsb0TIlaqjN2TAMK3JkHo6L\n\t6VvQ0dXXTKnFx4YicHPlBzLBO4bOrpBREbOx","X-Google-Smtp-Source":"ABdhPJwSIGGlgEx92jAUO6Gvr9rrWz4oILYw8IiL2D9vbWTu6ZTYrWzSF8c9mJqlOJAzslBmLZ3SnQ==","X-Received":"by 2002:ac2:538e:: with SMTP id\n\tg14mr37262271lfh.334.1621173682816; \n\tSun, 16 May 2021 07:01:22 -0700 (PDT)","Date":"Sun, 16 May 2021 16:01:20 +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":"<20210514131652.345486-1-nfraprado@collabora.com>\n\t<20210514131652.345486-4-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":"<20210514131652.345486-4-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] 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":17049,"web_url":"https://patchwork.libcamera.org/comment/17049/","msgid":"","date":"2021-05-20T15:17:17","subject":"Re: [libcamera-devel] [PATCH v3 3/4] 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 Niklas,\n\nthank you for the feedback.\n\nEm 2021-05-16 11:01, Niklas Söderlund escreveu:\n\n> Hi Nícolas,\n>\n> Thanks for your work!\n>\n> On 2021-05-14 10:16:51 -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 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 | 88 ++++++++++------\n> > src/lc-compliance/meson.build | 3 +\n> > src/lc-compliance/simple_capture.cpp | 74 +++++---------\n> > src/lc-compliance/simple_capture.h | 8 +-\n> > src/lc-compliance/single_stream.cpp | 148 ++++++++++++++-------------\n> > src/lc-compliance/tests.h | 19 +++-\n> > 6 files changed, 187 insertions(+), 153 deletions(-)\n> > \n> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > index 54cee54aa978..d5ff93f514df 100644\n> > --- a/src/lc-compliance/main.cpp\n> > +++ b/src/lc-compliance/main.cpp\n> > @@ -9,6 +9,8 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > +\n> > #include \n> > \n> > #include \"../cam/options.h\"\n> > @@ -21,21 +23,43 @@ enum {\n> > \tOptHelp = 'h',\n> > };\n> > \n> > +bool Environment::create(std::shared_ptr camera)\n> > +{\n> > +\tif (instance_)\n> > +\t\treturn false;\n> > +\n> > +\tcamera_ = camera;\n> > +\tinstance_ = new Environment;\n> > +\treturn true;\n> > +}\n> > +\n> > +void Environment::destroy()\n> > +{\n> > +\tif (!camera_)\n> > +\t\treturn;\n> > +\n> > +\tcamera_->release();\n> > +\tcamera_.reset();\n> > +}\n> > +\n> > +Environment *Environment::instance()\n> > +{\n> > +\treturn instance_;\n> > +}\n>\n> I think this and the corresponding class declaration should be moved to\n> a separate environment.{cpp,h} file. It could possibly be added in a\n> separate patch just previous to this one.\n\nYep, that makes sense. Will do for v4.\n\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> > \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 +70,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,18 +93,20 @@ 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> > @@ -133,6 +139,23 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> > \treturn 0;\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> > +Environment *Environment::instance_ = nullptr;\n> > +std::shared_ptr Environment::camera_ = nullptr;\n> > +\n> > int main(int argc, char **argv)\n> > {\n> > \tOptionsParser::Options options;\n> > @@ -143,6 +166,11 @@ 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> > +\t::testing::InitGoogleTest(&argc, argv);\n> > +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> > +\treturn RUN_ALL_TESTS();\n> > }\n> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> > index a2bfcceb1259..704bc18af3e1 100644\n> > --- a/src/lc-compliance/meson.build\n> > +++ b/src/lc-compliance/meson.build\n> > @@ -18,10 +18,13 @@ lc_compliance_sources = files([\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/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..0f8465083456 100644\n> > --- a/src/lc-compliance/simple_capture.h\n> > +++ b/src/lc-compliance/simple_capture.h\n> > @@ -17,13 +17,13 @@\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 +40,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 +56,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..aad32ecd051e 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 \"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, \"still\"},\n> > +\t\t\t\t\t\t {VideoRecording, \"video\"},\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> Are there some guidelines or recommendations around gtest to these\n> names? I really like that you make it possible to name each one in a\n> more human readable format. While testing this series I noticed tests\n> like BalancedMultiCycle/video2 and incorrectly thought \"This is testing\n> the V4L2 device /dev/video2\" not this is testing the video recording\n> role with 2 requests.\n\nThere aren't any guidelines, but there are big restrictions on the names.\nNamely, they should be valid C++ identifiers.\n\nTake this for example:\n\n\tRolesAndRequests/FixedRequestsTest.BalancedSingleCycle/raw1\n\n\"RolesAndRequests\" is the instantiation name (since different instantiations of\nthe same test suite using different parameter ranges are possible). It is\nappended with a \"/\".\n\n\"FixedRequestsTest\" is the test suite. It is appended with a \".\".\n\n\"BalancedSingleCycle\" is the test case. It is appended with a \"/\".\n\n\"raw1\" is the custom string I returned to represent the parameters for this\nparticular parametrized test case.\n\nThe complete test name reported is all of those joined with those special\ncharacters in-between. But internally they form identifiers, so each one should\nbe a valid identifier.\n\n>\n> Would it make sens to name it BalancedMultiCycle/VideoRecording/2 ?\n\nWhich means this isn't possible unfortunately. We could however use '_' to\nseparate the parameter values:\n\n\tVideoRecording_2\n\nto get:\n\n\tRolesAndRequests/FixedRequestsTest.BalancedSingleCycle/VideoRecording_2\n\nThanks,\nNícolas\n\n>\n> I fear this could turn into a bikeshedding discussion and we can always\n> work it on-top. But if others have ideas about naming please speak up. I\n> think for example in the CrOS test suite uses '.' as separator instead\n> of '/'? So maybe there are some best-practices to be found.\n>\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> > index 396605214e4b..076785a7185f 100644\n> > --- a/src/lc-compliance/tests.h\n> > +++ b/src/lc-compliance/tests.h\n> > @@ -11,6 +11,23 @@\n> > \n> > #include \"results.h\"\n> > \n> > -Results testSingleStream(std::shared_ptr camera);\n> > +using namespace libcamera;\n> > +\n> > +class Environment\n> > +{\n> > +public:\n> > +\tstatic bool create(std::shared_ptr camera);\n> > +\tvoid destroy();\n> > +\n> > +\tstatic Environment *instance();\n> > +\n> > +\tstd::shared_ptr camera() const { return camera_; }\n> > +\n> > +private:\n> > +\tEnvironment() {};\n> > +\n> > +\tstatic Environment *instance_;\n> > +\tstatic std::shared_ptr camera_;\n> > +};\n> > \n> > #endif /* __LC_COMPLIANCE_TESTS_H__ */\n> > -- \n> > 2.31.1\n> > \n>\n> --\n> Regards,\n> Niklas Söderlund","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 8DE08C31FF\n\tfor ;\n\tThu, 20 May 2021 15:17:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B6C568918;\n\tThu, 20 May 2021 17:17:58 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A04EB602B2\n\tfor ;\n\tThu, 20 May 2021 17:17:56 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:d269:289c:2421:e513])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id D39431F43D02;\n\tThu, 20 May 2021 16:17:54 +0100 (BST)"],"Mime-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=UTF-8","Date":"Thu, 20 May 2021 12:17:17 -0300","Message-Id":"","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","References":"<20210514131652.345486-1-nfraprado@collabora.com>\n\t<20210514131652.345486-4-nfraprado@collabora.com>\n\t","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] 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\" "}}]