[libcamera-devel,v4,4/5] lc-compliance: Refactor using Googletest

12349 diff mbox series

Refactor lc-compliance using Googletest as the test framework.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
Changes in v4:
- Removed old lc-compliance results classes
- Thanks to Niklas:
  - Improved naming of tests

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           |  71 +++++++------
 src/lc-compliance/meson.build        |   4 +-
 src/lc-compliance/results.cpp        |  75 --------------
 src/lc-compliance/results.h          |  47 ---------
 src/lc-compliance/simple_capture.cpp |  74 +++++--------
 src/lc-compliance/simple_capture.h   |   9 +-
 src/lc-compliance/single_stream.cpp  | 150 ++++++++++++++-------------
 src/lc-compliance/tests.h            |  16 ---
 8 files changed, 152 insertions(+), 294 deletions(-)
 delete mode 100644 src/lc-compliance/results.cpp
 delete mode 100644 src/lc-compliance/results.h
 delete mode 100644 src/lc-compliance/tests.h

Hi Nícolas,

Thanks for your effort!

On 2021-05-21 10:30:53 -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@collabora.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
> Changes in v4:
> - Removed old lc-compliance results classes
> - Thanks to Niklas:
>   - Improved naming of tests
> 
> 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           |  71 +++++++------
>  src/lc-compliance/meson.build        |   4 +-
>  src/lc-compliance/results.cpp        |  75 --------------
>  src/lc-compliance/results.h          |  47 ---------
>  src/lc-compliance/simple_capture.cpp |  74 +++++--------
>  src/lc-compliance/simple_capture.h   |   9 +-
>  src/lc-compliance/single_stream.cpp  | 150 ++++++++++++++-------------
>  src/lc-compliance/tests.h            |  16 ---
>  8 files changed, 152 insertions(+), 294 deletions(-)
>  delete mode 100644 src/lc-compliance/results.cpp
>  delete mode 100644 src/lc-compliance/results.h
>  delete mode 100644 src/lc-compliance/tests.h
> 
> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> index 54cee54aa978..37884ff70a69 100644
> --- a/src/lc-compliance/main.cpp
> +++ b/src/lc-compliance/main.cpp
> @@ -9,10 +9,12 @@
>  #include <iostream>
>  #include <string.h>
>  
> +#include <gtest/gtest.h>
> +
>  #include <libcamera/libcamera.h>
>  
> +#include "environment.h"
>  #include "../cam/options.h"
> -#include "tests.h"
>  
>  using namespace libcamera;
>  
> @@ -21,21 +23,34 @@ enum {
>  	OptHelp = 'h',
>  };
>  
> +/*
> + * 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);
> +		}
> +	}
> +};
> +
>  class Harness
>  {
>  public:
>  	Harness(const OptionsParser::Options &options);
>  	~Harness();
>  
> -	int exec();
> +	int init();
> +	int run(int argc, char **argv);
>  
>  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 +61,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,23 +84,34 @@ 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;
>  }
>  
> +int Harness::run(int argc, char **argv)
> +{
> +	::testing::InitGoogleTest(&argc, argv);
> +
> +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> +
> +	return RUN_ALL_TESTS();
> +}
> +
>  void Harness::listCameras()
>  {
>  	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> @@ -143,6 +149,9 @@ 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;
> +	return harness.run(argc, argv);
>  }
> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> index c287017575bd..ae68844ac9bd 100644
> --- a/src/lc-compliance/meson.build
> +++ b/src/lc-compliance/meson.build
> @@ -13,16 +13,18 @@ lc_compliance_sources = files([
>      '../cam/event_loop.cpp',
>      '../cam/options.cpp',
>      'main.cpp',
> -    'results.cpp',
>      'environment.cpp',
>      'simple_capture.cpp',
>      '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/results.cpp b/src/lc-compliance/results.cpp
> deleted file mode 100644
> index f149f7859e28..000000000000
> --- a/src/lc-compliance/results.cpp
> +++ /dev/null
> @@ -1,75 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * results.cpp - Test result aggregator
> - */
> -
> -#include "results.h"
> -
> -#include <iostream>
> -
> -void Results::add(const Result &result)
> -{
> -	if (result.first == Pass)
> -		passed_++;
> -	else if (result.first == Fail)
> -		failed_++;
> -	else if (result.first == Skip)
> -		skipped_++;
> -
> -	printResult(result);
> -}
> -
> -void Results::add(Status status, const std::string &message)
> -{
> -	add({ status, message });
> -}
> -
> -void Results::fail(const std::string &message)
> -{
> -	add(Fail, message);
> -}
> -
> -void Results::pass(const std::string &message)
> -{
> -	add(Pass, message);
> -}
> -
> -void Results::skip(const std::string &message)
> -{
> -	add(Skip, message);
> -}
> -
> -int Results::summary() const
> -{
> -	if (failed_ + passed_ + skipped_ != planned_) {
> -		std::cout << "Planned and executed number of tests differ "
> -			  << failed_ + passed_ + skipped_ << " executed "
> -			  << planned_ << " planned" << std::endl;
> -
> -		return -EINVAL;
> -	}
> -
> -	std::cout << planned_ << " tests executed, "
> -		  << passed_ << " tests passed, "
> -		  << skipped_ << " tests skipped and "
> -		  << failed_ << " tests failed " << std::endl;
> -
> -	return 0;
> -}
> -
> -void Results::printResult(const Result &result)
> -{
> -	std::string prefix;
> -
> -	/* \todo Make parsable as TAP. */
> -	if (result.first == Pass)
> -		prefix = "PASS";
> -	else if (result.first == Fail)
> -		prefix = "FAIL";
> -	else if (result.first == Skip)
> -		prefix = "SKIP";
> -
> -	std::cout << "- " << prefix << ": " << result.second << std::endl;
> -}
> diff --git a/src/lc-compliance/results.h b/src/lc-compliance/results.h
> deleted file mode 100644
> index 2a3722b841a6..000000000000
> --- a/src/lc-compliance/results.h
> +++ /dev/null
> @@ -1,47 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * results.h - Test result aggregator
> - */
> -#ifndef __LC_COMPLIANCE_RESULTS_H__
> -#define __LC_COMPLIANCE_RESULTS_H__
> -
> -#include <string>
> -#include <utility>
> -
> -/* \todo Check if result aggregator can be shared with self tests in test/ */
> -class Results
> -{
> -public:
> -	enum Status {
> -		Fail,
> -		Pass,
> -		Skip,
> -	};
> -
> -	using Result = std::pair<Status, std::string>;
> -
> -	Results(unsigned int planned)
> -		: planned_(planned), passed_(0), failed_(0), skipped_(0)
> -	{
> -	}
> -
> -	void add(const Result &result);
> -	void add(Status status, const std::string &message);
> -	void fail(const std::string &message);
> -	void pass(const std::string &message);
> -	void skip(const std::string &message);
> -
> -	int summary() const;
> -
> -private:
> -	void printResult(const Result &result);
> -
> -	unsigned int planned_;
> -	unsigned int passed_;
> -	unsigned int failed_;
> -	unsigned int skipped_;
> -};
> -
> -#endif /* __LC_COMPLIANCE_RESULTS_H__ */
> 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..62984243a1fa 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -12,18 +12,17 @@
>  #include <libcamera/libcamera.h>
>  
>  #include "../cam/event_loop.h"
> -#include "results.h"
>  
>  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 +39,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 +55,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..a8ae2f0e355b 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 "environment.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, "StillCapture"},
> +						      {VideoRecording, "VideoRecording"},
> +						      {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;
> +}
> +
> +/*
> + * 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
> deleted file mode 100644
> index 396605214e4b..000000000000
> --- a/src/lc-compliance/tests.h
> +++ /dev/null
> @@ -1,16 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * tests.h - Test modules
> - */
> -#ifndef __LC_COMPLIANCE_TESTS_H__
> -#define __LC_COMPLIANCE_TESTS_H__
> -
> -#include <libcamera/libcamera.h>
> -
> -#include "results.h"
> -
> -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera);
> -
> -#endif /* __LC_COMPLIANCE_TESTS_H__ */
> -- 
> 2.31.1
>

Hi Nícolas,

Thank you for the patch.

On Fri, May 21, 2021 at 10:30:53AM -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@collabora.com>
> ---
> Changes in v4:
> - Removed old lc-compliance results classes
> - Thanks to Niklas:
>   - Improved naming of tests
> 
> 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           |  71 +++++++------
>  src/lc-compliance/meson.build        |   4 +-
>  src/lc-compliance/results.cpp        |  75 --------------
>  src/lc-compliance/results.h          |  47 ---------
>  src/lc-compliance/simple_capture.cpp |  74 +++++--------
>  src/lc-compliance/simple_capture.h   |   9 +-
>  src/lc-compliance/single_stream.cpp  | 150 ++++++++++++++-------------
>  src/lc-compliance/tests.h            |  16 ---
>  8 files changed, 152 insertions(+), 294 deletions(-)
>  delete mode 100644 src/lc-compliance/results.cpp
>  delete mode 100644 src/lc-compliance/results.h
>  delete mode 100644 src/lc-compliance/tests.h
> 
> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> index 54cee54aa978..37884ff70a69 100644
> --- a/src/lc-compliance/main.cpp
> +++ b/src/lc-compliance/main.cpp
> @@ -9,10 +9,12 @@
>  #include <iostream>
>  #include <string.h>
>  
> +#include <gtest/gtest.h>
> +
>  #include <libcamera/libcamera.h>
>  
> +#include "environment.h"
>  #include "../cam/options.h"
> -#include "tests.h"
>  
>  using namespace libcamera;
>  
> @@ -21,21 +23,34 @@ enum {
>  	OptHelp = 'h',
>  };
>  
> +/*
> + * 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 {

The { should go on the next line.

> +	void OnTestPartResult(const testing::TestPartResult& result) override {

Same here. And s/& / &/ (as well as in another place below).

> +		if (result.type() == testing::TestPartResult::kFatalFailure
> +		    || result.type() == testing::TestPartResult::kSkip) {

And the || at the end of the previous line.

> +			throw testing::AssertionException(result);
> +		}

No need for braces here.

> +	}
> +};
> +
>  class Harness
>  {
>  public:
>  	Harness(const OptionsParser::Options &options);
>  	~Harness();
>  
> -	int exec();
> +	int init();
> +	int run(int argc, char **argv);
>  
>  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 +61,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,23 +84,34 @@ 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;
>  }
>  
> +int Harness::run(int argc, char **argv)
> +{
> +	::testing::InitGoogleTest(&argc, argv);

If we were to make it possible to select a camera through command line
arguments, how would that work ?

> +
> +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> +
> +	return RUN_ALL_TESTS();
> +}
> +
>  void Harness::listCameras()
>  {
>  	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> @@ -143,6 +149,9 @@ 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;
> +	return harness.run(argc, argv);
>  }
> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> index c287017575bd..ae68844ac9bd 100644
> --- a/src/lc-compliance/meson.build
> +++ b/src/lc-compliance/meson.build
> @@ -13,16 +13,18 @@ lc_compliance_sources = files([
>      '../cam/event_loop.cpp',
>      '../cam/options.cpp',
>      'main.cpp',
> -    'results.cpp',
>      'environment.cpp',
>      'simple_capture.cpp',
>      'single_stream.cpp',
>  ])
>  
> +gtest_dep = dependency('gtest')

I'd name this libgtest. The reason why libcamera_dep has a dep suffix is
because libcamera is the shared library itself and libcamera_dep the
dependency object. We may want to rename other dependencies with a dep
suffix, but for now, I'd stick to the existing naming scheme.

> +
>  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
>                              dependencies : [
>                                  libatomic,
>                                  libcamera_dep,
>                                  libevent,
> +                                gtest_dep,

Alphabetical order please.

>                              ],
>                              install : true)
> diff --git a/src/lc-compliance/results.cpp b/src/lc-compliance/results.cpp
> deleted file mode 100644
> index f149f7859e28..000000000000
> --- a/src/lc-compliance/results.cpp
> +++ /dev/null
> @@ -1,75 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * results.cpp - Test result aggregator
> - */
> -
> -#include "results.h"
> -
> -#include <iostream>
> -
> -void Results::add(const Result &result)
> -{
> -	if (result.first == Pass)
> -		passed_++;
> -	else if (result.first == Fail)
> -		failed_++;
> -	else if (result.first == Skip)
> -		skipped_++;
> -
> -	printResult(result);
> -}
> -
> -void Results::add(Status status, const std::string &message)
> -{
> -	add({ status, message });
> -}
> -
> -void Results::fail(const std::string &message)
> -{
> -	add(Fail, message);
> -}
> -
> -void Results::pass(const std::string &message)
> -{
> -	add(Pass, message);
> -}
> -
> -void Results::skip(const std::string &message)
> -{
> -	add(Skip, message);
> -}
> -
> -int Results::summary() const
> -{
> -	if (failed_ + passed_ + skipped_ != planned_) {
> -		std::cout << "Planned and executed number of tests differ "
> -			  << failed_ + passed_ + skipped_ << " executed "
> -			  << planned_ << " planned" << std::endl;
> -
> -		return -EINVAL;
> -	}
> -
> -	std::cout << planned_ << " tests executed, "
> -		  << passed_ << " tests passed, "
> -		  << skipped_ << " tests skipped and "
> -		  << failed_ << " tests failed " << std::endl;
> -
> -	return 0;
> -}
> -
> -void Results::printResult(const Result &result)
> -{
> -	std::string prefix;
> -
> -	/* \todo Make parsable as TAP. */
> -	if (result.first == Pass)
> -		prefix = "PASS";
> -	else if (result.first == Fail)
> -		prefix = "FAIL";
> -	else if (result.first == Skip)
> -		prefix = "SKIP";
> -
> -	std::cout << "- " << prefix << ": " << result.second << std::endl;
> -}
> diff --git a/src/lc-compliance/results.h b/src/lc-compliance/results.h
> deleted file mode 100644
> index 2a3722b841a6..000000000000
> --- a/src/lc-compliance/results.h
> +++ /dev/null
> @@ -1,47 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * results.h - Test result aggregator
> - */
> -#ifndef __LC_COMPLIANCE_RESULTS_H__
> -#define __LC_COMPLIANCE_RESULTS_H__
> -
> -#include <string>
> -#include <utility>
> -
> -/* \todo Check if result aggregator can be shared with self tests in test/ */
> -class Results
> -{
> -public:
> -	enum Status {
> -		Fail,
> -		Pass,
> -		Skip,
> -	};
> -
> -	using Result = std::pair<Status, std::string>;
> -
> -	Results(unsigned int planned)
> -		: planned_(planned), passed_(0), failed_(0), skipped_(0)
> -	{
> -	}
> -
> -	void add(const Result &result);
> -	void add(Status status, const std::string &message);
> -	void fail(const std::string &message);
> -	void pass(const std::string &message);
> -	void skip(const std::string &message);
> -
> -	int summary() const;
> -
> -private:
> -	void printResult(const Result &result);
> -
> -	unsigned int planned_;
> -	unsigned int passed_;
> -	unsigned int failed_;
> -	unsigned int skipped_;
> -};
> -
> -#endif /* __LC_COMPLIANCE_RESULTS_H__ */
> 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..62984243a1fa 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -12,18 +12,17 @@
>  #include <libcamera/libcamera.h>
>  
>  #include "../cam/event_loop.h"
> -#include "results.h"
>  
>  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 +39,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 +55,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..a8ae2f0e355b 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 "environment.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>> {

No line break after :, but a line break before {

> +};
> +
> +std::string nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType>& info)

Could this be a static function of the FixedRequestsTest class ?

>  {
> +	std::map<StreamRole, std::string> rolesMap = {{Raw, "Raw"},
> +						      {StillCapture, "StillCapture"},
> +						      {VideoRecording, "VideoRecording"},
> +						      {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;
> +}
> +
> +/*
> + * 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);

Not that it's required, but because the TEST_P macro creates a class
that inherits from FixedRequestsTest, you could also put some of the
code in the FixedRequestsTest class (either as helper functions, or even
the complete implementation of the tests).

I'm also wondering if it the FixedRequestsTest class shouldn't be merged
with the SimpleCapture* classes, but that's a question for later I
suppose.

> +};
>  
> -	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
> deleted file mode 100644
> index 396605214e4b..000000000000
> --- a/src/lc-compliance/tests.h
> +++ /dev/null
> @@ -1,16 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * tests.h - Test modules
> - */
> -#ifndef __LC_COMPLIANCE_TESTS_H__
> -#define __LC_COMPLIANCE_TESTS_H__
> -
> -#include <libcamera/libcamera.h>
> -
> -#include "results.h"
> -
> -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera);
> -
> -#endif /* __LC_COMPLIANCE_TESTS_H__ */

Hi Laurent,

Em 2021-05-23 19:01, Laurent Pinchart escreveu:

> Hi Nícolas,
>
> Thank you for the patch.
>
> On Fri, May 21, 2021 at 10:30:53AM -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@collabora.com>
> > ---
> > Changes in v4:
> > - Removed old lc-compliance results classes
> > - Thanks to Niklas:
> >   - Improved naming of tests
> > 
> > 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           |  71 +++++++------
> >  src/lc-compliance/meson.build        |   4 +-
> >  src/lc-compliance/results.cpp        |  75 --------------
> >  src/lc-compliance/results.h          |  47 ---------
> >  src/lc-compliance/simple_capture.cpp |  74 +++++--------
> >  src/lc-compliance/simple_capture.h   |   9 +-
> >  src/lc-compliance/single_stream.cpp  | 150 ++++++++++++++-------------
> >  src/lc-compliance/tests.h            |  16 ---
> >  8 files changed, 152 insertions(+), 294 deletions(-)
> >  delete mode 100644 src/lc-compliance/results.cpp
> >  delete mode 100644 src/lc-compliance/results.h
> >  delete mode 100644 src/lc-compliance/tests.h
> > 
> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > index 54cee54aa978..37884ff70a69 100644
> > --- a/src/lc-compliance/main.cpp
> > +++ b/src/lc-compliance/main.cpp
> > @@ -9,10 +9,12 @@
> >  #include <iostream>
> >  #include <string.h>
> >  
> > +#include <gtest/gtest.h>
> > +
> >  #include <libcamera/libcamera.h>
> >  
> > +#include "environment.h"
> >  #include "../cam/options.h"
> > -#include "tests.h"
> >  
> >  using namespace libcamera;
> >  
> > @@ -21,21 +23,34 @@ enum {
> >  	OptHelp = 'h',
> >  };
> >  
> > +/*
> > + * 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 {
>
> The { should go on the next line.
>
> > +	void OnTestPartResult(const testing::TestPartResult& result) override {
>
> Same here. And s/& / &/ (as well as in another place below).
>
> > +		if (result.type() == testing::TestPartResult::kFatalFailure
> > +		    || result.type() == testing::TestPartResult::kSkip) {
>
> And the || at the end of the previous line.
>
> > +			throw testing::AssertionException(result);
> > +		}
>
> No need for braces here.

Turns out I wasn't using checkstyle.py, and having copied this code from Gtest's
documentation, there were some style differences. I've addressed these and some
others pointed out by checkstyle.py. (Although there seem to be some
false-positives which I'll ignore based on common sense).

>
> > +	}
> > +};
> > +
> >  class Harness
> >  {
> >  public:
> >  	Harness(const OptionsParser::Options &options);
> >  	~Harness();
> >  
> > -	int exec();
> > +	int init();
> > +	int run(int argc, char **argv);
> >  
> >  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 +61,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,23 +84,34 @@ 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;
> >  }
> >  
> > +int Harness::run(int argc, char **argv)
> > +{
> > +	::testing::InitGoogleTest(&argc, argv);
>
> If we were to make it possible to select a camera through command line
> arguments, how would that work ?

That's already possible actually using the --camera flag that was already in
place. Gtest ignores flags that don't start with "gtest_". In any case, patch 5
changes some of these same lines to add the "--list" and "--filter" flags, and
while at it, makes some changes so that only gtest flags are passed to gtest.

>
> > +
> > +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> > +
> > +	return RUN_ALL_TESTS();
> > +}
> > +
> >  void Harness::listCameras()
> >  {
> >  	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > @@ -143,6 +149,9 @@ 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;
> > +	return harness.run(argc, argv);
> >  }
> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > index c287017575bd..ae68844ac9bd 100644
> > --- a/src/lc-compliance/meson.build
> > +++ b/src/lc-compliance/meson.build
> > @@ -13,16 +13,18 @@ lc_compliance_sources = files([
> >      '../cam/event_loop.cpp',
> >      '../cam/options.cpp',
> >      'main.cpp',
> > -    'results.cpp',
> >      'environment.cpp',
> >      'simple_capture.cpp',
> >      'single_stream.cpp',
> >  ])
> >  
> > +gtest_dep = dependency('gtest')
>
> I'd name this libgtest. The reason why libcamera_dep has a dep suffix is
> because libcamera is the shared library itself and libcamera_dep the
> dependency object. We may want to rename other dependencies with a dep
> suffix, but for now, I'd stick to the existing naming scheme.

Sounds good.

>
> > +
> >  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> >                              dependencies : [
> >                                  libatomic,
> >                                  libcamera_dep,
> >                                  libevent,
> > +                                gtest_dep,
>
> Alphabetical order please.

Luckily, changing it to libgtest makes it be in alphabetical order :P.

>
> >                              ],
> >                              install : true)
> > diff --git a/src/lc-compliance/results.cpp b/src/lc-compliance/results.cpp
> > deleted file mode 100644
> > index f149f7859e28..000000000000
> > --- a/src/lc-compliance/results.cpp
> > +++ /dev/null
> > @@ -1,75 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > -/*
> > - * Copyright (C) 2020, Google Inc.
> > - *
> > - * results.cpp - Test result aggregator
> > - */
> > -
> > -#include "results.h"
> > -
> > -#include <iostream>
> > -
> > -void Results::add(const Result &result)
> > -{
> > -	if (result.first == Pass)
> > -		passed_++;
> > -	else if (result.first == Fail)
> > -		failed_++;
> > -	else if (result.first == Skip)
> > -		skipped_++;
> > -
> > -	printResult(result);
> > -}
> > -
> > -void Results::add(Status status, const std::string &message)
> > -{
> > -	add({ status, message });
> > -}
> > -
> > -void Results::fail(const std::string &message)
> > -{
> > -	add(Fail, message);
> > -}
> > -
> > -void Results::pass(const std::string &message)
> > -{
> > -	add(Pass, message);
> > -}
> > -
> > -void Results::skip(const std::string &message)
> > -{
> > -	add(Skip, message);
> > -}
> > -
> > -int Results::summary() const
> > -{
> > -	if (failed_ + passed_ + skipped_ != planned_) {
> > -		std::cout << "Planned and executed number of tests differ "
> > -			  << failed_ + passed_ + skipped_ << " executed "
> > -			  << planned_ << " planned" << std::endl;
> > -
> > -		return -EINVAL;
> > -	}
> > -
> > -	std::cout << planned_ << " tests executed, "
> > -		  << passed_ << " tests passed, "
> > -		  << skipped_ << " tests skipped and "
> > -		  << failed_ << " tests failed " << std::endl;
> > -
> > -	return 0;
> > -}
> > -
> > -void Results::printResult(const Result &result)
> > -{
> > -	std::string prefix;
> > -
> > -	/* \todo Make parsable as TAP. */
> > -	if (result.first == Pass)
> > -		prefix = "PASS";
> > -	else if (result.first == Fail)
> > -		prefix = "FAIL";
> > -	else if (result.first == Skip)
> > -		prefix = "SKIP";
> > -
> > -	std::cout << "- " << prefix << ": " << result.second << std::endl;
> > -}
> > diff --git a/src/lc-compliance/results.h b/src/lc-compliance/results.h
> > deleted file mode 100644
> > index 2a3722b841a6..000000000000
> > --- a/src/lc-compliance/results.h
> > +++ /dev/null
> > @@ -1,47 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > -/*
> > - * Copyright (C) 2020, Google Inc.
> > - *
> > - * results.h - Test result aggregator
> > - */
> > -#ifndef __LC_COMPLIANCE_RESULTS_H__
> > -#define __LC_COMPLIANCE_RESULTS_H__
> > -
> > -#include <string>
> > -#include <utility>
> > -
> > -/* \todo Check if result aggregator can be shared with self tests in test/ */
> > -class Results
> > -{
> > -public:
> > -	enum Status {
> > -		Fail,
> > -		Pass,
> > -		Skip,
> > -	};
> > -
> > -	using Result = std::pair<Status, std::string>;
> > -
> > -	Results(unsigned int planned)
> > -		: planned_(planned), passed_(0), failed_(0), skipped_(0)
> > -	{
> > -	}
> > -
> > -	void add(const Result &result);
> > -	void add(Status status, const std::string &message);
> > -	void fail(const std::string &message);
> > -	void pass(const std::string &message);
> > -	void skip(const std::string &message);
> > -
> > -	int summary() const;
> > -
> > -private:
> > -	void printResult(const Result &result);
> > -
> > -	unsigned int planned_;
> > -	unsigned int passed_;
> > -	unsigned int failed_;
> > -	unsigned int skipped_;
> > -};
> > -
> > -#endif /* __LC_COMPLIANCE_RESULTS_H__ */
> > 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..62984243a1fa 100644
> > --- a/src/lc-compliance/simple_capture.h
> > +++ b/src/lc-compliance/simple_capture.h
> > @@ -12,18 +12,17 @@
> >  #include <libcamera/libcamera.h>
> >  
> >  #include "../cam/event_loop.h"
> > -#include "results.h"
> >  
> >  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 +39,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 +55,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..a8ae2f0e355b 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 "environment.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>> {
>
> No line break after :, but a line break before {
>
> > +};
> > +
> > +std::string nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType>& info)
>
> Could this be a static function of the FixedRequestsTest class ?

Yes. Done.

>
> >  {
> > +	std::map<StreamRole, std::string> rolesMap = {{Raw, "Raw"},
> > +						      {StillCapture, "StillCapture"},
> > +						      {VideoRecording, "VideoRecording"},
> > +						      {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;
> > +}
> > +
> > +/*
> > + * 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);
>
> Not that it's required, but because the TEST_P macro creates a class
> that inherits from FixedRequestsTest, you could also put some of the
> code in the FixedRequestsTest class (either as helper functions, or even
> the complete implementation of the tests).

I'm not sure the tests are similar or long enough yet for helper functions to be
that useful. But we certainly should have that in mind as more tests are added.

That said, in order to have the camera be acquired by each individual test as
you suggested in the review of patch 3, I'm already adding a SetUp() function
in the base class to be used by all tests to acquire the camera automatically at
startup.

>
> I'm also wondering if it the FixedRequestsTest class shouldn't be merged
> with the SimpleCapture* classes, but that's a question for later I
> suppose.

Hm, my impression is that those functions are more generic and could be useful
for all test suites, so makes sense to be out in its own class, but I guess time
will tell.

Thanks,
Nícolas

>
> > +};
> >  
> > -	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
> > deleted file mode 100644
> > index 396605214e4b..000000000000
> > --- a/src/lc-compliance/tests.h
> > +++ /dev/null
> > @@ -1,16 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > -/*
> > - * Copyright (C) 2020, Google Inc.
> > - *
> > - * tests.h - Test modules
> > - */
> > -#ifndef __LC_COMPLIANCE_TESTS_H__
> > -#define __LC_COMPLIANCE_TESTS_H__
> > -
> > -#include <libcamera/libcamera.h>
> > -
> > -#include "results.h"
> > -
> > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera);
> > -
> > -#endif /* __LC_COMPLIANCE_TESTS_H__ */
>
> --
> Regards,
>
> Laurent Pinchart
>
> --
> To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.