[libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using Googletest
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Apr 29 13:48:33 CEST 2021
Hello Nícolas,
Thanks for your work.
On 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote:
> Refactor lc-compliance using Googletest as the test framework.
I really like the gtest rework from a first reading of this patch.
Unfortunately it no longer applies to master so I have not been able to
take it for a test drive.
I like that we get most if not all features we discussed such as skip
and test suite/case selection form the CLI more or less out-for-the box
for free.
Some random comments below.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
> src/lc-compliance/main.cpp | 60 +++++------
> src/lc-compliance/meson.build | 3 +
> src/lc-compliance/simple_capture.cpp | 73 +++++---------
> src/lc-compliance/simple_capture.h | 8 +-
> src/lc-compliance/single_stream.cpp | 142 ++++++++++++++-------------
> 5 files changed, 132 insertions(+), 154 deletions(-)
>
> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> index 54cee54aa978..04cd0732ee3d 100644
> --- a/src/lc-compliance/main.cpp
> +++ b/src/lc-compliance/main.cpp
> @@ -14,8 +14,12 @@
> #include "../cam/options.h"
> #include "tests.h"
>
> +#include "gtest/gtest.h"
As gtest is pulled in as an dependably and not added to the libcamera
tee should this not be <gtest/gtest.h> ?
> +
> using namespace libcamera;
>
> +std::shared_ptr<Camera> cam;
> +
Would it be possible to factor the code so that the Camera instance is
not a global variable? I'm thinking that this will not scale when tests
requiring more then one camera are needed.
> enum {
> OptCamera = 'c',
> OptHelp = 'h',
> @@ -28,14 +32,13 @@ public:
> ~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,33 +49,14 @@ Harness::Harness(const OptionsParser::Options &options)
>
> Harness::~Harness()
> {
> - if (camera_) {
> - camera_->release();
> - camera_.reset();
> + if (cam) {
> + cam->release();
> + cam.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();
> @@ -89,14 +73,14 @@ int Harness::init()
> }
>
> const std::string &cameraId = options_[OptCamera];
> - camera_ = cm_->get(cameraId);
> - if (!camera_) {
> + cam = cm_->get(cameraId);
> + if (!cam) {
> std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> listCameras();
> return -ENODEV;
> }
>
> - if (camera_->acquire()) {
> + if (cam->acquire()) {
> std::cout << "Failed to acquire camera" << std::endl;
> return -EINVAL;
> }
> @@ -108,8 +92,8 @@ int Harness::init()
>
> 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)
> @@ -133,6 +117,17 @@ 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 */
nit:
/*
* Multieline comments start
* with a blank line.
*/
> +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);
> + }
> + }
> +};
Indentation looks odd here.
> +
> int main(int argc, char **argv)
> {
> OptionsParser::Options options;
> @@ -143,6 +138,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..4c41ce6da43a 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', main: true)
> +
> lc_compliance = executable('lc-compliance', lc_compliance_sources,
> dependencies : [
> libatomic,
> libcamera_dep,
> libevent,
> + gtest_dep
Appaned a trailing ','. This is done to prepare for the next time this
file needs to be edited as the diff will then be easier to read as it
will only touch new lines.
> ],
> install : true)
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index e6715ac07f12..24abbff138a3 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -6,6 +6,7 @@
> */
>
> #include "simple_capture.h"
> +#include "gtest/gtest.h"
>
> using namespace libcamera;
>
> @@ -20,35 +21,29 @@ SimpleCapture::~SimpleCapture()
> stop();
> }
>
> -Results::Result SimpleCapture::configure(StreamRole role)
> +void SimpleCapture::configure(StreamRole role)
> {
> config_ = camera_->generateConfiguration({ role });
>
> 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";
I like these asserts instead of the return type I used in the initial
version ;-)
>
> - 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()
> @@ -73,22 +68,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;
> @@ -99,17 +91,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));
> }
> @@ -120,12 +106,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)
> @@ -157,11 +138,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);
> @@ -173,17 +152,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));
> }
> @@ -194,7 +167,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..18830cd9bce5 100644
> --- a/src/lc-compliance/single_stream.cpp
> +++ b/src/lc-compliance/single_stream.cpp
> @@ -6,92 +6,94 @@
> */
>
> #include <iostream>
> +#include <sstream>
>
> #include "simple_capture.h"
> #include "tests.h"
> +#include "gtest/gtest.h"
>
> using namespace libcamera;
>
> -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
> - StreamRole role, unsigned int startCycles,
> - unsigned int numRequests)
> +extern std::shared_ptr<Camera> cam;
> +
> +std::vector<int> NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89};
> +
> +class FixedRequestsTest :
> + public testing::TestWithParam<std::tuple<StreamRole, int>> {
> +};
> +
> +/*
> + * 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)
> {
> - SimpleCaptureBalanced capture(camera);
> + auto [role, numRequests] = GetParam();
>
> - Results::Result ret = capture.configure(role);
> - if (ret.first != Results::Pass)
> - return ret;
> + SimpleCaptureBalanced capture(cam);
>
> - for (unsigned int starts = 0; starts < startCycles; starts++) {
> - ret = capture.capture(numRequests);
> - if (ret.first != Results::Pass)
> - return ret;
> - }
> + capture.configure(role);
>
> - return { Results::Pass, "Balanced capture of " +
> - std::to_string(numRequests) + " requests with " +
> - std::to_string(startCycles) + " start cycles" };
> + capture.capture(numRequests);
> }
>
> -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> - StreamRole role, unsigned int 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.
> + */
> +TEST_P(FixedRequestsTest, BalancedMultiCycle)
> {
> - SimpleCaptureUnbalanced capture(camera);
> + auto [role, numRequests] = GetParam();
> + unsigned int numRepeats = 3;
>
> - Results::Result ret = capture.configure(role);
> - if (ret.first != Results::Pass)
> - return ret;
> + SimpleCaptureBalanced capture(cam);
>
> - return capture.capture(numRequests);
> + capture.configure(role);
> +
> + for (unsigned int starts = 0; starts < numRepeats; starts++)
> + capture.capture(numRequests);
> }
>
> -Results testSingleStream(std::shared_ptr<Camera> camera)
> +/*
> + * 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)
> {
> - 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;
> + auto [role, numRequests] = GetParam();
> +
> + SimpleCaptureUnbalanced capture(cam);
> +
> + capture.configure(role);
> +
> + capture.capture(numRequests);
> }
> +
> +INSTANTIATE_TEST_SUITE_P(Raw,
> + FixedRequestsTest,
> + testing::Combine(testing::Values(Raw),
> + testing::ValuesIn(NUMREQUESTS)));
> +
> +INSTANTIATE_TEST_SUITE_P(StillCapture,
> + FixedRequestsTest,
> + testing::Combine(testing::Values(StillCapture),
> + testing::ValuesIn(NUMREQUESTS)));
> +
> +INSTANTIATE_TEST_SUITE_P(VideoRecording,
> + FixedRequestsTest,
> + testing::Combine(testing::Values(VideoRecording),
> + testing::ValuesIn(NUMREQUESTS)));
> +
> +INSTANTIATE_TEST_SUITE_P(Viewfinder,
> + FixedRequestsTest,
> + testing::Combine(testing::Values(Viewfinder),
> + testing::ValuesIn(NUMREQUESTS)));
> --
> 2.31.1
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list