[libcamera-devel] [RFC PATCH v2 3/3] lc-compliance: Refactor using Googletest
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu May 6 10:48:16 CEST 2021
Hi Nícolas,
Thanks for your work!
I really like how gtest helps make the actual tests to be easier to
read.
On 2021-05-03 16:33:07 -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 v2:
> - Changed from static to dynamic test registration
> - Removed -c flag
>
> There's still an issue with the refactoring that the shared_ptr of the tests
> apparently aren't being deleted on time, which causes:
>
> [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use
> [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use
> Segmentation fault (core dumped)
>
> I tried explicitly resetting the shared_ptr on the destructor of the tests but
> that didn't work. In fact, when just listing the tests, the constructor for the
> tests isn't even called. So I think the issue has to do with the passing of the
> camera shared pointer to the REGISTER_TESTS() macro through that functor (?):
>
> [=]() -> testsuite* { return new testcase(c, r, rq); }); \
>
> Not sure how to solve this. Any tip would be welcome.
I will leave this for now as I have a different worry about
REGISTER_TESTS() design below.
>
> src/lc-compliance/main.cpp | 99 +++++++++---------
> 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 | 151 ++++++++++++++-------------
> src/lc-compliance/tests.h | 15 ++-
> 6 files changed, 175 insertions(+), 175 deletions(-)
>
> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> index 54cee54aa978..add0d7729aec 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"
> @@ -17,7 +19,6 @@
> using namespace libcamera;
>
> enum {
> - OptCamera = 'c',
> OptHelp = 'h',
> };
>
> @@ -28,14 +29,15 @@ public:
> ~Harness();
>
> int exec();
> + int init();
> + void registerTests();
>
> private:
> - int init();
> void listCameras();
>
> OptionsParser::Options options_;
> std::unique_ptr<CameraManager> cm_;
> - std::shared_ptr<Camera> camera_;
> + std::vector<std::shared_ptr<Camera>> cameras_;
> };
>
> Harness::Harness(const OptionsParser::Options &options)
> @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options)
>
> Harness::~Harness()
> {
> - if (camera_) {
> - camera_->release();
> - camera_.reset();
> + for (auto &c : cameras_) {
> + c->release();
> + c.reset();
> }
>
> 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()
> {
> int ret = cm_->start();
> @@ -82,42 +65,26 @@ int Harness::init()
> return ret;
> }
>
> - if (!options_.isSet(OptCamera)) {
> - std::cout << "No camera specified, available cameras:" << std::endl;
> - listCameras();
> - return -ENODEV;
> - }
> -
> - const std::string &cameraId = options_[OptCamera];
> - camera_ = cm_->get(cameraId);
> - if (!camera_) {
> - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> - listCameras();
> - return -ENODEV;
> + for (auto cam : cm_->cameras()) {
> + if (cam->acquire()) {
> + std::cout << "Failed to acquire camera" << std::endl;
> + return -EINVAL;
> + }
> + cameras_.push_back(cam);
I don't like this, I think we need to retain the ability to select which
camera to test. And further down the line maybe allow more then one
camera to be selected if we want to test concurrent use of two specific
ones. But for the work in this series I think you can ignore multiple
cameras.
If I understand the coverletter correctly the correct thing here the cli
arguments should be feed to gtest directly to be able to filter on what
tests are run. Is there support in getst to also pass arguments to
tests? Maybe we could remove the need to parse arguments ourself
completely?
> }
>
> - if (camera_->acquire()) {
> - std::cout << "Failed to acquire camera" << std::endl;
> - return -EINVAL;
> - }
> -
> - std::cout << "Using camera " << cameraId << std::endl;
> -
> return 0;
> }
>
> void Harness::listCameras()
> {
> - for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> - std::cout << "- " << cam.get()->id() << std::endl;
> + for (const std::shared_ptr<Camera> &c : cm_->cameras())
> + std::cout << "- " << c.get()->id() << std::endl;
> }
>
> static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
> {
> OptionsParser parser;
> - parser.addOption(OptCamera, OptionString,
> - "Specify which camera to operate on, by id", "camera",
> - ArgumentRequired, "camera");
> parser.addOption(OptHelp, OptionNone, "Display this help message",
> "help");
>
> @@ -133,6 +100,31 @@ 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);
> + }
> + }
> +};
> +
> +
> +void Harness::registerTests() {
> + std::map<StreamRole, std::string> roles = {{Raw, "Raw"},
> + {StillCapture, "Still"},
> + {VideoRecording, "Video"},
> + {Viewfinder, "Viewfinder"}};
> + std::vector<int> requests = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89};
> +
> + registerSingleStreamTests(cameras_, roles, requests);
> +}
> +
> int main(int argc, char **argv)
> {
> OptionsParser::Options options;
> @@ -143,6 +135,13 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
>
> Harness harness(options);
> + ret = harness.init();
> + if (ret)
> + return ret;
> +
> + harness.registerTests();
>
> - 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..eb6a6f305826 100644
> --- a/src/lc-compliance/single_stream.cpp
> +++ b/src/lc-compliance/single_stream.cpp
> @@ -7,91 +7,94 @@
>
> #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)
> -{
> - SimpleCaptureBalanced capture(camera);
> +class SingleStream : public testing::Test {
> +public:
> + explicit SingleStream(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests)
> + : camera_(camera), role_(role), numRequests_(numRequests) {}
>
> - Results::Result ret = capture.configure(role);
> - if (ret.first != Results::Pass)
> - return ret;
> +protected:
> + std::shared_ptr<Camera> camera_;
> + StreamRole role_;
> + unsigned int numRequests_;
> +};
>
> - for (unsigned int starts = 0; starts < startCycles; starts++) {
> - ret = capture.capture(numRequests);
> - if (ret.first != Results::Pass)
> - return ret;
> - }
> +/*
> + * 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.
> + */
> +class BalancedSingleCycle : public SingleStream {
> +public:
> + explicit BalancedSingleCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests)
> + : SingleStream(camera, role, numRequests) {}
>
> - return { Results::Pass, "Balanced capture of " +
> - std::to_string(numRequests) + " requests with " +
> - std::to_string(startCycles) + " start cycles" };
> -}
> + void TestBody() override {
> + SimpleCaptureBalanced capture(camera_);
>
> -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> - StreamRole role, unsigned int numRequests)
> -{
> - SimpleCaptureUnbalanced capture(camera);
> + capture.configure(role_);
>
> - Results::Result ret = capture.configure(role);
> - if (ret.first != Results::Pass)
> - return ret;
> + capture.capture(numRequests_);
> + };
> +};
>
> - return capture.capture(numRequests);
> -}
> +/*
> + * 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.
> + */
> +class BalancedMultiCycle : public SingleStream {
> +public:
> + explicit BalancedMultiCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests)
> + : SingleStream(camera, role, 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 },
> + void TestBody() override {
> + unsigned int numRepeats = 3;
> +
> + SimpleCaptureBalanced capture(camera_);
> +
> + capture.configure(role_);
> +
> + for (unsigned int starts = 0; starts < numRepeats; starts++)
> + capture.capture(numRequests_);
> };
> - 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;
> +};
> +
> +/*
> + * 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.
> + */
> +class Unbalanced : public SingleStream {
> +public:
> + explicit Unbalanced(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests)
> + : SingleStream(camera, role, numRequests) {}
> +
> + void TestBody() override {
> + SimpleCaptureUnbalanced capture(camera_);
> +
> + capture.configure(role_);
> +
> + capture.capture(numRequests_);
> + };
> +};
> +
> +void registerSingleStreamTests(std::vector<std::shared_ptr<Camera>> cameras,
> + std::map<StreamRole, std::string> roles, std::vector<int> requests)
> +{
> + REGISTER_TESTS(SingleStream, BalancedSingleCycle, cameras, roles, requests);
> + REGISTER_TESTS(SingleStream, BalancedMultiCycle, cameras, roles, requests);
> + REGISTER_TESTS(SingleStream, Unbalanced, cameras, roles, requests);
> }
> diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h
> index 396605214e4b..6d5a8b88c287 100644
> --- a/src/lc-compliance/tests.h
> +++ b/src/lc-compliance/tests.h
> @@ -11,6 +11,19 @@
>
> #include "results.h"
>
> -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera);
> +void registerSingleStreamTests(std::vector<std::shared_ptr<libcamera::Camera>> cameras,
> + std::map<libcamera::StreamRole, std::string> roles, std::vector<int> requests);
> +
> +#define REGISTER_TESTS(testsuite, testcase, cameras, roles, requests) \
> +for (auto c : cameras) { \
> + for (auto [r, rn] : roles) { \
> + for (auto rq : requests) { \
> + testing::RegisterTest( \
> + #testsuite, (std::string(#testcase) + "/" + c->id() + "/" + rn + "/" + std::to_string(rq)).c_str(), \
> + nullptr, nullptr, __FILE__, __LINE__, \
> + [=]() -> testsuite* { return new testcase(c, r, rq); }); \
> + } \
> + } \
> +}
>
> #endif /* __LC_COMPLIANCE_TESTS_H__ */
> --
> 2.31.1
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list