[{"id":17820,"web_url":"https://patchwork.libcamera.org/comment/17820/","msgid":"<20210626081231.cgruwdgdwhtkdqbn@uno.localdomain>","date":"2021-06-26T08:12:31","subject":"Re: [libcamera-devel] [PATCH v9 4/5] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas,\n\n first of all: this is a great job! The introduction of\n lc-compliance in kernel-ci is an amazing step forward, thanks a lot\n for getting to this point so quickly!\n\nOn Fri, Jun 25, 2021 at 04:39:24PM -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> Reviewed-by: Niklas Söderlund \n> ---\n> Changes in v9:\n> - Thanks to Jacopo:\n> - Removed the Harness class, substituting methods with static helper functions\n> - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()\n> - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()\n> - Changed tests naming:\n> - TestSuite instance: RolesAndRequests -> CaptureTests\n> - TestSuite: FixedRequestsTest -> SingleStream\n> - TestCase: BalancedSingleCycle -> Capture\n> - TestCase: BalancedMultiCycle -> CaptureStartStop\n> - TestCase: Unbalanced -> UnbalancedStop\n> - Renamed single_stream.cpp to capture_test.cpp\n>\n> Changes in v8:\n> - Thanks to Laurent:\n> - Fixed lc-compliance's meson.build to disable when gtest is not installed\n> - Fixed compiling error in Clang\n>\n> Changes in v7:\n> - Thanks to Jacopo:\n> - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n>\n> No changes in v6\n>\n> Changes in v5:\n> - Thanks to Laurent:\n> - Fixed style issues\n> - Added SetUp and TearDown functions to acquire and release the camera for\n> each test\n>\n> Changes in v4:\n> - Removed old lc-compliance results classes\n> - Thanks to Niklas:\n> - Improved naming of tests\n>\n> Changes in v3:\n> - Thanks to Niklas:\n> - Went back to static test registration, and created a Singleton Environment\n> class to store the camera global variable\n> - Added a nameParameters() function to give meaningful names to the test\n> parameters, removing the need to register each test suite for every role\n>\n> src/lc-compliance/capture_test.cpp | 127 +++++++++++++++++++++++++++\n> src/lc-compliance/main.cpp | 106 +++++++++-------------\n> src/lc-compliance/meson.build | 7 +-\n> src/lc-compliance/results.cpp | 75 ----------------\n> src/lc-compliance/results.h | 47 ----------\n> src/lc-compliance/simple_capture.cpp | 77 +++++++---------\n> src/lc-compliance/simple_capture.h | 9 +-\n> src/lc-compliance/single_stream.cpp | 97 --------------------\n> src/lc-compliance/tests.h | 16 ----\n> 9 files changed, 206 insertions(+), 355 deletions(-)\n> create mode 100644 src/lc-compliance/capture_test.cpp\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/single_stream.cpp\n> delete mode 100644 src/lc-compliance/tests.h\n>\n> diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp\n> new file mode 100644\n> index 000000000000..116ae496fe04\n> --- /dev/null\n> +++ b/src/lc-compliance/capture_test.cpp\n> @@ -0,0 +1,127 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n\nAaaah, time flies! Could you check the year for the other files too ?\n\n> + *\n> + * single_stream.cpp - Test a single camera stream\n> + */\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"environment.h\"\n> +#include \"simple_capture.h\"\n\nWe tend to include the file header first to make sure it's\nself-contained.\n\n> +\n> +using namespace libcamera;\n> +\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 SingleStream : public testing::TestWithParam>\n> +{\n> +public:\n> +\tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> +\n> +protected:\n> +\tvoid SetUp() override;\n> +\tvoid TearDown() override;\n> +\n> +\tstd::shared_ptr camera_;\n> +};\n> +\n> +/*\n> + * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> + * in order to be able to assert on them.\n> + */\n> +void SingleStream::SetUp()\n> +{\n> +\tEnvironment *env = Environment::get();\n> +\n> +\tcamera_ = env->cm()->get(env->cameraId());\n> +\n> +\tASSERT_EQ(camera_->acquire(), 0);\n> +}\n> +\n> +void SingleStream::TearDown()\n> +{\n> +\tif (!camera_)\n> +\t\treturn;\n> +\n> +\tcamera_->release();\n> +\tcamera_.reset();\n> +}\n> +\n> +std::string SingleStream::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> +\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\nI'm not sure I get the last statement (probably I had the same comment on the\nprevious version :) is \"M\" the queue depth ? Honestly I would drop it.\n\n> + */\n> +TEST_P(SingleStream, Capture)\n> +{\n> +\tauto [role, numRequests] = GetParam();\n> +\n> +\tSimpleCaptureBalanced capture(camera_);\n> +\n> +\tcapture.configure(role);\n> +\n> +\tcapture.capture(numRequests);\n> +}\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(SingleStream, CaptureStartStop)\n> +{\n> +\tauto [role, numRequests] = GetParam();\n> +\tunsigned int numRepeats = 3;\n> +\n> +\tSimpleCaptureBalanced capture(camera_);\n> +\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(SingleStream, UnbalancedStop)\n> +{\n> +\tauto [role, numRequests] = GetParam();\n> +\n> +\tSimpleCaptureUnbalanced capture(camera_);\n> +\n> +\tcapture.configure(role);\n> +\n> +\tcapture.capture(numRequests);\n> +}\n> +\n> +INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> +\t\t\t SingleStream,\n> +\t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> +\t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> +\t\t\t SingleStream::nameParameters);\n> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {\n> \tOptHelp = 'h',\n> };\n>\n> -class Harness\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> -public:\n> -\tHarness(const OptionsParser::Options &options);\n> -\t~Harness();\n> -\n> -\tint exec();\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> +\tvoid OnTestPartResult(const testing::TestPartResult &result) override\n> +\t{\n> +\t\tif (result.type() == testing::TestPartResult::kFatalFailure ||\n> +\t\t result.type() == testing::TestPartResult::kSkip)\n> +\t\t\tthrow testing::AssertionException(result);\n> +\t}\n> };\n>\n> -Harness::Harness(const OptionsParser::Options &options)\n> -\t: options_(options)\n> -{\n> -\tcm_ = std::make_unique();\n> -}\n> -\n> -Harness::~Harness()\n> +static void listCameras(CameraManager *cm)\n> {\n> -\tif (camera_) {\n> -\t\tcamera_->release();\n> -\t\tcamera_.reset();\n> -\t}\n> -\n> -\tcm_->stop();\n> +\tfor (const std::shared_ptr &cam : cm->cameras())\n> +\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> }\n>\n> -int Harness::exec()\n> +static int initCamera(CameraManager *cm, OptionsParser::Options options)\n> {\n> -\tint ret = init();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tstd::vector results;\n> +\tstd::shared_ptr camera;\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> -\tint ret = cm_->start();\n> +\tint ret = cm->start();\n> \tif (ret) {\n> \t\tstd::cout << \"Failed to start camera manager: \"\n> \t\t\t << strerror(-ret) << std::endl;\n> \t\treturn ret;\n> \t}\n>\n> -\tif (!options_.isSet(OptCamera)) {\n> +\tif (!options.isSet(OptCamera)) {\n> \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> -\t\tlistCameras();\n> +\t\tlistCameras(cm);\n> \t\treturn -ENODEV;\n> \t}\n>\n> -\tconst std::string &cameraId = options_[OptCamera];\n> -\tcamera_ = cm_->get(cameraId);\n> -\tif (!camera_) {\n> +\tconst std::string &cameraId = options[OptCamera];\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\tlistCameras(cm);\n> \t\treturn -ENODEV;\n> \t}\n>\n> -\tif (camera_->acquire()) {\n> -\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tEnvironment::get()->setup(cm, cameraId);\n>\n> \tstd::cout << \"Using camera \" << cameraId << std::endl;\n>\n> \treturn 0;\n> }\n>\n> -void Harness::listCameras()\n> -{\n> -\tfor (const std::shared_ptr &cam : cm_->cameras())\n> -\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> -}\n> -\n> static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> {\n> \tOptionsParser parser;\n> @@ -142,7 +106,17 @@ int main(int argc, char **argv)\n> \tif (ret < 0)\n> \t\treturn EXIT_FAILURE;\n>\n> -\tHarness harness(options);\n> +\tstd::unique_ptr cm = std::make_unique();\n> +\n> +\tret = initCamera(cm.get(), options);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> +\n> +\tret = RUN_ALL_TESTS();\n> +\n> +\tcm->stop();\n>\n> -\treturn harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;\n> +\treturn ret;\n> }\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index 6dd49085569f..f3a79ae438a9 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -1,8 +1,9 @@\n> # SPDX-License-Identifier: CC0-1.0\n>\n> libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))\n> +libgtest = dependency('gtest', required : get_option('lc-compliance'))\n>\n> -if not libevent.found()\n> +if not (libevent.found() and libgtest.found())\n> lc_compliance_enabled = false\n> subdir_done()\n> endif\n> @@ -14,9 +15,8 @@ lc_compliance_sources = files([\n> '../cam/options.cpp',\n> 'environment.cpp',\n> 'main.cpp',\n> - 'results.cpp',\n> 'simple_capture.cpp',\n> - 'single_stream.cpp',\n> + 'capture_test.cpp',\n> ])\n>\n> lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> @@ -24,5 +24,6 @@ lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> libatomic,\n> libcamera_dep,\n> libevent,\n> + libgtest,\n\nAfter the recent split of libcamera-base this does not apply anymore.\nRebase is trivial, but much easier for you since you have a branch.\nI'm afraid this would require a v10 to make things simpler on our side\n(fixing this is trivial, but I have conflicts also when applying the\nnext patch and that would require manually applying the diff :(\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 bfc91b26444e..ce0883672709 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,37 @@ 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> +\tint count = allocator_->allocate(stream);\n>\n> -\tif (camera_->start())\n> -\t\treturn { Results::Fail, \"Failed to start camera\" };\n> +\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> +\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n>\n> -\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> +\tASSERT_TRUE(!camera_->start()) << \"Failed to start camera\";\n>\n> -\treturn { Results::Pass, \"Started camera\" };\n> +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n\nShouldn't the signal be connected before starting the camera ?\n\n> }\n>\n> void SimpleCapture::stop()\n> @@ -74,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> @@ -100,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\nASSERT_EQ(.., 0)\n\n>\n> -\t\tif (queueRequest(request.get()) < 0)\n> -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> +\t\tASSERT_GE(queueRequest(request.get()), 0) << \"Failed to queue request\";\n>\n> \t\trequests.push_back(std::move(request));\n> \t}\n> @@ -118,12 +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> @@ -155,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> @@ -171,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\nmaybe it's just me being disappointed by the usage of FALSE() to test the\nresuls of a function is 0 ? If that's the case just ignore the\ncomments on this topic :)\n\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\nASSERT_EQ() ?\n\nAll minors, I like this new setup and the naming scheme (at least I\nagree with myself time to time :) I'm sure we'll refactor later as you\nsuggested but for now that's fine.\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n>\n> \t\trequests.push_back(std::move(request));\n> \t}\n> @@ -189,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> deleted file mode 100644\n> index 8318b42f42d6..000000000000\n> --- a/src/lc-compliance/single_stream.cpp\n> +++ /dev/null\n> @@ -1,97 +0,0 @@\n> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * single_stream.cpp - Test a single camera stream\n> - */\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> -{\n> -\tSimpleCaptureBalanced capture(camera);\n> -\n> -\tResults::Result ret = capture.configure(role);\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\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> -\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> -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> -\t\t\t\t StreamRole role, unsigned int numRequests)\n> -{\n> -\tSimpleCaptureUnbalanced capture(camera);\n> -\n> -\tResults::Result ret = capture.configure(role);\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> -\n> -\treturn capture.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> 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.32.0\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 2974FC321A\n\tfor ;\n\tSat, 26 Jun 2021 08:11:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 687AE684D4;\n\tSat, 26 Jun 2021 10:11:46 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F5A6684D2\n\tfor ;\n\tSat, 26 Jun 2021 10:11:44 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id F1AA4FF804;\n\tSat, 26 Jun 2021 08:11:42 +0000 (UTC)"],"Date":"Sat, 26 Jun 2021 10:12:31 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210626081231.cgruwdgdwhtkdqbn@uno.localdomain>","References":"<20210625193925.517406-1-nfraprado@collabora.com>\n\t<20210625193925.517406-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":"<20210625193925.517406-5-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v9 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":17877,"web_url":"https://patchwork.libcamera.org/comment/17877/","msgid":"<20210628082804.GZ2599@pyrite.rasen.tech>","date":"2021-06-28T08:28:04","subject":"Re: [libcamera-devel] [PATCH v9 4/5] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Fri, Jun 25, 2021 at 04:39:24PM -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> Reviewed-by: Niklas Söderlund \n> ---\n> Changes in v9:\n> - Thanks to Jacopo:\n> - Removed the Harness class, substituting methods with static helper functions\n> - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()\n> - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()\n> - Changed tests naming:\n> - TestSuite instance: RolesAndRequests -> CaptureTests\n> - TestSuite: FixedRequestsTest -> SingleStream\n> - TestCase: BalancedSingleCycle -> Capture\n> - TestCase: BalancedMultiCycle -> CaptureStartStop\n> - TestCase: Unbalanced -> UnbalancedStop\n> - Renamed single_stream.cpp to capture_test.cpp\n> \n> Changes in v8:\n> - Thanks to Laurent:\n> - Fixed lc-compliance's meson.build to disable when gtest is not installed\n> - Fixed compiling error in Clang\n> \n> Changes in v7:\n> - Thanks to Jacopo:\n> - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n> \n> No changes in v6\n> \n> Changes in v5:\n> - Thanks to Laurent:\n> - Fixed style issues\n> - Added SetUp and TearDown functions to acquire and release the camera for\n> each test\n> \n> Changes in v4:\n> - Removed old lc-compliance results classes\n> - Thanks to Niklas:\n> - Improved naming of tests\n> \n> Changes in v3:\n> - Thanks to Niklas:\n> - Went back to static test registration, and created a Singleton Environment\n> class to store the camera global variable\n> - Added a nameParameters() function to give meaningful names to the test\n> parameters, removing the need to register each test suite for every role\n> \n> src/lc-compliance/capture_test.cpp | 127 +++++++++++++++++++++++++++\n> src/lc-compliance/main.cpp | 106 +++++++++-------------\n> src/lc-compliance/meson.build | 7 +-\n> src/lc-compliance/results.cpp | 75 ----------------\n> src/lc-compliance/results.h | 47 ----------\n> src/lc-compliance/simple_capture.cpp | 77 +++++++---------\n> src/lc-compliance/simple_capture.h | 9 +-\n> src/lc-compliance/single_stream.cpp | 97 --------------------\n> src/lc-compliance/tests.h | 16 ----\n> 9 files changed, 206 insertions(+), 355 deletions(-)\n> create mode 100644 src/lc-compliance/capture_test.cpp\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/single_stream.cpp\n> delete mode 100644 src/lc-compliance/tests.h\n> \n> diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp\n> new file mode 100644\n> index 000000000000..116ae496fe04\n> --- /dev/null\n> +++ b/src/lc-compliance/capture_test.cpp\n> @@ -0,0 +1,127 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * single_stream.cpp - Test a single camera stream\n> + */\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"environment.h\"\n> +#include \"simple_capture.h\"\n> +\n> +using namespace libcamera;\n> +\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 SingleStream : public testing::TestWithParam>\n> +{\n> +public:\n> +\tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> +\n> +protected:\n> +\tvoid SetUp() override;\n> +\tvoid TearDown() override;\n> +\n> +\tstd::shared_ptr camera_;\n> +};\n> +\n> +/*\n> + * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> + * in order to be able to assert on them.\n> + */\n> +void SingleStream::SetUp()\n> +{\n> +\tEnvironment *env = Environment::get();\n> +\n> +\tcamera_ = env->cm()->get(env->cameraId());\n> +\n> +\tASSERT_EQ(camera_->acquire(), 0);\n> +}\n> +\n> +void SingleStream::TearDown()\n> +{\n> +\tif (!camera_)\n> +\t\treturn;\n> +\n> +\tcamera_->release();\n> +\tcamera_.reset();\n> +}\n> +\n> +std::string SingleStream::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(SingleStream, Capture)\n> +{\n> +\tauto [role, numRequests] = GetParam();\n> +\n> +\tSimpleCaptureBalanced capture(camera_);\n> +\n> +\tcapture.configure(role);\n> +\n> +\tcapture.capture(numRequests);\n> +}\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(SingleStream, CaptureStartStop)\n> +{\n> +\tauto [role, numRequests] = GetParam();\n> +\tunsigned int numRepeats = 3;\n> +\n> +\tSimpleCaptureBalanced capture(camera_);\n> +\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(SingleStream, UnbalancedStop)\n> +{\n> +\tauto [role, numRequests] = GetParam();\n> +\n> +\tSimpleCaptureUnbalanced capture(camera_);\n> +\n> +\tcapture.configure(role);\n> +\n> +\tcapture.capture(numRequests);\n> +}\n> +\n> +INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> +\t\t\t SingleStream,\n> +\t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> +\t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> +\t\t\t SingleStream::nameParameters);\n> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {\n> \tOptHelp = 'h',\n> };\n> \n> -class Harness\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> -public:\n> -\tHarness(const OptionsParser::Options &options);\n> -\t~Harness();\n> -\n> -\tint exec();\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> +\tvoid OnTestPartResult(const testing::TestPartResult &result) override\n> +\t{\n> +\t\tif (result.type() == testing::TestPartResult::kFatalFailure ||\n> +\t\t result.type() == testing::TestPartResult::kSkip)\n> +\t\t\tthrow testing::AssertionException(result);\n\nThis fails compilation in cros_sdk, as exceptions are disabled. The\nbehavior can be reproduced outside of the cros_sdk by adding\n-fno-exceptions and compiling on clang.\n\nAdding this fixes the issue:\n\ndiff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\nindex 0fd2aca1..aa5852f6 100644\n--- a/src/lc-compliance/meson.build\n+++ b/src/lc-compliance/meson.build\n@@ -20,6 +20,7 @@ lc_compliance_sources = files([\n ])\n \n lc_compliance = executable('lc-compliance', lc_compliance_sources,\n+ cpp_args : [ '-fexceptions' ],\n dependencies : [\n libatomic,\n libcamera_public,\n\n\nPaul\n\n> +\t}\n> };\n> \n> -Harness::Harness(const OptionsParser::Options &options)\n> -\t: options_(options)\n> -{\n> -\tcm_ = std::make_unique();\n> -}\n> -\n> -Harness::~Harness()\n> +static void listCameras(CameraManager *cm)\n> {\n> -\tif (camera_) {\n> -\t\tcamera_->release();\n> -\t\tcamera_.reset();\n> -\t}\n> -\n> -\tcm_->stop();\n> +\tfor (const std::shared_ptr &cam : cm->cameras())\n> +\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> }\n> \n> -int Harness::exec()\n> +static int initCamera(CameraManager *cm, OptionsParser::Options options)\n> {\n> -\tint ret = init();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tstd::vector results;\n> +\tstd::shared_ptr camera;\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> -\tint ret = cm_->start();\n> +\tint ret = cm->start();\n> \tif (ret) {\n> \t\tstd::cout << \"Failed to start camera manager: \"\n> \t\t\t << strerror(-ret) << std::endl;\n> \t\treturn ret;\n> \t}\n> \n> -\tif (!options_.isSet(OptCamera)) {\n> +\tif (!options.isSet(OptCamera)) {\n> \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> -\t\tlistCameras();\n> +\t\tlistCameras(cm);\n> \t\treturn -ENODEV;\n> \t}\n> \n> -\tconst std::string &cameraId = options_[OptCamera];\n> -\tcamera_ = cm_->get(cameraId);\n> -\tif (!camera_) {\n> +\tconst std::string &cameraId = options[OptCamera];\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\tlistCameras(cm);\n> \t\treturn -ENODEV;\n> \t}\n> \n> -\tif (camera_->acquire()) {\n> -\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tEnvironment::get()->setup(cm, cameraId);\n> \n> \tstd::cout << \"Using camera \" << cameraId << std::endl;\n> \n> \treturn 0;\n> }\n> \n> -void Harness::listCameras()\n> -{\n> -\tfor (const std::shared_ptr &cam : cm_->cameras())\n> -\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> -}\n> -\n> static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> {\n> \tOptionsParser parser;\n> @@ -142,7 +106,17 @@ int main(int argc, char **argv)\n> \tif (ret < 0)\n> \t\treturn EXIT_FAILURE;\n> \n> -\tHarness harness(options);\n> +\tstd::unique_ptr cm = std::make_unique();\n> +\n> +\tret = initCamera(cm.get(), options);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> +\n> +\tret = RUN_ALL_TESTS();\n> +\n> +\tcm->stop();\n> \n> -\treturn harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;\n> +\treturn ret;\n> }\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index 6dd49085569f..f3a79ae438a9 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -1,8 +1,9 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))\n> +libgtest = dependency('gtest', required : get_option('lc-compliance'))\n> \n> -if not libevent.found()\n> +if not (libevent.found() and libgtest.found())\n> lc_compliance_enabled = false\n> subdir_done()\n> endif\n> @@ -14,9 +15,8 @@ lc_compliance_sources = files([\n> '../cam/options.cpp',\n> 'environment.cpp',\n> 'main.cpp',\n> - 'results.cpp',\n> 'simple_capture.cpp',\n> - 'single_stream.cpp',\n> + 'capture_test.cpp',\n> ])\n> \n> lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> @@ -24,5 +24,6 @@ lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> libatomic,\n> libcamera_dep,\n> libevent,\n> + libgtest,\n> ],\n> install : true)\n> diff --git a/src/lc-compliance/results.cpp b/src/lc-compliance/results.cpp\n> deleted file mode 100644\n> index f149f7859e28..000000000000\n> --- a/src/lc-compliance/results.cpp\n> +++ /dev/null\n> @@ -1,75 +0,0 @@\n> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * results.cpp - Test result aggregator\n> - */\n> -\n> -#include \"results.h\"\n> -\n> -#include \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 bfc91b26444e..ce0883672709 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,37 @@ 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> +\tint count = allocator_->allocate(stream);\n> \n> -\tif (camera_->start())\n> -\t\treturn { Results::Fail, \"Failed to start camera\" };\n> +\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> +\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> \n> -\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> +\tASSERT_TRUE(!camera_->start()) << \"Failed to start camera\";\n> \n> -\treturn { Results::Pass, \"Started camera\" };\n> +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> }\n> \n> void SimpleCapture::stop()\n> @@ -74,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> @@ -100,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> @@ -118,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> @@ -155,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> @@ -171,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> @@ -189,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> deleted file mode 100644\n> index 8318b42f42d6..000000000000\n> --- a/src/lc-compliance/single_stream.cpp\n> +++ /dev/null\n> @@ -1,97 +0,0 @@\n> -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * single_stream.cpp - Test a single camera stream\n> - */\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> -{\n> -\tSimpleCaptureBalanced capture(camera);\n> -\n> -\tResults::Result ret = capture.configure(role);\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\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> -\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> -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> -\t\t\t\t StreamRole role, unsigned int numRequests)\n> -{\n> -\tSimpleCaptureUnbalanced capture(camera);\n> -\n> -\tResults::Result ret = capture.configure(role);\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> -\n> -\treturn capture.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> 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.32.0\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 B6214C3220\n\tfor ;\n\tMon, 28 Jun 2021 08:28:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22FD9684D6;\n\tMon, 28 Jun 2021 10:28:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74F85684D2\n\tfor ;\n\tMon, 28 Jun 2021 10:28:13 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 002CFB8A;\n\tMon, 28 Jun 2021 10:28:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ur187Biz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624868893;\n\tbh=KdolKjwLBE1UQN4ui1CYm7ygxwYD1+Sl1IausFe+GFI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ur187BizcpzQDFCYomnJ4M2xn5hgibt8IZ5YEDNCMg7e5qT5khqm8tCLA0Inn8BSS\n\t5wmk3gDBoLpyO51FLm/siDXimW8KyO59LqQBaELQOlzYeA3KVy/i+sXWcTSP55QU3J\n\tee2oR+gRAIXG4TvUSWI+0pxStULhIdWqPYy4RHyI=","Date":"Mon, 28 Jun 2021 17:28:04 +0900","From":"paul.elder@ideasonboard.com","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado ","Message-ID":"<20210628082804.GZ2599@pyrite.rasen.tech>","References":"<20210625193925.517406-1-nfraprado@collabora.com>\n\t<20210625193925.517406-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":"<20210625193925.517406-5-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v9 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":17894,"web_url":"https://patchwork.libcamera.org/comment/17894/","msgid":"<20210628151153.kmzfnwv5ynynyng3@notapiano>","date":"2021-06-28T15:11:53","subject":"Re: [libcamera-devel] [PATCH v9 4/5] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Jacopo,\n\nOn Sat, Jun 26, 2021 at 10:12:31AM +0200, Jacopo Mondi wrote:\n> Hi Nicolas,\n> \n> first of all: this is a great job! The introduction of\n> lc-compliance in kernel-ci is an amazing step forward, thanks a lot\n> for getting to this point so quickly!\n\nGlad you like it :)\n\n> \n> On Fri, Jun 25, 2021 at 04:39:24PM -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> > Reviewed-by: Niklas Söderlund \n> > ---\n> > Changes in v9:\n> > - Thanks to Jacopo:\n> > - Removed the Harness class, substituting methods with static helper functions\n> > - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()\n> > - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()\n> > - Changed tests naming:\n> > - TestSuite instance: RolesAndRequests -> CaptureTests\n> > - TestSuite: FixedRequestsTest -> SingleStream\n> > - TestCase: BalancedSingleCycle -> Capture\n> > - TestCase: BalancedMultiCycle -> CaptureStartStop\n> > - TestCase: Unbalanced -> UnbalancedStop\n> > - Renamed single_stream.cpp to capture_test.cpp\n> >\n> > Changes in v8:\n> > - Thanks to Laurent:\n> > - Fixed lc-compliance's meson.build to disable when gtest is not installed\n> > - Fixed compiling error in Clang\n> >\n> > Changes in v7:\n> > - Thanks to Jacopo:\n> > - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n> >\n> > No changes in v6\n> >\n> > Changes in v5:\n> > - Thanks to Laurent:\n> > - Fixed style issues\n> > - Added SetUp and TearDown functions to acquire and release the camera for\n> > each test\n> >\n> > Changes in v4:\n> > - Removed old lc-compliance results classes\n> > - Thanks to Niklas:\n> > - Improved naming of tests\n> >\n> > Changes in v3:\n> > - Thanks to Niklas:\n> > - Went back to static test registration, and created a Singleton Environment\n> > class to store the camera global variable\n> > - Added a nameParameters() function to give meaningful names to the test\n> > parameters, removing the need to register each test suite for every role\n> >\n> > src/lc-compliance/capture_test.cpp | 127 +++++++++++++++++++++++++++\n> > src/lc-compliance/main.cpp | 106 +++++++++-------------\n> > src/lc-compliance/meson.build | 7 +-\n> > src/lc-compliance/results.cpp | 75 ----------------\n> > src/lc-compliance/results.h | 47 ----------\n> > src/lc-compliance/simple_capture.cpp | 77 +++++++---------\n> > src/lc-compliance/simple_capture.h | 9 +-\n> > src/lc-compliance/single_stream.cpp | 97 --------------------\n> > src/lc-compliance/tests.h | 16 ----\n> > 9 files changed, 206 insertions(+), 355 deletions(-)\n> > create mode 100644 src/lc-compliance/capture_test.cpp\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/single_stream.cpp\n> > delete mode 100644 src/lc-compliance/tests.h\n> >\n> > diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp\n> > new file mode 100644\n> > index 000000000000..116ae496fe04\n> > --- /dev/null\n> > +++ b/src/lc-compliance/capture_test.cpp\n> > @@ -0,0 +1,127 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> \n> Aaaah, time flies! Could you check the year for the other files too ?\n\nNow that you mention it, I wonder if the changes were big enough that I should\nadd a\n\n Copyright (C) 2021, Collabora Ltd.\n\ninstead on this file. Maybe even for main.cpp as well?\n\n> \n> > + *\n> > + * single_stream.cpp - Test a single camera stream\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"environment.h\"\n> > +#include \"simple_capture.h\"\n> \n> We tend to include the file header first to make sure it's\n> self-contained.\n\nThis file (now named capture_test.cpp) doesn't have a header file though. But\nthat shows that I forgot to update the file name and description in the comment\na few lines above.\n\n> \n> > +\n> > +using namespace libcamera;\n> > +\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 SingleStream : public testing::TestWithParam>\n> > +{\n> > +public:\n> > +\tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> > +\n> > +protected:\n> > +\tvoid SetUp() override;\n> > +\tvoid TearDown() override;\n> > +\n> > +\tstd::shared_ptr camera_;\n> > +};\n> > +\n> > +/*\n> > + * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> > + * in order to be able to assert on them.\n> > + */\n> > +void SingleStream::SetUp()\n> > +{\n> > +\tEnvironment *env = Environment::get();\n> > +\n> > +\tcamera_ = env->cm()->get(env->cameraId());\n> > +\n> > +\tASSERT_EQ(camera_->acquire(), 0);\n> > +}\n> > +\n> > +void SingleStream::TearDown()\n> > +{\n> > +\tif (!camera_)\n> > +\t\treturn;\n> > +\n> > +\tcamera_->release();\n> > +\tcamera_.reset();\n> > +}\n> > +\n> > +std::string SingleStream::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> > +\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> I'm not sure I get the last statement (probably I had the same comment on the\n> previous version :) is \"M\" the queue depth ? Honestly I would drop it.\n\nI think that statement could be later improved to be less generic, but still I\nsee some use in it. Regardless of what M means, if there are cases where a few\nextra requests are required to complete the intended amount, we should be on the\nlookout for them.\n\n> \n> > + */\n> > +TEST_P(SingleStream, Capture)\n> > +{\n> > +\tauto [role, numRequests] = GetParam();\n> > +\n> > +\tSimpleCaptureBalanced capture(camera_);\n> > +\n> > +\tcapture.configure(role);\n> > +\n> > +\tcapture.capture(numRequests);\n> > +}\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(SingleStream, CaptureStartStop)\n> > +{\n> > +\tauto [role, numRequests] = GetParam();\n> > +\tunsigned int numRepeats = 3;\n> > +\n> > +\tSimpleCaptureBalanced capture(camera_);\n> > +\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(SingleStream, UnbalancedStop)\n> > +{\n> > +\tauto [role, numRequests] = GetParam();\n> > +\n> > +\tSimpleCaptureUnbalanced capture(camera_);\n> > +\n> > +\tcapture.configure(role);\n> > +\n> > +\tcapture.capture(numRequests);\n> > +}\n> > +\n> > +INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > +\t\t\t SingleStream,\n> > +\t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > +\t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> > +\t\t\t SingleStream::nameParameters);\n> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {\n> > \tOptHelp = 'h',\n> > };\n> >\n> > -class Harness\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> > -public:\n> > -\tHarness(const OptionsParser::Options &options);\n> > -\t~Harness();\n> > -\n> > -\tint exec();\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> > +\tvoid OnTestPartResult(const testing::TestPartResult &result) override\n> > +\t{\n> > +\t\tif (result.type() == testing::TestPartResult::kFatalFailure ||\n> > +\t\t result.type() == testing::TestPartResult::kSkip)\n> > +\t\t\tthrow testing::AssertionException(result);\n> > +\t}\n> > };\n> >\n> > -Harness::Harness(const OptionsParser::Options &options)\n> > -\t: options_(options)\n> > -{\n> > -\tcm_ = std::make_unique();\n> > -}\n> > -\n> > -Harness::~Harness()\n> > +static void listCameras(CameraManager *cm)\n> > {\n> > -\tif (camera_) {\n> > -\t\tcamera_->release();\n> > -\t\tcamera_.reset();\n> > -\t}\n> > -\n> > -\tcm_->stop();\n> > +\tfor (const std::shared_ptr &cam : cm->cameras())\n> > +\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> > }\n> >\n> > -int Harness::exec()\n> > +static int initCamera(CameraManager *cm, OptionsParser::Options options)\n> > {\n> > -\tint ret = init();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\tstd::vector results;\n> > +\tstd::shared_ptr camera;\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> > -\tint ret = cm_->start();\n> > +\tint ret = cm->start();\n> > \tif (ret) {\n> > \t\tstd::cout << \"Failed to start camera manager: \"\n> > \t\t\t << strerror(-ret) << std::endl;\n> > \t\treturn ret;\n> > \t}\n> >\n> > -\tif (!options_.isSet(OptCamera)) {\n> > +\tif (!options.isSet(OptCamera)) {\n> > \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> > -\t\tlistCameras();\n> > +\t\tlistCameras(cm);\n> > \t\treturn -ENODEV;\n> > \t}\n> >\n> > -\tconst std::string &cameraId = options_[OptCamera];\n> > -\tcamera_ = cm_->get(cameraId);\n> > -\tif (!camera_) {\n> > +\tconst std::string &cameraId = options[OptCamera];\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\tlistCameras(cm);\n> > \t\treturn -ENODEV;\n> > \t}\n> >\n> > -\tif (camera_->acquire()) {\n> > -\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > +\tEnvironment::get()->setup(cm, cameraId);\n> >\n> > \tstd::cout << \"Using camera \" << cameraId << std::endl;\n> >\n> > \treturn 0;\n> > }\n> >\n> > -void Harness::listCameras()\n> > -{\n> > -\tfor (const std::shared_ptr &cam : cm_->cameras())\n> > -\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> > -}\n> > -\n> > static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> > {\n> > \tOptionsParser parser;\n> > @@ -142,7 +106,17 @@ int main(int argc, char **argv)\n> > \tif (ret < 0)\n> > \t\treturn EXIT_FAILURE;\n> >\n> > -\tHarness harness(options);\n> > +\tstd::unique_ptr cm = std::make_unique();\n> > +\n> > +\tret = initCamera(cm.get(), options);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> > +\n> > +\tret = RUN_ALL_TESTS();\n> > +\n> > +\tcm->stop();\n> >\n> > -\treturn harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;\n> > +\treturn ret;\n> > }\n> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> > index 6dd49085569f..f3a79ae438a9 100644\n> > --- a/src/lc-compliance/meson.build\n> > +++ b/src/lc-compliance/meson.build\n> > @@ -1,8 +1,9 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))\n> > +libgtest = dependency('gtest', required : get_option('lc-compliance'))\n> >\n> > -if not libevent.found()\n> > +if not (libevent.found() and libgtest.found())\n> > lc_compliance_enabled = false\n> > subdir_done()\n> > endif\n> > @@ -14,9 +15,8 @@ lc_compliance_sources = files([\n> > '../cam/options.cpp',\n> > 'environment.cpp',\n> > 'main.cpp',\n> > - 'results.cpp',\n> > 'simple_capture.cpp',\n> > - 'single_stream.cpp',\n> > + 'capture_test.cpp',\n> > ])\n> >\n> > lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > @@ -24,5 +24,6 @@ lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > libatomic,\n> > libcamera_dep,\n> > libevent,\n> > + libgtest,\n> \n> After the recent split of libcamera-base this does not apply anymore.\n> Rebase is trivial, but much easier for you since you have a branch.\n> I'm afraid this would require a v10 to make things simpler on our side\n> (fixing this is trivial, but I have conflicts also when applying the\n> next patch and that would require manually applying the diff :(\n\nInteresting, I didn't know a rebase was able to automatically resolve basic\nconflicts while applying patches wasn't. But indeed I rebased it now on master\nwithout any tweaks needed. Should be good for v10.\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 bfc91b26444e..ce0883672709 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,37 @@ 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> > +\tint count = allocator_->allocate(stream);\n> >\n> > -\tif (camera_->start())\n> > -\t\treturn { Results::Fail, \"Failed to start camera\" };\n> > +\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > +\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> >\n> > -\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > +\tASSERT_TRUE(!camera_->start()) << \"Failed to start camera\";\n> >\n> > -\treturn { Results::Pass, \"Started camera\" };\n> > +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> \n> Shouldn't the signal be connected before starting the camera ?\n\nI guess it doesn't matter as long as it's connected before any requests are\nqueued. But also doesn't hurt to move it :), and that makes it more symmetrical\nto stop().\n\n> \n> > }\n> >\n> > void SimpleCapture::stop()\n> > @@ -74,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> > @@ -100,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> ASSERT_EQ(.., 0)\n\nSure.\n\n> \n> >\n> > -\t\tif (queueRequest(request.get()) < 0)\n> > -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> > +\t\tASSERT_GE(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> >\n> > \t\trequests.push_back(std::move(request));\n> > \t}\n> > @@ -118,12 +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> > @@ -155,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> > @@ -171,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> maybe it's just me being disappointed by the usage of FALSE() to test the\n> resuls of a function is 0 ? If that's the case just ignore the\n> comments on this topic :)\n\nNo, I do think it makes sense. I went ahead and changed some others.\n\nRelated to that, checking that pointers are valid is being done with\nASSERT_TRUE(ptr). Do you think it would be better to use ASSERT_NE(ptr, 0)\ninstead?\n\n> \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> ASSERT_EQ() ?\n\nSure.\n\n> \n> All minors, I like this new setup and the naming scheme (at least I\n> agree with myself time to time :) I'm sure we'll refactor later as you\n> suggested but for now that's fine.\n> \n> Reviewed-by: Jacopo Mondi \n\nThanks!\n\nNícolas\n\n> \n> Thanks\n> j\n> \n> \n> >\n> > \t\trequests.push_back(std::move(request));\n> > \t}\n> > @@ -189,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> > deleted file mode 100644\n> > index 8318b42f42d6..000000000000\n> > --- a/src/lc-compliance/single_stream.cpp\n> > +++ /dev/null\n> > @@ -1,97 +0,0 @@\n> > -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Google Inc.\n> > - *\n> > - * single_stream.cpp - Test a single camera stream\n> > - */\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> > -{\n> > -\tSimpleCaptureBalanced capture(camera);\n> > -\n> > -\tResults::Result ret = capture.configure(role);\n> > -\tif (ret.first != Results::Pass)\n> > -\t\treturn ret;\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> > -\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> > -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> > -\t\t\t\t StreamRole role, unsigned int numRequests)\n> > -{\n> > -\tSimpleCaptureUnbalanced capture(camera);\n> > -\n> > -\tResults::Result ret = capture.configure(role);\n> > -\tif (ret.first != Results::Pass)\n> > -\t\treturn ret;\n> > -\n> > -\treturn capture.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> > 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.32.0\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 71719C321F\n\tfor ;\n\tMon, 28 Jun 2021 15:12:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5745684D7;\n\tMon, 28 Jun 2021 17:12:03 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0EFA66028C\n\tfor ;\n\tMon, 28 Jun 2021 17:12:02 +0200 (CEST)","from notapiano (unknown [IPv6:2804:14c:1a9:2434:f67:4f14:468a:ed5])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\t(Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id B00EE1F427DA;\n\tMon, 28 Jun 2021 16:11:58 +0100 (BST)"],"Date":"Mon, 28 Jun 2021 12:11:53 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Jacopo Mondi ","Message-ID":"<20210628151153.kmzfnwv5ynynyng3@notapiano>","References":"<20210625193925.517406-1-nfraprado@collabora.com>\n\t<20210625193925.517406-5-nfraprado@collabora.com>\n\t<20210626081231.cgruwdgdwhtkdqbn@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210626081231.cgruwdgdwhtkdqbn@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v9 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":17897,"web_url":"https://patchwork.libcamera.org/comment/17897/","msgid":"<20210628163022.hadnl7bwui6k6evy@notapiano>","date":"2021-06-28T16:30:22","subject":"Re: [libcamera-devel] [PATCH v9 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 Paul,\n\nOn Mon, Jun 28, 2021 at 05:28:04PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Nicolas,\n> \n> On Fri, Jun 25, 2021 at 04:39:24PM -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> > Reviewed-by: Niklas Söderlund \n> > ---\n> > Changes in v9:\n> > - Thanks to Jacopo:\n> > - Removed the Harness class, substituting methods with static helper functions\n> > - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()\n> > - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()\n> > - Changed tests naming:\n> > - TestSuite instance: RolesAndRequests -> CaptureTests\n> > - TestSuite: FixedRequestsTest -> SingleStream\n> > - TestCase: BalancedSingleCycle -> Capture\n> > - TestCase: BalancedMultiCycle -> CaptureStartStop\n> > - TestCase: Unbalanced -> UnbalancedStop\n> > - Renamed single_stream.cpp to capture_test.cpp\n> > \n> > Changes in v8:\n> > - Thanks to Laurent:\n> > - Fixed lc-compliance's meson.build to disable when gtest is not installed\n> > - Fixed compiling error in Clang\n> > \n> > Changes in v7:\n> > - Thanks to Jacopo:\n> > - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n> > \n> > No changes in v6\n> > \n> > Changes in v5:\n> > - Thanks to Laurent:\n> > - Fixed style issues\n> > - Added SetUp and TearDown functions to acquire and release the camera for\n> > each test\n> > \n> > Changes in v4:\n> > - Removed old lc-compliance results classes\n> > - Thanks to Niklas:\n> > - Improved naming of tests\n> > \n> > Changes in v3:\n> > - Thanks to Niklas:\n> > - Went back to static test registration, and created a Singleton Environment\n> > class to store the camera global variable\n> > - Added a nameParameters() function to give meaningful names to the test\n> > parameters, removing the need to register each test suite for every role\n> > \n> > src/lc-compliance/capture_test.cpp | 127 +++++++++++++++++++++++++++\n> > src/lc-compliance/main.cpp | 106 +++++++++-------------\n> > src/lc-compliance/meson.build | 7 +-\n> > src/lc-compliance/results.cpp | 75 ----------------\n> > src/lc-compliance/results.h | 47 ----------\n> > src/lc-compliance/simple_capture.cpp | 77 +++++++---------\n> > src/lc-compliance/simple_capture.h | 9 +-\n> > src/lc-compliance/single_stream.cpp | 97 --------------------\n> > src/lc-compliance/tests.h | 16 ----\n> > 9 files changed, 206 insertions(+), 355 deletions(-)\n> > create mode 100644 src/lc-compliance/capture_test.cpp\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/single_stream.cpp\n> > delete mode 100644 src/lc-compliance/tests.h\n> > \n> > diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp\n> > new file mode 100644\n> > index 000000000000..116ae496fe04\n> > --- /dev/null\n> > +++ b/src/lc-compliance/capture_test.cpp\n> > @@ -0,0 +1,127 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * single_stream.cpp - Test a single camera stream\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"environment.h\"\n> > +#include \"simple_capture.h\"\n> > +\n> > +using namespace libcamera;\n> > +\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 SingleStream : public testing::TestWithParam>\n> > +{\n> > +public:\n> > +\tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> > +\n> > +protected:\n> > +\tvoid SetUp() override;\n> > +\tvoid TearDown() override;\n> > +\n> > +\tstd::shared_ptr camera_;\n> > +};\n> > +\n> > +/*\n> > + * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> > + * in order to be able to assert on them.\n> > + */\n> > +void SingleStream::SetUp()\n> > +{\n> > +\tEnvironment *env = Environment::get();\n> > +\n> > +\tcamera_ = env->cm()->get(env->cameraId());\n> > +\n> > +\tASSERT_EQ(camera_->acquire(), 0);\n> > +}\n> > +\n> > +void SingleStream::TearDown()\n> > +{\n> > +\tif (!camera_)\n> > +\t\treturn;\n> > +\n> > +\tcamera_->release();\n> > +\tcamera_.reset();\n> > +}\n> > +\n> > +std::string SingleStream::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(SingleStream, Capture)\n> > +{\n> > +\tauto [role, numRequests] = GetParam();\n> > +\n> > +\tSimpleCaptureBalanced capture(camera_);\n> > +\n> > +\tcapture.configure(role);\n> > +\n> > +\tcapture.capture(numRequests);\n> > +}\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(SingleStream, CaptureStartStop)\n> > +{\n> > +\tauto [role, numRequests] = GetParam();\n> > +\tunsigned int numRepeats = 3;\n> > +\n> > +\tSimpleCaptureBalanced capture(camera_);\n> > +\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(SingleStream, UnbalancedStop)\n> > +{\n> > +\tauto [role, numRequests] = GetParam();\n> > +\n> > +\tSimpleCaptureUnbalanced capture(camera_);\n> > +\n> > +\tcapture.configure(role);\n> > +\n> > +\tcapture.capture(numRequests);\n> > +}\n> > +\n> > +INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > +\t\t\t SingleStream,\n> > +\t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > +\t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> > +\t\t\t SingleStream::nameParameters);\n> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {\n> > \tOptHelp = 'h',\n> > };\n> > \n> > -class Harness\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> > -public:\n> > -\tHarness(const OptionsParser::Options &options);\n> > -\t~Harness();\n> > -\n> > -\tint exec();\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> > +\tvoid OnTestPartResult(const testing::TestPartResult &result) override\n> > +\t{\n> > +\t\tif (result.type() == testing::TestPartResult::kFatalFailure ||\n> > +\t\t result.type() == testing::TestPartResult::kSkip)\n> > +\t\t\tthrow testing::AssertionException(result);\n> \n> This fails compilation in cros_sdk, as exceptions are disabled. The\n> behavior can be reproduced outside of the cros_sdk by adding\n> -fno-exceptions and compiling on clang.\n> \n> Adding this fixes the issue:\n> \n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index 0fd2aca1..aa5852f6 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -20,6 +20,7 @@ lc_compliance_sources = files([\n> ])\n> \n> lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> + cpp_args : [ '-fexceptions' ],\n> dependencies : [\n> libatomic,\n> libcamera_public,\n\nThank you for the feedback and the fix. Added for v10.\n\nNícolas\n\n> \n> \n> Paul\n> \n> > +\t}\n> > };\n> > \n> > -Harness::Harness(const OptionsParser::Options &options)\n> > -\t: options_(options)\n> > -{\n> > -\tcm_ = std::make_unique();\n> > -}\n> > -\n> > -Harness::~Harness()\n> > +static void listCameras(CameraManager *cm)\n> > {\n> > -\tif (camera_) {\n> > -\t\tcamera_->release();\n> > -\t\tcamera_.reset();\n> > -\t}\n> > -\n> > -\tcm_->stop();\n> > +\tfor (const std::shared_ptr &cam : cm->cameras())\n> > +\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> > }\n> > \n> > -int Harness::exec()\n> > +static int initCamera(CameraManager *cm, OptionsParser::Options options)\n> > {\n> > -\tint ret = init();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\tstd::vector results;\n> > +\tstd::shared_ptr camera;\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> > -\tint ret = cm_->start();\n> > +\tint ret = cm->start();\n> > \tif (ret) {\n> > \t\tstd::cout << \"Failed to start camera manager: \"\n> > \t\t\t << strerror(-ret) << std::endl;\n> > \t\treturn ret;\n> > \t}\n> > \n> > -\tif (!options_.isSet(OptCamera)) {\n> > +\tif (!options.isSet(OptCamera)) {\n> > \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> > -\t\tlistCameras();\n> > +\t\tlistCameras(cm);\n> > \t\treturn -ENODEV;\n> > \t}\n> > \n> > -\tconst std::string &cameraId = options_[OptCamera];\n> > -\tcamera_ = cm_->get(cameraId);\n> > -\tif (!camera_) {\n> > +\tconst std::string &cameraId = options[OptCamera];\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\tlistCameras(cm);\n> > \t\treturn -ENODEV;\n> > \t}\n> > \n> > -\tif (camera_->acquire()) {\n> > -\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > +\tEnvironment::get()->setup(cm, cameraId);\n> > \n> > \tstd::cout << \"Using camera \" << cameraId << std::endl;\n> > \n> > \treturn 0;\n> > }\n> > \n> > -void Harness::listCameras()\n> > -{\n> > -\tfor (const std::shared_ptr &cam : cm_->cameras())\n> > -\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> > -}\n> > -\n> > static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> > {\n> > \tOptionsParser parser;\n> > @@ -142,7 +106,17 @@ int main(int argc, char **argv)\n> > \tif (ret < 0)\n> > \t\treturn EXIT_FAILURE;\n> > \n> > -\tHarness harness(options);\n> > +\tstd::unique_ptr cm = std::make_unique();\n> > +\n> > +\tret = initCamera(cm.get(), options);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> > +\n> > +\tret = RUN_ALL_TESTS();\n> > +\n> > +\tcm->stop();\n> > \n> > -\treturn harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;\n> > +\treturn ret;\n> > }\n> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> > index 6dd49085569f..f3a79ae438a9 100644\n> > --- a/src/lc-compliance/meson.build\n> > +++ b/src/lc-compliance/meson.build\n> > @@ -1,8 +1,9 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> > \n> > libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))\n> > +libgtest = dependency('gtest', required : get_option('lc-compliance'))\n> > \n> > -if not libevent.found()\n> > +if not (libevent.found() and libgtest.found())\n> > lc_compliance_enabled = false\n> > subdir_done()\n> > endif\n> > @@ -14,9 +15,8 @@ lc_compliance_sources = files([\n> > '../cam/options.cpp',\n> > 'environment.cpp',\n> > 'main.cpp',\n> > - 'results.cpp',\n> > 'simple_capture.cpp',\n> > - 'single_stream.cpp',\n> > + 'capture_test.cpp',\n> > ])\n> > \n> > lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > @@ -24,5 +24,6 @@ lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > libatomic,\n> > libcamera_dep,\n> > libevent,\n> > + libgtest,\n> > ],\n> > install : true)\n> > diff --git a/src/lc-compliance/results.cpp b/src/lc-compliance/results.cpp\n> > deleted file mode 100644\n> > index f149f7859e28..000000000000\n> > --- a/src/lc-compliance/results.cpp\n> > +++ /dev/null\n> > @@ -1,75 +0,0 @@\n> > -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Google Inc.\n> > - *\n> > - * results.cpp - Test result aggregator\n> > - */\n> > -\n> > -#include \"results.h\"\n> > -\n> > -#include \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 bfc91b26444e..ce0883672709 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,37 @@ 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> > +\tint count = allocator_->allocate(stream);\n> > \n> > -\tif (camera_->start())\n> > -\t\treturn { Results::Fail, \"Failed to start camera\" };\n> > +\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > +\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > \n> > -\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > +\tASSERT_TRUE(!camera_->start()) << \"Failed to start camera\";\n> > \n> > -\treturn { Results::Pass, \"Started camera\" };\n> > +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > }\n> > \n> > void SimpleCapture::stop()\n> > @@ -74,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> > @@ -100,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> > @@ -118,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> > @@ -155,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> > @@ -171,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> > @@ -189,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> > deleted file mode 100644\n> > index 8318b42f42d6..000000000000\n> > --- a/src/lc-compliance/single_stream.cpp\n> > +++ /dev/null\n> > @@ -1,97 +0,0 @@\n> > -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Google Inc.\n> > - *\n> > - * single_stream.cpp - Test a single camera stream\n> > - */\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> > -{\n> > -\tSimpleCaptureBalanced capture(camera);\n> > -\n> > -\tResults::Result ret = capture.configure(role);\n> > -\tif (ret.first != Results::Pass)\n> > -\t\treturn ret;\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> > -\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> > -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> > -\t\t\t\t StreamRole role, unsigned int numRequests)\n> > -{\n> > -\tSimpleCaptureUnbalanced capture(camera);\n> > -\n> > -\tResults::Result ret = capture.configure(role);\n> > -\tif (ret.first != Results::Pass)\n> > -\t\treturn ret;\n> > -\n> > -\treturn capture.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> > 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.32.0\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 B1DAFC3220\n\tfor ;\n\tMon, 28 Jun 2021 16:30:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D853684D7;\n\tMon, 28 Jun 2021 18:30:30 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E08106028C\n\tfor ;\n\tMon, 28 Jun 2021 18:30:28 +0200 (CEST)","from notapiano (unknown [IPv6:2804:14c:1a9:2434:f67:4f14:468a:ed5])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\t(Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 57C421F423C2;\n\tMon, 28 Jun 2021 17:30:27 +0100 (BST)"],"Date":"Mon, 28 Jun 2021 13:30:22 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"paul.elder@ideasonboard.com","Message-ID":"<20210628163022.hadnl7bwui6k6evy@notapiano>","References":"<20210625193925.517406-1-nfraprado@collabora.com>\n\t<20210625193925.517406-5-nfraprado@collabora.com>\n\t<20210628082804.GZ2599@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210628082804.GZ2599@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v9 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":17902,"web_url":"https://patchwork.libcamera.org/comment/17902/","msgid":"<20210628175630.zahg4gmcvhriexmu@uno.localdomain>","date":"2021-06-28T17:56:30","subject":"Re: [libcamera-devel] [PATCH v9 4/5] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas,\n\nOn Mon, Jun 28, 2021 at 12:11:53PM -0300, Nícolas F. R. A. Prado wrote:\n> Hi Jacopo,\n>\n> On Sat, Jun 26, 2021 at 10:12:31AM +0200, Jacopo Mondi wrote:\n> > Hi Nicolas,\n> >\n> > first of all: this is a great job! The introduction of\n> > lc-compliance in kernel-ci is an amazing step forward, thanks a lot\n> > for getting to this point so quickly!\n>\n> Glad you like it :)\n>\n> >\n> > On Fri, Jun 25, 2021 at 04:39:24PM -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> > > Reviewed-by: Niklas Söderlund \n> > > ---\n> > > Changes in v9:\n> > > - Thanks to Jacopo:\n> > > - Removed the Harness class, substituting methods with static helper functions\n> > > - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()\n> > > - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()\n> > > - Changed tests naming:\n> > > - TestSuite instance: RolesAndRequests -> CaptureTests\n> > > - TestSuite: FixedRequestsTest -> SingleStream\n> > > - TestCase: BalancedSingleCycle -> Capture\n> > > - TestCase: BalancedMultiCycle -> CaptureStartStop\n> > > - TestCase: Unbalanced -> UnbalancedStop\n> > > - Renamed single_stream.cpp to capture_test.cpp\n> > >\n> > > Changes in v8:\n> > > - Thanks to Laurent:\n> > > - Fixed lc-compliance's meson.build to disable when gtest is not installed\n> > > - Fixed compiling error in Clang\n> > >\n> > > Changes in v7:\n> > > - Thanks to Jacopo:\n> > > - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n> > >\n> > > No changes in v6\n> > >\n> > > Changes in v5:\n> > > - Thanks to Laurent:\n> > > - Fixed style issues\n> > > - Added SetUp and TearDown functions to acquire and release the camera for\n> > > each test\n> > >\n> > > Changes in v4:\n> > > - Removed old lc-compliance results classes\n> > > - Thanks to Niklas:\n> > > - Improved naming of tests\n> > >\n> > > Changes in v3:\n> > > - Thanks to Niklas:\n> > > - Went back to static test registration, and created a Singleton Environment\n> > > class to store the camera global variable\n> > > - Added a nameParameters() function to give meaningful names to the test\n> > > parameters, removing the need to register each test suite for every role\n> > >\n> > > src/lc-compliance/capture_test.cpp | 127 +++++++++++++++++++++++++++\n> > > src/lc-compliance/main.cpp | 106 +++++++++-------------\n> > > src/lc-compliance/meson.build | 7 +-\n> > > src/lc-compliance/results.cpp | 75 ----------------\n> > > src/lc-compliance/results.h | 47 ----------\n> > > src/lc-compliance/simple_capture.cpp | 77 +++++++---------\n> > > src/lc-compliance/simple_capture.h | 9 +-\n> > > src/lc-compliance/single_stream.cpp | 97 --------------------\n> > > src/lc-compliance/tests.h | 16 ----\n> > > 9 files changed, 206 insertions(+), 355 deletions(-)\n> > > create mode 100644 src/lc-compliance/capture_test.cpp\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/single_stream.cpp\n> > > delete mode 100644 src/lc-compliance/tests.h\n> > >\n> > > diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp\n> > > new file mode 100644\n> > > index 000000000000..116ae496fe04\n> > > --- /dev/null\n> > > +++ b/src/lc-compliance/capture_test.cpp\n> > > @@ -0,0 +1,127 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> >\n> > Aaaah, time flies! Could you check the year for the other files too ?\n>\n> Now that you mention it, I wonder if the changes were big enough that I should\n> add a\n>\n> Copyright (C) 2021, Collabora Ltd.\n>\n> instead on this file. Maybe even for main.cpp as well?\n\nI think it's fair to do so, this is quite a substantial change to the\nexisting code.\n\nI'm not aware if there's a fixed rule for that or it is left to the\ncommon sense...\n\n> >\n> > > + *\n> > > + * single_stream.cpp - Test a single camera stream\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"environment.h\"\n> > > +#include \"simple_capture.h\"\n> >\n> > We tend to include the file header first to make sure it's\n> > self-contained.\n>\n> This file (now named capture_test.cpp) doesn't have a header file though. But\n> that shows that I forgot to update the file name and description in the comment\n> a few lines above.\n\nRight, I confused single_stream.h and simple_capture.h. Regardelss,\nplease update the file name in the initial comment block\n\n>\n> >\n> > > +\n> > > +using namespace libcamera;\n> > > +\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 SingleStream : public testing::TestWithParam>\n> > > +{\n> > > +public:\n> > > +\tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> > > +\n> > > +protected:\n> > > +\tvoid SetUp() override;\n> > > +\tvoid TearDown() override;\n> > > +\n> > > +\tstd::shared_ptr camera_;\n> > > +};\n> > > +\n> > > +/*\n> > > + * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> > > + * in order to be able to assert on them.\n> > > + */\n> > > +void SingleStream::SetUp()\n> > > +{\n> > > +\tEnvironment *env = Environment::get();\n> > > +\n> > > +\tcamera_ = env->cm()->get(env->cameraId());\n> > > +\n> > > +\tASSERT_EQ(camera_->acquire(), 0);\n> > > +}\n> > > +\n> > > +void SingleStream::TearDown()\n> > > +{\n> > > +\tif (!camera_)\n> > > +\t\treturn;\n> > > +\n> > > +\tcamera_->release();\n> > > +\tcamera_.reset();\n> > > +}\n> > > +\n> > > +std::string SingleStream::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> > > +\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> > I'm not sure I get the last statement (probably I had the same comment on the\n> > previous version :) is \"M\" the queue depth ? Honestly I would drop it.\n>\n> I think that statement could be later improved to be less generic, but still I\n> see some use in it. Regardless of what M means, if there are cases where a few\n> extra requests are required to complete the intended amount, we should be on the\n> lookout for them.\n\nNow I probably better get what was meant there. As we skip testing a\nnumber of requests < than the queue depth a camera that completes N\nrequests if you queue M+N means not all requets are completed and\nthat's why it is faulty :)\n\nI would drop or say \"an example failure is a camera that completes\nless requests that the queued ones\".\n\nOr maybe I didn't get it properly yet :)\n\n>\n> >\n> > > + */\n> > > +TEST_P(SingleStream, Capture)\n> > > +{\n> > > +\tauto [role, numRequests] = GetParam();\n> > > +\n> > > +\tSimpleCaptureBalanced capture(camera_);\n> > > +\n> > > +\tcapture.configure(role);\n> > > +\n> > > +\tcapture.capture(numRequests);\n> > > +}\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(SingleStream, CaptureStartStop)\n> > > +{\n> > > +\tauto [role, numRequests] = GetParam();\n> > > +\tunsigned int numRepeats = 3;\n> > > +\n> > > +\tSimpleCaptureBalanced capture(camera_);\n> > > +\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(SingleStream, UnbalancedStop)\n> > > +{\n> > > +\tauto [role, numRequests] = GetParam();\n> > > +\n> > > +\tSimpleCaptureUnbalanced capture(camera_);\n> > > +\n> > > +\tcapture.configure(role);\n> > > +\n> > > +\tcapture.capture(numRequests);\n> > > +}\n> > > +\n> > > +INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > +\t\t\t SingleStream,\n> > > +\t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > > +\t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> > > +\t\t\t SingleStream::nameParameters);\n> > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > > index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {\n> > > \tOptHelp = 'h',\n> > > };\n> > >\n> > > -class Harness\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> > > -public:\n> > > -\tHarness(const OptionsParser::Options &options);\n> > > -\t~Harness();\n> > > -\n> > > -\tint exec();\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> > > +\tvoid OnTestPartResult(const testing::TestPartResult &result) override\n> > > +\t{\n> > > +\t\tif (result.type() == testing::TestPartResult::kFatalFailure ||\n> > > +\t\t result.type() == testing::TestPartResult::kSkip)\n> > > +\t\t\tthrow testing::AssertionException(result);\n> > > +\t}\n> > > };\n> > >\n> > > -Harness::Harness(const OptionsParser::Options &options)\n> > > -\t: options_(options)\n> > > -{\n> > > -\tcm_ = std::make_unique();\n> > > -}\n> > > -\n> > > -Harness::~Harness()\n> > > +static void listCameras(CameraManager *cm)\n> > > {\n> > > -\tif (camera_) {\n> > > -\t\tcamera_->release();\n> > > -\t\tcamera_.reset();\n> > > -\t}\n> > > -\n> > > -\tcm_->stop();\n> > > +\tfor (const std::shared_ptr &cam : cm->cameras())\n> > > +\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> > > }\n> > >\n> > > -int Harness::exec()\n> > > +static int initCamera(CameraManager *cm, OptionsParser::Options options)\n> > > {\n> > > -\tint ret = init();\n> > > -\tif (ret)\n> > > -\t\treturn ret;\n> > > -\n> > > -\tstd::vector results;\n> > > +\tstd::shared_ptr camera;\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> > > -\tint ret = cm_->start();\n> > > +\tint ret = cm->start();\n> > > \tif (ret) {\n> > > \t\tstd::cout << \"Failed to start camera manager: \"\n> > > \t\t\t << strerror(-ret) << std::endl;\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > -\tif (!options_.isSet(OptCamera)) {\n> > > +\tif (!options.isSet(OptCamera)) {\n> > > \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> > > -\t\tlistCameras();\n> > > +\t\tlistCameras(cm);\n> > > \t\treturn -ENODEV;\n> > > \t}\n> > >\n> > > -\tconst std::string &cameraId = options_[OptCamera];\n> > > -\tcamera_ = cm_->get(cameraId);\n> > > -\tif (!camera_) {\n> > > +\tconst std::string &cameraId = options[OptCamera];\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\tlistCameras(cm);\n> > > \t\treturn -ENODEV;\n> > > \t}\n> > >\n> > > -\tif (camera_->acquire()) {\n> > > -\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > > -\t\treturn -EINVAL;\n> > > -\t}\n> > > +\tEnvironment::get()->setup(cm, cameraId);\n> > >\n> > > \tstd::cout << \"Using camera \" << cameraId << std::endl;\n> > >\n> > > \treturn 0;\n> > > }\n> > >\n> > > -void Harness::listCameras()\n> > > -{\n> > > -\tfor (const std::shared_ptr &cam : cm_->cameras())\n> > > -\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> > > -}\n> > > -\n> > > static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> > > {\n> > > \tOptionsParser parser;\n> > > @@ -142,7 +106,17 @@ int main(int argc, char **argv)\n> > > \tif (ret < 0)\n> > > \t\treturn EXIT_FAILURE;\n> > >\n> > > -\tHarness harness(options);\n> > > +\tstd::unique_ptr cm = std::make_unique();\n> > > +\n> > > +\tret = initCamera(cm.get(), options);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> > > +\n> > > +\tret = RUN_ALL_TESTS();\n> > > +\n> > > +\tcm->stop();\n> > >\n> > > -\treturn harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;\n> > > +\treturn ret;\n> > > }\n> > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> > > index 6dd49085569f..f3a79ae438a9 100644\n> > > --- a/src/lc-compliance/meson.build\n> > > +++ b/src/lc-compliance/meson.build\n> > > @@ -1,8 +1,9 @@\n> > > # SPDX-License-Identifier: CC0-1.0\n> > >\n> > > libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))\n> > > +libgtest = dependency('gtest', required : get_option('lc-compliance'))\n> > >\n> > > -if not libevent.found()\n> > > +if not (libevent.found() and libgtest.found())\n> > > lc_compliance_enabled = false\n> > > subdir_done()\n> > > endif\n> > > @@ -14,9 +15,8 @@ lc_compliance_sources = files([\n> > > '../cam/options.cpp',\n> > > 'environment.cpp',\n> > > 'main.cpp',\n> > > - 'results.cpp',\n> > > 'simple_capture.cpp',\n> > > - 'single_stream.cpp',\n> > > + 'capture_test.cpp',\n> > > ])\n> > >\n> > > lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > > @@ -24,5 +24,6 @@ lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > > libatomic,\n> > > libcamera_dep,\n> > > libevent,\n> > > + libgtest,\n> >\n> > After the recent split of libcamera-base this does not apply anymore.\n> > Rebase is trivial, but much easier for you since you have a branch.\n> > I'm afraid this would require a v10 to make things simpler on our side\n> > (fixing this is trivial, but I have conflicts also when applying the\n> > next patch and that would require manually applying the diff :(\n>\n> Interesting, I didn't know a rebase was able to automatically resolve basic\n> conflicts while applying patches wasn't. But indeed I rebased it now on master\n> without any tweaks needed. Should be good for v10.\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 bfc91b26444e..ce0883672709 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,37 @@ 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> > > +\tint count = allocator_->allocate(stream);\n> > >\n> > > -\tif (camera_->start())\n> > > -\t\treturn { Results::Fail, \"Failed to start camera\" };\n> > > +\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > +\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > >\n> > > -\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > > +\tASSERT_TRUE(!camera_->start()) << \"Failed to start camera\";\n> > >\n> > > -\treturn { Results::Pass, \"Started camera\" };\n> > > +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> >\n> > Shouldn't the signal be connected before starting the camera ?\n>\n> I guess it doesn't matter as long as it's connected before any requests are\n> queued. But also doesn't hurt to move it :), and that makes it more symmetrical\n> to stop().\n>\n\nSimmetry is nice indeed!\n\n> >\n> > > }\n> > >\n> > > void SimpleCapture::stop()\n> > > @@ -74,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> > > @@ -100,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> > ASSERT_EQ(.., 0)\n>\n> Sure.\n>\n> >\n> > >\n> > > -\t\tif (queueRequest(request.get()) < 0)\n> > > -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> > > +\t\tASSERT_GE(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > >\n> > > \t\trequests.push_back(std::move(request));\n> > > \t}\n> > > @@ -118,12 +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> > > @@ -155,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> > > @@ -171,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> > maybe it's just me being disappointed by the usage of FALSE() to test the\n> > resuls of a function is 0 ? If that's the case just ignore the\n> > comments on this topic :)\n>\n> No, I do think it makes sense. I went ahead and changed some others.\n\nThank you!\n\n>\n> Related to that, checking that pointers are valid is being done with\n> ASSERT_TRUE(ptr). Do you think it would be better to use ASSERT_NE(ptr, 0)\n> instead?\n>\n\nAs we usually do if(!ptr) I think ASSERT_TRUE is fine. Whatever,\nreally :)\n\n> >\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> > ASSERT_EQ() ?\n>\n> Sure.\n>\n> >\n> > All minors, I like this new setup and the naming scheme (at least I\n> > agree with myself time to time :) I'm sure we'll refactor later as you\n> > suggested but for now that's fine.\n> >\n> > Reviewed-by: Jacopo Mondi \n>\n> Thanks!\n>\n\nThank you for pushing this up to v10!\n\n> Nícolas\n>\n> >\n> > Thanks\n> > j\n> >\n> >\n> > >\n> > > \t\trequests.push_back(std::move(request));\n> > > \t}\n> > > @@ -189,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> > > deleted file mode 100644\n> > > index 8318b42f42d6..000000000000\n> > > --- a/src/lc-compliance/single_stream.cpp\n> > > +++ /dev/null\n> > > @@ -1,97 +0,0 @@\n> > > -/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > -/*\n> > > - * Copyright (C) 2020, Google Inc.\n> > > - *\n> > > - * single_stream.cpp - Test a single camera stream\n> > > - */\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> > > -{\n> > > -\tSimpleCaptureBalanced capture(camera);\n> > > -\n> > > -\tResults::Result ret = capture.configure(role);\n> > > -\tif (ret.first != Results::Pass)\n> > > -\t\treturn ret;\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> > > -\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> > > -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> > > -\t\t\t\t StreamRole role, unsigned int numRequests)\n> > > -{\n> > > -\tSimpleCaptureUnbalanced capture(camera);\n> > > -\n> > > -\tResults::Result ret = capture.configure(role);\n> > > -\tif (ret.first != Results::Pass)\n> > > -\t\treturn ret;\n> > > -\n> > > -\treturn capture.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> > > 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.32.0\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 91843C3220\n\tfor ;\n\tMon, 28 Jun 2021 17:55:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAD07684D7;\n\tMon, 28 Jun 2021 19:55:44 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DF2A6028C\n\tfor ;\n\tMon, 28 Jun 2021 19:55:43 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 261BBE0007;\n\tMon, 28 Jun 2021 17:55:41 +0000 (UTC)"],"Date":"Mon, 28 Jun 2021 19:56:30 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210628175630.zahg4gmcvhriexmu@uno.localdomain>","References":"<20210625193925.517406-1-nfraprado@collabora.com>\n\t<20210625193925.517406-5-nfraprado@collabora.com>\n\t<20210626081231.cgruwdgdwhtkdqbn@uno.localdomain>\n\t<20210628151153.kmzfnwv5ynynyng3@notapiano>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210628151153.kmzfnwv5ynynyng3@notapiano>","Subject":"Re: [libcamera-devel] [PATCH v9 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\" "}}]