[libcamera-devel] [PATCH v3 3/4] lc-compliance: Refactor using Googletest
Nícolas F. R. A. Prado
nfraprado at collabora.com
Thu May 20 17:17:17 CEST 2021
Hi Niklas,
thank you for the feedback.
Em 2021-05-16 11:01, Niklas Söderlund escreveu:
> Hi Nícolas,
>
> Thanks for your work!
>
> On 2021-05-14 10:16:51 -0300, Nícolas F. R. A. Prado wrote:
> > Refactor lc-compliance using Googletest as the test framework.
> >
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > ---
> > Changes in v3:
> > - Thanks to Niklas:
> > - Went back to static test registration, and created a Singleton Environment
> > class to store the camera global variable
> > - Added a nameParameters() function to give meaningful names to the test
> > parameters, removing the need to register each test suite for every role
> >
> > src/lc-compliance/main.cpp | 88 ++++++++++------
> > src/lc-compliance/meson.build | 3 +
> > src/lc-compliance/simple_capture.cpp | 74 +++++---------
> > src/lc-compliance/simple_capture.h | 8 +-
> > src/lc-compliance/single_stream.cpp | 148 ++++++++++++++-------------
> > src/lc-compliance/tests.h | 19 +++-
> > 6 files changed, 187 insertions(+), 153 deletions(-)
> >
> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > index 54cee54aa978..d5ff93f514df 100644
> > --- a/src/lc-compliance/main.cpp
> > +++ b/src/lc-compliance/main.cpp
> > @@ -9,6 +9,8 @@
> > #include <iostream>
> > #include <string.h>
> >
> > +#include <gtest/gtest.h>
> > +
> > #include <libcamera/libcamera.h>
> >
> > #include "../cam/options.h"
> > @@ -21,21 +23,43 @@ enum {
> > OptHelp = 'h',
> > };
> >
> > +bool Environment::create(std::shared_ptr<Camera> camera)
> > +{
> > + if (instance_)
> > + return false;
> > +
> > + camera_ = camera;
> > + instance_ = new Environment;
> > + return true;
> > +}
> > +
> > +void Environment::destroy()
> > +{
> > + if (!camera_)
> > + return;
> > +
> > + camera_->release();
> > + camera_.reset();
> > +}
> > +
> > +Environment *Environment::instance()
> > +{
> > + return instance_;
> > +}
>
> I think this and the corresponding class declaration should be moved to
> a separate environment.{cpp,h} file. It could possibly be added in a
> separate patch just previous to this one.
Yep, that makes sense. Will do for v4.
>
> > +
> > class Harness
> > {
> > public:
> > Harness(const OptionsParser::Options &options);
> > ~Harness();
> >
> > - int exec();
> > + int init();
> >
> > private:
> > - int init();
> > void listCameras();
> >
> > OptionsParser::Options options_;
> > std::unique_ptr<CameraManager> cm_;
> > - std::shared_ptr<Camera> camera_;
> > };
> >
> > Harness::Harness(const OptionsParser::Options &options)
> > @@ -46,35 +70,15 @@ Harness::Harness(const OptionsParser::Options &options)
> >
> > Harness::~Harness()
> > {
> > - if (camera_) {
> > - camera_->release();
> > - camera_.reset();
> > - }
> > + Environment::instance()->destroy();
> >
> > cm_->stop();
> > }
> >
> > -int Harness::exec()
> > -{
> > - int ret = init();
> > - if (ret)
> > - return ret;
> > -
> > - std::vector<Results> results;
> > -
> > - results.push_back(testSingleStream(camera_));
> > -
> > - for (const Results &result : results) {
> > - ret = result.summary();
> > - if (ret)
> > - return ret;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > int Harness::init()
> > {
> > + std::shared_ptr<Camera> camera;
> > +
> > int ret = cm_->start();
> > if (ret) {
> > std::cout << "Failed to start camera manager: "
> > @@ -89,18 +93,20 @@ int Harness::init()
> > }
> >
> > const std::string &cameraId = options_[OptCamera];
> > - camera_ = cm_->get(cameraId);
> > - if (!camera_) {
> > + camera = cm_->get(cameraId);
> > + if (!camera) {
> > std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> > listCameras();
> > return -ENODEV;
> > }
> >
> > - if (camera_->acquire()) {
> > + if (camera->acquire()) {
> > std::cout << "Failed to acquire camera" << std::endl;
> > return -EINVAL;
> > }
> >
> > + Environment::create(camera);
> > +
> > std::cout << "Using camera " << cameraId << std::endl;
> >
> > return 0;
> > @@ -133,6 +139,23 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
> > return 0;
> > }
> >
> > +/*
> > + * Make asserts act like exceptions, otherwise they only fail (or skip) the
> > + * current function. From gtest documentation:
> > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception
> > + */
> > +class ThrowListener : public testing::EmptyTestEventListener {
> > + void OnTestPartResult(const testing::TestPartResult& result) override {
> > + if (result.type() == testing::TestPartResult::kFatalFailure
> > + || result.type() == testing::TestPartResult::kSkip) {
> > + throw testing::AssertionException(result);
> > + }
> > + }
> > +};
> > +
> > +Environment *Environment::instance_ = nullptr;
> > +std::shared_ptr<Camera> Environment::camera_ = nullptr;
> > +
> > int main(int argc, char **argv)
> > {
> > OptionsParser::Options options;
> > @@ -143,6 +166,11 @@ int main(int argc, char **argv)
> > return EXIT_FAILURE;
> >
> > Harness harness(options);
> > + ret = harness.init();
> > + if (ret)
> > + return ret;
> >
> > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;
> > + ::testing::InitGoogleTest(&argc, argv);
> > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> > + return RUN_ALL_TESTS();
> > }
> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > index a2bfcceb1259..704bc18af3e1 100644
> > --- a/src/lc-compliance/meson.build
> > +++ b/src/lc-compliance/meson.build
> > @@ -18,10 +18,13 @@ lc_compliance_sources = files([
> > 'single_stream.cpp',
> > ])
> >
> > +gtest_dep = dependency('gtest')
> > +
> > lc_compliance = executable('lc-compliance', lc_compliance_sources,
> > dependencies : [
> > libatomic,
> > libcamera_dep,
> > libevent,
> > + gtest_dep,
> > ],
> > install : true)
> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > index f90fe6d0f9aa..7731eb16f8c2 100644
> > --- a/src/lc-compliance/simple_capture.cpp
> > +++ b/src/lc-compliance/simple_capture.cpp
> > @@ -5,6 +5,8 @@
> > * simple_capture.cpp - Simple capture helper
> > */
> >
> > +#include <gtest/gtest.h>
> > +
> > #include "simple_capture.h"
> >
> > using namespace libcamera;
> > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture()
> > stop();
> > }
> >
> > -Results::Result SimpleCapture::configure(StreamRole role)
> > +void SimpleCapture::configure(StreamRole role)
> > {
> > config_ = camera_->generateConfiguration({ role });
> >
> > - if (!config_)
> > - return { Results::Skip, "Role not supported by camera" };
> > + if (!config_) {
> > + std::cout << "Role not supported by camera" << std::endl;
> > + GTEST_SKIP();
> > + }
> >
> > if (config_->validate() != CameraConfiguration::Valid) {
> > config_.reset();
> > - return { Results::Fail, "Configuration not valid" };
> > + FAIL() << "Configuration not valid";
> > }
> >
> > if (camera_->configure(config_.get())) {
> > config_.reset();
> > - return { Results::Fail, "Failed to configure camera" };
> > + FAIL() << "Failed to configure camera";
> > }
> > -
> > - return { Results::Pass, "Configure camera" };
> > }
> >
> > -Results::Result SimpleCapture::start()
> > +void SimpleCapture::start()
> > {
> > Stream *stream = config_->at(0).stream();
> > - if (allocator_->allocate(stream) < 0)
> > - return { Results::Fail, "Failed to allocate buffers" };
> > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers";
> >
> > - if (camera_->start())
> > - return { Results::Fail, "Failed to start camera" };
> > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera";
> >
> > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> > -
> > - return { Results::Pass, "Started camera" };
> > }
> >
> > void SimpleCapture::stop()
> > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)
> > {
> > }
> >
> > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
> > +void SimpleCaptureBalanced::capture(unsigned int numRequests)
> > {
> > - Results::Result ret = start();
> > - if (ret.first != Results::Pass)
> > - return ret;
> > + start();
> >
> > Stream *stream = config_->at(0).stream();
> > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> >
> > /* No point in testing less requests then the camera depth. */
> > if (buffers.size() > numRequests) {
> > - /* Cache buffers.size() before we destroy it in stop() */
> > - int buffers_size = buffers.size();
> > -
> > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size)
> > - + " requests, can't test only " + std::to_string(numRequests) };
> > + std::cout << "Camera needs " + std::to_string(buffers.size())
> > + + " requests, can't test only "
> > + + std::to_string(numRequests) << std::endl;
> > + GTEST_SKIP();
> > }
> >
> > queueCount_ = 0;
> > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
> > std::vector<std::unique_ptr<libcamera::Request>> requests;
> > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > std::unique_ptr<Request> request = camera_->createRequest();
> > - if (!request)
> > - return { Results::Fail, "Can't create request" };
> > + ASSERT_TRUE(request) << "Can't create request";
> >
> > - if (request->addBuffer(stream, buffer.get()))
> > - return { Results::Fail, "Can't set buffer for request" };
> > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request";
> >
> > - if (queueRequest(request.get()) < 0)
> > - return { Results::Fail, "Failed to queue request" };
> > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request";
> >
> > requests.push_back(std::move(request));
> > }
> > @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
> > stop();
> > delete loop_;
> >
> > - if (captureCount_ != captureLimit_)
> > - return { Results::Fail, "Got " + std::to_string(captureCount_) +
> > - " request, wanted " + std::to_string(captureLimit_) };
> > -
> > - return { Results::Pass, "Balanced capture of " +
> > - std::to_string(numRequests) + " requests" };
> > + ASSERT_EQ(captureCount_, captureLimit_);
> > }
> >
> > int SimpleCaptureBalanced::queueRequest(Request *request)
> > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera)
> > {
> > }
> >
> > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> > {
> > - Results::Result ret = start();
> > - if (ret.first != Results::Pass)
> > - return ret;
> > + start();
> >
> > Stream *stream = config_->at(0).stream();
> > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> > std::vector<std::unique_ptr<libcamera::Request>> requests;
> > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > std::unique_ptr<Request> request = camera_->createRequest();
> > - if (!request)
> > - return { Results::Fail, "Can't create request" };
> > + ASSERT_TRUE(request) << "Can't create request";
> >
> > - if (request->addBuffer(stream, buffer.get()))
> > - return { Results::Fail, "Can't set buffer for request" };
> > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request";
> >
> > - if (camera_->queueRequest(request.get()) < 0)
> > - return { Results::Fail, "Failed to queue request" };
> > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
> >
> > requests.push_back(std::move(request));
> > }
> > @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> > stop();
> > delete loop_;
> >
> > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" };
> > + ASSERT_FALSE(status);
> > }
> >
> > void SimpleCaptureUnbalanced::requestComplete(Request *request)
> > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> > index d9de53fb63a3..0f8465083456 100644
> > --- a/src/lc-compliance/simple_capture.h
> > +++ b/src/lc-compliance/simple_capture.h
> > @@ -17,13 +17,13 @@
> > class SimpleCapture
> > {
> > public:
> > - Results::Result configure(libcamera::StreamRole role);
> > + void configure(libcamera::StreamRole role);
> >
> > protected:
> > SimpleCapture(std::shared_ptr<libcamera::Camera> camera);
> > virtual ~SimpleCapture();
> >
> > - Results::Result start();
> > + void start();
> > void stop();
> >
> > virtual void requestComplete(libcamera::Request *request) = 0;
> > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture
> > public:
> > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera);
> >
> > - Results::Result capture(unsigned int numRequests);
> > + void capture(unsigned int numRequests);
> >
> > private:
> > int queueRequest(libcamera::Request *request);
> > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture
> > public:
> > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera);
> >
> > - Results::Result capture(unsigned int numRequests);
> > + void capture(unsigned int numRequests);
> >
> > private:
> > void requestComplete(libcamera::Request *request) override;
> > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp
> > index 8318b42f42d6..aad32ecd051e 100644
> > --- a/src/lc-compliance/single_stream.cpp
> > +++ b/src/lc-compliance/single_stream.cpp
> > @@ -7,91 +7,95 @@
> >
> > #include <iostream>
> >
> > +#include <gtest/gtest.h>
> > +
> > #include "simple_capture.h"
> > #include "tests.h"
> >
> > using namespace libcamera;
> >
> > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
> > - StreamRole role, unsigned int startCycles,
> > - unsigned int numRequests)
> > +const std::vector<int> NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89};
> > +const std::vector<StreamRole> ROLES = {Raw, StillCapture, VideoRecording, Viewfinder};
> > +
> > +class FixedRequestsTest :
> > + public testing::TestWithParam<std::tuple<StreamRole, int>> {
> > +};
> > +
> > +std::string nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType>& info)
> > {
> > + std::map<StreamRole, std::string> rolesMap = {{Raw, "raw"},
> > + {StillCapture, "still"},
> > + {VideoRecording, "video"},
> > + {Viewfinder, "viewfinder"}};
> > +
> > + std::string roleName = rolesMap[std::get<0>(info.param)];
> > + std::string numRequestsName = std::to_string(std::get<1>(info.param));
> > +
> > + return roleName + numRequestsName;
>
> Are there some guidelines or recommendations around gtest to these
> names? I really like that you make it possible to name each one in a
> more human readable format. While testing this series I noticed tests
> like BalancedMultiCycle/video2 and incorrectly thought "This is testing
> the V4L2 device /dev/video2" not this is testing the video recording
> role with 2 requests.
There aren't any guidelines, but there are big restrictions on the names.
Namely, they should be valid C++ identifiers.
Take this for example:
RolesAndRequests/FixedRequestsTest.BalancedSingleCycle/raw1
"RolesAndRequests" is the instantiation name (since different instantiations of
the same test suite using different parameter ranges are possible). It is
appended with a "/".
"FixedRequestsTest" is the test suite. It is appended with a ".".
"BalancedSingleCycle" is the test case. It is appended with a "/".
"raw1" is the custom string I returned to represent the parameters for this
particular parametrized test case.
The complete test name reported is all of those joined with those special
characters in-between. But internally they form identifiers, so each one should
be a valid identifier.
>
> Would it make sens to name it BalancedMultiCycle/VideoRecording/2 ?
Which means this isn't possible unfortunately. We could however use '_' to
separate the parameter values:
VideoRecording_2
to get:
RolesAndRequests/FixedRequestsTest.BalancedSingleCycle/VideoRecording_2
Thanks,
Nícolas
>
> I fear this could turn into a bikeshedding discussion and we can always
> work it on-top. But if others have ideas about naming please speak up. I
> think for example in the CrOS test suite uses '.' as separator instead
> of '/'? So maybe there are some best-practices to be found.
>
> > +}
> > +
> > +/*
> > + * Test single capture cycles
> > + *
> > + * Makes sure the camera completes the exact number of requests queued. Example
> > + * failure is a camera that needs N+M requests queued to complete N requests to
> > + * the application.
> > + */
> > +TEST_P(FixedRequestsTest, BalancedSingleCycle)
> > +{
> > + auto [role, numRequests] = GetParam();
> > + std::shared_ptr<Camera> camera = Environment::instance()->camera();
> > +
> > SimpleCaptureBalanced capture(camera);
> >
> > - Results::Result ret = capture.configure(role);
> > - if (ret.first != Results::Pass)
> > - return ret;
> > + capture.configure(role);
> >
> > - for (unsigned int starts = 0; starts < startCycles; starts++) {
> > - ret = capture.capture(numRequests);
> > - if (ret.first != Results::Pass)
> > - return ret;
> > - }
> > + capture.capture(numRequests);
> > +};
> >
> > - return { Results::Pass, "Balanced capture of " +
> > - std::to_string(numRequests) + " requests with " +
> > - std::to_string(startCycles) + " start cycles" };
> > -}
> > +/*
> > + * Test multiple start/stop cycles
> > + *
> > + * Makes sure the camera supports multiple start/stop cycles. Example failure is
> > + * a camera that does not clean up correctly in its error path but is only
> > + * tested by single-capture applications.
> > + */
> > +TEST_P(FixedRequestsTest, BalancedMultiCycle)
> > +{
> > + auto [role, numRequests] = GetParam();
> > + std::shared_ptr<Camera> camera = Environment::instance()->camera();
> > + unsigned int numRepeats = 3;
> > +
> > + SimpleCaptureBalanced capture(camera);
> >
> > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> > - StreamRole role, unsigned int numRequests)
> > + capture.configure(role);
> > +
> > + for (unsigned int starts = 0; starts < numRepeats; starts++)
> > + capture.capture(numRequests);
> > +};
> > +
> > +/*
> > + * Test unbalanced stop
> > + *
> > + * Makes sure the camera supports a stop with requests queued. Example failure
> > + * is a camera that does not handle cancelation of buffers coming back from the
> > + * video device while stopping.
> > + */
> > +TEST_P(FixedRequestsTest, Unbalanced)
> > {
> > + auto [role, numRequests] = GetParam();
> > + std::shared_ptr<Camera> camera = Environment::instance()->camera();
> > +
> > SimpleCaptureUnbalanced capture(camera);
> >
> > - Results::Result ret = capture.configure(role);
> > - if (ret.first != Results::Pass)
> > - return ret;
> > + capture.configure(role);
> >
> > - return capture.capture(numRequests);
> > -}
> > + capture.capture(numRequests);
> > +};
> >
> > -Results testSingleStream(std::shared_ptr<Camera> camera)
> > -{
> > - static const std::vector<std::pair<std::string, StreamRole>> roles = {
> > - { "raw", Raw },
> > - { "still", StillCapture },
> > - { "video", VideoRecording },
> > - { "viewfinder", Viewfinder },
> > - };
> > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
> > -
> > - Results results(numRequests.size() * roles.size() * 3);
> > -
> > - for (const auto &role : roles) {
> > - std::cout << "= Test role " << role.first << std::endl;
> > - /*
> > - * Test single capture cycles
> > - *
> > - * Makes sure the camera completes the exact number of requests queued.
> > - * Example failure is a camera that needs N+M requests queued to
> > - * complete N requests to the application.
> > - */
> > - std::cout << "* Test single capture cycles" << std::endl;
> > - for (unsigned int num : numRequests)
> > - results.add(testRequestBalance(camera, role.second, 1, num));
> > -
> > - /*
> > - * Test multiple start/stop cycles
> > - *
> > - * Makes sure the camera supports multiple start/stop cycles.
> > - * Example failure is a camera that does not clean up correctly in its
> > - * error path but is only tested by single-capture applications.
> > - */
> > - std::cout << "* Test multiple start/stop cycles" << std::endl;
> > - for (unsigned int num : numRequests)
> > - results.add(testRequestBalance(camera, role.second, 3, num));
> > -
> > - /*
> > - * Test unbalanced stop
> > - *
> > - * Makes sure the camera supports a stop with requests queued.
> > - * Example failure is a camera that does not handle cancelation
> > - * of buffers coming back from the video device while stopping.
> > - */
> > - std::cout << "* Test unbalanced stop" << std::endl;
> > - for (unsigned int num : numRequests)
> > - results.add(testRequestUnbalance(camera, role.second, num));
> > - }
> > -
> > - return results;
> > -}
> > +
> > +INSTANTIATE_TEST_SUITE_P(RolesAndRequests,
> > + FixedRequestsTest,
> > + testing::Combine(testing::ValuesIn(ROLES),
> > + testing::ValuesIn(NUMREQUESTS)),
> > + nameParameters);
> > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h
> > index 396605214e4b..076785a7185f 100644
> > --- a/src/lc-compliance/tests.h
> > +++ b/src/lc-compliance/tests.h
> > @@ -11,6 +11,23 @@
> >
> > #include "results.h"
> >
> > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera);
> > +using namespace libcamera;
> > +
> > +class Environment
> > +{
> > +public:
> > + static bool create(std::shared_ptr<Camera> camera);
> > + void destroy();
> > +
> > + static Environment *instance();
> > +
> > + std::shared_ptr<Camera> camera() const { return camera_; }
> > +
> > +private:
> > + Environment() {};
> > +
> > + static Environment *instance_;
> > + static std::shared_ptr<Camera> camera_;
> > +};
> >
> > #endif /* __LC_COMPLIANCE_TESTS_H__ */
> > --
> > 2.31.1
> >
>
> --
> Regards,
> Niklas Söderlund
More information about the libcamera-devel
mailing list