[libcamera-devel] [PATCH v3 3/4] lc-compliance: Refactor using Googletest
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun May 16 16:01:20 CEST 2021
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.
> +
> 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.
Would it make sens to name it BalancedMultiCycle/VideoRecording/2 ?
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