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

12713 diff mbox series

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 v9:
- Thanks to Jacopo:
  - Removed the Harness class, substituting methods with static helper functions
  - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()
  - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()
  - Changed tests naming:
    - TestSuite instance: RolesAndRequests -> CaptureTests
    - TestSuite: FixedRequestsTest -> SingleStream
    - TestCase: BalancedSingleCycle -> Capture
    - TestCase: BalancedMultiCycle -> CaptureStartStop
    - TestCase: Unbalanced -> UnbalancedStop
  - Renamed single_stream.cpp to capture_test.cpp

Changes in v8:
- Thanks to Laurent:
  - Fixed lc-compliance's meson.build to disable when gtest is not installed
  - Fixed compiling error in Clang

Changes in v7:
- Thanks to Jacopo:
  - Made CameraManager a unique_ptr and passed to Environment as raw pointer

No changes in v6

Changes in v5:
- Thanks to Laurent:
  - Fixed style issues
  - Added SetUp and TearDown functions to acquire and release the camera for
    each test

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/capture_test.cpp   | 127 +++++++++++++++++++++++++++
 src/lc-compliance/main.cpp           | 106 +++++++++-------------
 src/lc-compliance/meson.build        |   7 +-
 src/lc-compliance/results.cpp        |  75 ----------------
 src/lc-compliance/results.h          |  47 ----------
 src/lc-compliance/simple_capture.cpp |  77 +++++++---------
 src/lc-compliance/simple_capture.h   |   9 +-
 src/lc-compliance/single_stream.cpp  |  97 --------------------
 src/lc-compliance/tests.h            |  16 ----
 9 files changed, 206 insertions(+), 355 deletions(-)
 create mode 100644 src/lc-compliance/capture_test.cpp
 delete mode 100644 src/lc-compliance/results.cpp
 delete mode 100644 src/lc-compliance/results.h
 delete mode 100644 src/lc-compliance/single_stream.cpp
 delete mode 100644 src/lc-compliance/tests.h

Hi Nicolas,

   first of all: this is a great job! The introduction of
   lc-compliance in kernel-ci is an amazing step forward, thanks a lot
   for getting to this point so quickly!

On Fri, Jun 25, 2021 at 04:39:24PM -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 v9:
> - Thanks to Jacopo:
>   - Removed the Harness class, substituting methods with static helper functions
>   - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()
>   - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()
>   - Changed tests naming:
>     - TestSuite instance: RolesAndRequests -> CaptureTests
>     - TestSuite: FixedRequestsTest -> SingleStream
>     - TestCase: BalancedSingleCycle -> Capture
>     - TestCase: BalancedMultiCycle -> CaptureStartStop
>     - TestCase: Unbalanced -> UnbalancedStop
>   - Renamed single_stream.cpp to capture_test.cpp
>
> Changes in v8:
> - Thanks to Laurent:
>   - Fixed lc-compliance's meson.build to disable when gtest is not installed
>   - Fixed compiling error in Clang
>
> Changes in v7:
> - Thanks to Jacopo:
>   - Made CameraManager a unique_ptr and passed to Environment as raw pointer
>
> No changes in v6
>
> Changes in v5:
> - Thanks to Laurent:
>   - Fixed style issues
>   - Added SetUp and TearDown functions to acquire and release the camera for
>     each test
>
> 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/capture_test.cpp   | 127 +++++++++++++++++++++++++++
>  src/lc-compliance/main.cpp           | 106 +++++++++-------------
>  src/lc-compliance/meson.build        |   7 +-
>  src/lc-compliance/results.cpp        |  75 ----------------
>  src/lc-compliance/results.h          |  47 ----------
>  src/lc-compliance/simple_capture.cpp |  77 +++++++---------
>  src/lc-compliance/simple_capture.h   |   9 +-
>  src/lc-compliance/single_stream.cpp  |  97 --------------------
>  src/lc-compliance/tests.h            |  16 ----
>  9 files changed, 206 insertions(+), 355 deletions(-)
>  create mode 100644 src/lc-compliance/capture_test.cpp
>  delete mode 100644 src/lc-compliance/results.cpp
>  delete mode 100644 src/lc-compliance/results.h
>  delete mode 100644 src/lc-compliance/single_stream.cpp
>  delete mode 100644 src/lc-compliance/tests.h
>
> diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp
> new file mode 100644
> index 000000000000..116ae496fe04
> --- /dev/null
> +++ b/src/lc-compliance/capture_test.cpp
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.

Aaaah, time flies! Could you check the year for the other files too ?

> + *
> + * single_stream.cpp - Test a single camera stream
> + */
> +
> +#include <iostream>
> +
> +#include <gtest/gtest.h>
> +
> +#include "environment.h"
> +#include "simple_capture.h"

We tend to include the file header first to make sure it's
self-contained.

> +
> +using namespace libcamera;
> +
> +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 SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> +{
> +public:
> +	static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> +
> +protected:
> +	void SetUp() override;
> +	void TearDown() override;
> +
> +	std::shared_ptr<Camera> camera_;
> +};
> +
> +/*
> + * We use gtest's SetUp() and TearDown() instead of constructor and destructor
> + * in order to be able to assert on them.
> + */
> +void SingleStream::SetUp()
> +{
> +	Environment *env = Environment::get();
> +
> +	camera_ = env->cm()->get(env->cameraId());
> +
> +	ASSERT_EQ(camera_->acquire(), 0);
> +}
> +
> +void SingleStream::TearDown()
> +{
> +	if (!camera_)
> +		return;
> +
> +	camera_->release();
> +	camera_.reset();
> +}
> +
> +std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::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.

I'm not sure I get the last statement (probably I had the same comment on the
previous version :) is "M" the queue depth ? Honestly I would drop it.

> + */
> +TEST_P(SingleStream, Capture)
> +{
> +	auto [role, numRequests] = GetParam();
> +
> +	SimpleCaptureBalanced capture(camera_);
> +
> +	capture.configure(role);
> +
> +	capture.capture(numRequests);
> +}
> +
> +/*
> + * Test multiple start/stop cycles
> + *
> + * Makes sure the camera supports multiple start/stop cycles. Example failure is
> + * a camera that does not clean up correctly in its error path but is only
> + * tested by single-capture applications.
> + */
> +TEST_P(SingleStream, CaptureStartStop)
> +{
> +	auto [role, numRequests] = GetParam();
> +	unsigned int numRepeats = 3;
> +
> +	SimpleCaptureBalanced capture(camera_);
> +
> +	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(SingleStream, UnbalancedStop)
> +{
> +	auto [role, numRequests] = GetParam();
> +
> +	SimpleCaptureUnbalanced capture(camera_);
> +
> +	capture.configure(role);
> +
> +	capture.capture(numRequests);
> +}
> +
> +INSTANTIATE_TEST_SUITE_P(CaptureTests,
> +			 SingleStream,
> +			 testing::Combine(testing::ValuesIn(ROLES),
> +					  testing::ValuesIn(NUMREQUESTS)),
> +			 SingleStream::nameParameters);
> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {
>  	OptHelp = 'h',
>  };
>
> -class Harness
> +/*
> + * 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
>  {
> -public:
> -	Harness(const OptionsParser::Options &options);
> -	~Harness();
> -
> -	int exec();
> -
> -private:
> -	int init();
> -	void listCameras();
> -
> -	OptionsParser::Options options_;
> -	std::unique_ptr<CameraManager> cm_;
> -	std::shared_ptr<Camera> camera_;
> +	void OnTestPartResult(const testing::TestPartResult &result) override
> +	{
> +		if (result.type() == testing::TestPartResult::kFatalFailure ||
> +		    result.type() == testing::TestPartResult::kSkip)
> +			throw testing::AssertionException(result);
> +	}
>  };
>
> -Harness::Harness(const OptionsParser::Options &options)
> -	: options_(options)
> -{
> -	cm_ = std::make_unique<CameraManager>();
> -}
> -
> -Harness::~Harness()
> +static void listCameras(CameraManager *cm)
>  {
> -	if (camera_) {
> -		camera_->release();
> -		camera_.reset();
> -	}
> -
> -	cm_->stop();
> +	for (const std::shared_ptr<Camera> &cam : cm->cameras())
> +		std::cout << "- " << cam.get()->id() << std::endl;
>  }
>
> -int Harness::exec()
> +static int initCamera(CameraManager *cm, OptionsParser::Options options)
>  {
> -	int ret = init();
> -	if (ret)
> -		return ret;
> -
> -	std::vector<Results> results;
> +	std::shared_ptr<Camera> camera;
>
> -	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();
> +	int ret = cm->start();
>  	if (ret) {
>  		std::cout << "Failed to start camera manager: "
>  			  << strerror(-ret) << std::endl;
>  		return ret;
>  	}
>
> -	if (!options_.isSet(OptCamera)) {
> +	if (!options.isSet(OptCamera)) {
>  		std::cout << "No camera specified, available cameras:" << std::endl;
> -		listCameras();
> +		listCameras(cm);
>  		return -ENODEV;
>  	}
>
> -	const std::string &cameraId = options_[OptCamera];
> -	camera_ = cm_->get(cameraId);
> -	if (!camera_) {
> +	const std::string &cameraId = options[OptCamera];
> +	camera = cm->get(cameraId);
> +	if (!camera) {
>  		std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> -		listCameras();
> +		listCameras(cm);
>  		return -ENODEV;
>  	}
>
> -	if (camera_->acquire()) {
> -		std::cout << "Failed to acquire camera" << std::endl;
> -		return -EINVAL;
> -	}
> +	Environment::get()->setup(cm, cameraId);
>
>  	std::cout << "Using camera " << cameraId << std::endl;
>
>  	return 0;
>  }
>
> -void Harness::listCameras()
> -{
> -	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> -		std::cout << "- " << cam.get()->id() << std::endl;
> -}
> -
>  static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
>  {
>  	OptionsParser parser;
> @@ -142,7 +106,17 @@ int main(int argc, char **argv)
>  	if (ret < 0)
>  		return EXIT_FAILURE;
>
> -	Harness harness(options);
> +	std::unique_ptr<CameraManager> cm = std::make_unique<CameraManager>();
> +
> +	ret = initCamera(cm.get(), options);
> +	if (ret)
> +		return ret;
> +
> +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> +
> +	ret = RUN_ALL_TESTS();
> +
> +	cm->stop();
>
> -	return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;
> +	return ret;
>  }
> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> index 6dd49085569f..f3a79ae438a9 100644
> --- a/src/lc-compliance/meson.build
> +++ b/src/lc-compliance/meson.build
> @@ -1,8 +1,9 @@
>  # SPDX-License-Identifier: CC0-1.0
>
>  libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> +libgtest = dependency('gtest', required : get_option('lc-compliance'))
>
> -if not libevent.found()
> +if not (libevent.found() and libgtest.found())
>      lc_compliance_enabled = false
>      subdir_done()
>  endif
> @@ -14,9 +15,8 @@ lc_compliance_sources = files([
>      '../cam/options.cpp',
>      'environment.cpp',
>      'main.cpp',
> -    'results.cpp',
>      'simple_capture.cpp',
> -    'single_stream.cpp',
> +    'capture_test.cpp',
>  ])
>
>  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> @@ -24,5 +24,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,
>                                  libatomic,
>                                  libcamera_dep,
>                                  libevent,
> +                                libgtest,

After the recent split of libcamera-base this does not apply anymore.
Rebase is trivial, but much easier for you since you have a branch.
I'm afraid this would require a v10 to make things simpler on our side
(fixing this is trivial, but I have conflicts also when applying the
next patch and that would require manually applying the diff :(

>                              ],
>                              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 bfc91b26444e..ce0883672709 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,37 @@ 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" };
> +	int count = allocator_->allocate(stream);
>
> -	if (camera_->start())
> -		return { Results::Fail, "Failed to start camera" };
> +	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> +	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
>
> -	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> +	ASSERT_TRUE(!camera_->start()) << "Failed to start camera";
>
> -	return { Results::Pass, "Started camera" };
> +	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);

Shouldn't the signal be connected before starting the camera ?

>  }
>
>  void SimpleCapture::stop()
> @@ -74,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;
> @@ -100,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";

ASSERT_EQ(.., 0)

>
> -		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));
>  	}
> @@ -118,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)
> @@ -155,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);
> @@ -171,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";

maybe it's just me being disappointed by the usage of FALSE() to test the
resuls of a function is 0 ? If that's the case just ignore the
comments on this topic :)

>
> -		if (camera_->queueRequest(request.get()) < 0)
> -			return { Results::Fail, "Failed to queue request" };
> +		ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request";

ASSERT_EQ() ?

All minors, I like this new setup and the naming scheme (at least I
agree with myself time to time :) I'm sure we'll refactor later as you
suggested but for now that's fine.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


>
>  		requests.push_back(std::move(request));
>  	}
> @@ -189,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
> deleted file mode 100644
> index 8318b42f42d6..000000000000
> --- a/src/lc-compliance/single_stream.cpp
> +++ /dev/null
> @@ -1,97 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * single_stream.cpp - Test a single camera stream
> - */
> -
> -#include <iostream>
> -
> -#include "simple_capture.h"
> -#include "tests.h"
> -
> -using namespace libcamera;
> -
> -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
> -				   StreamRole role, unsigned int startCycles,
> -				   unsigned int numRequests)
> -{
> -	SimpleCaptureBalanced capture(camera);
> -
> -	Results::Result ret = capture.configure(role);
> -	if (ret.first != Results::Pass)
> -		return ret;
> -
> -	for (unsigned int starts = 0; starts < startCycles; starts++) {
> -		ret = capture.capture(numRequests);
> -		if (ret.first != Results::Pass)
> -			return ret;
> -	}
> -
> -	return { Results::Pass, "Balanced capture of " +
> -		std::to_string(numRequests) + " requests with " +
> -		std::to_string(startCycles) + " start cycles" };
> -}
> -
> -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> -				     StreamRole role, unsigned int numRequests)
> -{
> -	SimpleCaptureUnbalanced capture(camera);
> -
> -	Results::Result ret = capture.configure(role);
> -	if (ret.first != Results::Pass)
> -		return ret;
> -
> -	return 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;
> -}
> 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.32.0
>

Hi Nicolas,

On Fri, Jun 25, 2021 at 04:39:24PM -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 v9:
> - Thanks to Jacopo:
>   - Removed the Harness class, substituting methods with static helper functions
>   - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()
>   - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()
>   - Changed tests naming:
>     - TestSuite instance: RolesAndRequests -> CaptureTests
>     - TestSuite: FixedRequestsTest -> SingleStream
>     - TestCase: BalancedSingleCycle -> Capture
>     - TestCase: BalancedMultiCycle -> CaptureStartStop
>     - TestCase: Unbalanced -> UnbalancedStop
>   - Renamed single_stream.cpp to capture_test.cpp
> 
> Changes in v8:
> - Thanks to Laurent:
>   - Fixed lc-compliance's meson.build to disable when gtest is not installed
>   - Fixed compiling error in Clang
> 
> Changes in v7:
> - Thanks to Jacopo:
>   - Made CameraManager a unique_ptr and passed to Environment as raw pointer
> 
> No changes in v6
> 
> Changes in v5:
> - Thanks to Laurent:
>   - Fixed style issues
>   - Added SetUp and TearDown functions to acquire and release the camera for
>     each test
> 
> 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/capture_test.cpp   | 127 +++++++++++++++++++++++++++
>  src/lc-compliance/main.cpp           | 106 +++++++++-------------
>  src/lc-compliance/meson.build        |   7 +-
>  src/lc-compliance/results.cpp        |  75 ----------------
>  src/lc-compliance/results.h          |  47 ----------
>  src/lc-compliance/simple_capture.cpp |  77 +++++++---------
>  src/lc-compliance/simple_capture.h   |   9 +-
>  src/lc-compliance/single_stream.cpp  |  97 --------------------
>  src/lc-compliance/tests.h            |  16 ----
>  9 files changed, 206 insertions(+), 355 deletions(-)
>  create mode 100644 src/lc-compliance/capture_test.cpp
>  delete mode 100644 src/lc-compliance/results.cpp
>  delete mode 100644 src/lc-compliance/results.h
>  delete mode 100644 src/lc-compliance/single_stream.cpp
>  delete mode 100644 src/lc-compliance/tests.h
> 
> diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp
> new file mode 100644
> index 000000000000..116ae496fe04
> --- /dev/null
> +++ b/src/lc-compliance/capture_test.cpp
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * single_stream.cpp - Test a single camera stream
> + */
> +
> +#include <iostream>
> +
> +#include <gtest/gtest.h>
> +
> +#include "environment.h"
> +#include "simple_capture.h"
> +
> +using namespace libcamera;
> +
> +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 SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> +{
> +public:
> +	static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> +
> +protected:
> +	void SetUp() override;
> +	void TearDown() override;
> +
> +	std::shared_ptr<Camera> camera_;
> +};
> +
> +/*
> + * We use gtest's SetUp() and TearDown() instead of constructor and destructor
> + * in order to be able to assert on them.
> + */
> +void SingleStream::SetUp()
> +{
> +	Environment *env = Environment::get();
> +
> +	camera_ = env->cm()->get(env->cameraId());
> +
> +	ASSERT_EQ(camera_->acquire(), 0);
> +}
> +
> +void SingleStream::TearDown()
> +{
> +	if (!camera_)
> +		return;
> +
> +	camera_->release();
> +	camera_.reset();
> +}
> +
> +std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::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(SingleStream, Capture)
> +{
> +	auto [role, numRequests] = GetParam();
> +
> +	SimpleCaptureBalanced capture(camera_);
> +
> +	capture.configure(role);
> +
> +	capture.capture(numRequests);
> +}
> +
> +/*
> + * Test multiple start/stop cycles
> + *
> + * Makes sure the camera supports multiple start/stop cycles. Example failure is
> + * a camera that does not clean up correctly in its error path but is only
> + * tested by single-capture applications.
> + */
> +TEST_P(SingleStream, CaptureStartStop)
> +{
> +	auto [role, numRequests] = GetParam();
> +	unsigned int numRepeats = 3;
> +
> +	SimpleCaptureBalanced capture(camera_);
> +
> +	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(SingleStream, UnbalancedStop)
> +{
> +	auto [role, numRequests] = GetParam();
> +
> +	SimpleCaptureUnbalanced capture(camera_);
> +
> +	capture.configure(role);
> +
> +	capture.capture(numRequests);
> +}
> +
> +INSTANTIATE_TEST_SUITE_P(CaptureTests,
> +			 SingleStream,
> +			 testing::Combine(testing::ValuesIn(ROLES),
> +					  testing::ValuesIn(NUMREQUESTS)),
> +			 SingleStream::nameParameters);
> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {
>  	OptHelp = 'h',
>  };
>  
> -class Harness
> +/*
> + * 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
>  {
> -public:
> -	Harness(const OptionsParser::Options &options);
> -	~Harness();
> -
> -	int exec();
> -
> -private:
> -	int init();
> -	void listCameras();
> -
> -	OptionsParser::Options options_;
> -	std::unique_ptr<CameraManager> cm_;
> -	std::shared_ptr<Camera> camera_;
> +	void OnTestPartResult(const testing::TestPartResult &result) override
> +	{
> +		if (result.type() == testing::TestPartResult::kFatalFailure ||
> +		    result.type() == testing::TestPartResult::kSkip)
> +			throw testing::AssertionException(result);

This fails compilation in cros_sdk, as exceptions are disabled. The
behavior can be reproduced outside of the cros_sdk by adding
-fno-exceptions and compiling on clang.

Adding this fixes the issue:

diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
index 0fd2aca1..aa5852f6 100644
--- a/src/lc-compliance/meson.build
+++ b/src/lc-compliance/meson.build
@@ -20,6 +20,7 @@ lc_compliance_sources = files([
 ])
 
 lc_compliance  = executable('lc-compliance', lc_compliance_sources,
+                            cpp_args : [ '-fexceptions' ],
                             dependencies : [
                                 libatomic,
                                 libcamera_public,


Paul

> +	}
>  };
>  
> -Harness::Harness(const OptionsParser::Options &options)
> -	: options_(options)
> -{
> -	cm_ = std::make_unique<CameraManager>();
> -}
> -
> -Harness::~Harness()
> +static void listCameras(CameraManager *cm)
>  {
> -	if (camera_) {
> -		camera_->release();
> -		camera_.reset();
> -	}
> -
> -	cm_->stop();
> +	for (const std::shared_ptr<Camera> &cam : cm->cameras())
> +		std::cout << "- " << cam.get()->id() << std::endl;
>  }
>  
> -int Harness::exec()
> +static int initCamera(CameraManager *cm, OptionsParser::Options options)
>  {
> -	int ret = init();
> -	if (ret)
> -		return ret;
> -
> -	std::vector<Results> results;
> +	std::shared_ptr<Camera> camera;
>  
> -	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();
> +	int ret = cm->start();
>  	if (ret) {
>  		std::cout << "Failed to start camera manager: "
>  			  << strerror(-ret) << std::endl;
>  		return ret;
>  	}
>  
> -	if (!options_.isSet(OptCamera)) {
> +	if (!options.isSet(OptCamera)) {
>  		std::cout << "No camera specified, available cameras:" << std::endl;
> -		listCameras();
> +		listCameras(cm);
>  		return -ENODEV;
>  	}
>  
> -	const std::string &cameraId = options_[OptCamera];
> -	camera_ = cm_->get(cameraId);
> -	if (!camera_) {
> +	const std::string &cameraId = options[OptCamera];
> +	camera = cm->get(cameraId);
> +	if (!camera) {
>  		std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> -		listCameras();
> +		listCameras(cm);
>  		return -ENODEV;
>  	}
>  
> -	if (camera_->acquire()) {
> -		std::cout << "Failed to acquire camera" << std::endl;
> -		return -EINVAL;
> -	}
> +	Environment::get()->setup(cm, cameraId);
>  
>  	std::cout << "Using camera " << cameraId << std::endl;
>  
>  	return 0;
>  }
>  
> -void Harness::listCameras()
> -{
> -	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> -		std::cout << "- " << cam.get()->id() << std::endl;
> -}
> -
>  static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
>  {
>  	OptionsParser parser;
> @@ -142,7 +106,17 @@ int main(int argc, char **argv)
>  	if (ret < 0)
>  		return EXIT_FAILURE;
>  
> -	Harness harness(options);
> +	std::unique_ptr<CameraManager> cm = std::make_unique<CameraManager>();
> +
> +	ret = initCamera(cm.get(), options);
> +	if (ret)
> +		return ret;
> +
> +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> +
> +	ret = RUN_ALL_TESTS();
> +
> +	cm->stop();
>  
> -	return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;
> +	return ret;
>  }
> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> index 6dd49085569f..f3a79ae438a9 100644
> --- a/src/lc-compliance/meson.build
> +++ b/src/lc-compliance/meson.build
> @@ -1,8 +1,9 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> +libgtest = dependency('gtest', required : get_option('lc-compliance'))
>  
> -if not libevent.found()
> +if not (libevent.found() and libgtest.found())
>      lc_compliance_enabled = false
>      subdir_done()
>  endif
> @@ -14,9 +15,8 @@ lc_compliance_sources = files([
>      '../cam/options.cpp',
>      'environment.cpp',
>      'main.cpp',
> -    'results.cpp',
>      'simple_capture.cpp',
> -    'single_stream.cpp',
> +    'capture_test.cpp',
>  ])
>  
>  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> @@ -24,5 +24,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,
>                                  libatomic,
>                                  libcamera_dep,
>                                  libevent,
> +                                libgtest,
>                              ],
>                              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 bfc91b26444e..ce0883672709 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,37 @@ 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" };
> +	int count = allocator_->allocate(stream);
>  
> -	if (camera_->start())
> -		return { Results::Fail, "Failed to start camera" };
> +	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> +	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
>  
> -	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> +	ASSERT_TRUE(!camera_->start()) << "Failed to start camera";
>  
> -	return { Results::Pass, "Started camera" };
> +	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
>  }
>  
>  void SimpleCapture::stop()
> @@ -74,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;
> @@ -100,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));
>  	}
> @@ -118,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)
> @@ -155,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);
> @@ -171,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));
>  	}
> @@ -189,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
> deleted file mode 100644
> index 8318b42f42d6..000000000000
> --- a/src/lc-compliance/single_stream.cpp
> +++ /dev/null
> @@ -1,97 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * single_stream.cpp - Test a single camera stream
> - */
> -
> -#include <iostream>
> -
> -#include "simple_capture.h"
> -#include "tests.h"
> -
> -using namespace libcamera;
> -
> -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
> -				   StreamRole role, unsigned int startCycles,
> -				   unsigned int numRequests)
> -{
> -	SimpleCaptureBalanced capture(camera);
> -
> -	Results::Result ret = capture.configure(role);
> -	if (ret.first != Results::Pass)
> -		return ret;
> -
> -	for (unsigned int starts = 0; starts < startCycles; starts++) {
> -		ret = capture.capture(numRequests);
> -		if (ret.first != Results::Pass)
> -			return ret;
> -	}
> -
> -	return { Results::Pass, "Balanced capture of " +
> -		std::to_string(numRequests) + " requests with " +
> -		std::to_string(startCycles) + " start cycles" };
> -}
> -
> -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> -				     StreamRole role, unsigned int numRequests)
> -{
> -	SimpleCaptureUnbalanced capture(camera);
> -
> -	Results::Result ret = capture.configure(role);
> -	if (ret.first != Results::Pass)
> -		return ret;
> -
> -	return 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;
> -}
> 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.32.0
>

Hi Jacopo,

On Sat, Jun 26, 2021 at 10:12:31AM +0200, Jacopo Mondi wrote:
> Hi Nicolas,
> 
>    first of all: this is a great job! The introduction of
>    lc-compliance in kernel-ci is an amazing step forward, thanks a lot
>    for getting to this point so quickly!

Glad you like it :)

> 
> On Fri, Jun 25, 2021 at 04:39:24PM -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 v9:
> > - Thanks to Jacopo:
> >   - Removed the Harness class, substituting methods with static helper functions
> >   - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()
> >   - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()
> >   - Changed tests naming:
> >     - TestSuite instance: RolesAndRequests -> CaptureTests
> >     - TestSuite: FixedRequestsTest -> SingleStream
> >     - TestCase: BalancedSingleCycle -> Capture
> >     - TestCase: BalancedMultiCycle -> CaptureStartStop
> >     - TestCase: Unbalanced -> UnbalancedStop
> >   - Renamed single_stream.cpp to capture_test.cpp
> >
> > Changes in v8:
> > - Thanks to Laurent:
> >   - Fixed lc-compliance's meson.build to disable when gtest is not installed
> >   - Fixed compiling error in Clang
> >
> > Changes in v7:
> > - Thanks to Jacopo:
> >   - Made CameraManager a unique_ptr and passed to Environment as raw pointer
> >
> > No changes in v6
> >
> > Changes in v5:
> > - Thanks to Laurent:
> >   - Fixed style issues
> >   - Added SetUp and TearDown functions to acquire and release the camera for
> >     each test
> >
> > 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/capture_test.cpp   | 127 +++++++++++++++++++++++++++
> >  src/lc-compliance/main.cpp           | 106 +++++++++-------------
> >  src/lc-compliance/meson.build        |   7 +-
> >  src/lc-compliance/results.cpp        |  75 ----------------
> >  src/lc-compliance/results.h          |  47 ----------
> >  src/lc-compliance/simple_capture.cpp |  77 +++++++---------
> >  src/lc-compliance/simple_capture.h   |   9 +-
> >  src/lc-compliance/single_stream.cpp  |  97 --------------------
> >  src/lc-compliance/tests.h            |  16 ----
> >  9 files changed, 206 insertions(+), 355 deletions(-)
> >  create mode 100644 src/lc-compliance/capture_test.cpp
> >  delete mode 100644 src/lc-compliance/results.cpp
> >  delete mode 100644 src/lc-compliance/results.h
> >  delete mode 100644 src/lc-compliance/single_stream.cpp
> >  delete mode 100644 src/lc-compliance/tests.h
> >
> > diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp
> > new file mode 100644
> > index 000000000000..116ae496fe04
> > --- /dev/null
> > +++ b/src/lc-compliance/capture_test.cpp
> > @@ -0,0 +1,127 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> 
> Aaaah, time flies! Could you check the year for the other files too ?

Now that you mention it, I wonder if the changes were big enough that I should
add a

    Copyright (C) 2021, Collabora Ltd.

instead on this file. Maybe even for main.cpp as well?

> 
> > + *
> > + * single_stream.cpp - Test a single camera stream
> > + */
> > +
> > +#include <iostream>
> > +
> > +#include <gtest/gtest.h>
> > +
> > +#include "environment.h"
> > +#include "simple_capture.h"
> 
> We tend to include the file header first to make sure it's
> self-contained.

This file (now named capture_test.cpp) doesn't have a header file though. But
that shows that I forgot to update the file name and description in the comment
a few lines above.

> 
> > +
> > +using namespace libcamera;
> > +
> > +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 SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> > +{
> > +public:
> > +	static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> > +
> > +protected:
> > +	void SetUp() override;
> > +	void TearDown() override;
> > +
> > +	std::shared_ptr<Camera> camera_;
> > +};
> > +
> > +/*
> > + * We use gtest's SetUp() and TearDown() instead of constructor and destructor
> > + * in order to be able to assert on them.
> > + */
> > +void SingleStream::SetUp()
> > +{
> > +	Environment *env = Environment::get();
> > +
> > +	camera_ = env->cm()->get(env->cameraId());
> > +
> > +	ASSERT_EQ(camera_->acquire(), 0);
> > +}
> > +
> > +void SingleStream::TearDown()
> > +{
> > +	if (!camera_)
> > +		return;
> > +
> > +	camera_->release();
> > +	camera_.reset();
> > +}
> > +
> > +std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::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.
> 
> I'm not sure I get the last statement (probably I had the same comment on the
> previous version :) is "M" the queue depth ? Honestly I would drop it.

I think that statement could be later improved to be less generic, but still I
see some use in it. Regardless of what M means, if there are cases where a few
extra requests are required to complete the intended amount, we should be on the
lookout for them.

> 
> > + */
> > +TEST_P(SingleStream, Capture)
> > +{
> > +	auto [role, numRequests] = GetParam();
> > +
> > +	SimpleCaptureBalanced capture(camera_);
> > +
> > +	capture.configure(role);
> > +
> > +	capture.capture(numRequests);
> > +}
> > +
> > +/*
> > + * Test multiple start/stop cycles
> > + *
> > + * Makes sure the camera supports multiple start/stop cycles. Example failure is
> > + * a camera that does not clean up correctly in its error path but is only
> > + * tested by single-capture applications.
> > + */
> > +TEST_P(SingleStream, CaptureStartStop)
> > +{
> > +	auto [role, numRequests] = GetParam();
> > +	unsigned int numRepeats = 3;
> > +
> > +	SimpleCaptureBalanced capture(camera_);
> > +
> > +	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(SingleStream, UnbalancedStop)
> > +{
> > +	auto [role, numRequests] = GetParam();
> > +
> > +	SimpleCaptureUnbalanced capture(camera_);
> > +
> > +	capture.configure(role);
> > +
> > +	capture.capture(numRequests);
> > +}
> > +
> > +INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > +			 SingleStream,
> > +			 testing::Combine(testing::ValuesIn(ROLES),
> > +					  testing::ValuesIn(NUMREQUESTS)),
> > +			 SingleStream::nameParameters);
> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {
> >  	OptHelp = 'h',
> >  };
> >
> > -class Harness
> > +/*
> > + * 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
> >  {
> > -public:
> > -	Harness(const OptionsParser::Options &options);
> > -	~Harness();
> > -
> > -	int exec();
> > -
> > -private:
> > -	int init();
> > -	void listCameras();
> > -
> > -	OptionsParser::Options options_;
> > -	std::unique_ptr<CameraManager> cm_;
> > -	std::shared_ptr<Camera> camera_;
> > +	void OnTestPartResult(const testing::TestPartResult &result) override
> > +	{
> > +		if (result.type() == testing::TestPartResult::kFatalFailure ||
> > +		    result.type() == testing::TestPartResult::kSkip)
> > +			throw testing::AssertionException(result);
> > +	}
> >  };
> >
> > -Harness::Harness(const OptionsParser::Options &options)
> > -	: options_(options)
> > -{
> > -	cm_ = std::make_unique<CameraManager>();
> > -}
> > -
> > -Harness::~Harness()
> > +static void listCameras(CameraManager *cm)
> >  {
> > -	if (camera_) {
> > -		camera_->release();
> > -		camera_.reset();
> > -	}
> > -
> > -	cm_->stop();
> > +	for (const std::shared_ptr<Camera> &cam : cm->cameras())
> > +		std::cout << "- " << cam.get()->id() << std::endl;
> >  }
> >
> > -int Harness::exec()
> > +static int initCamera(CameraManager *cm, OptionsParser::Options options)
> >  {
> > -	int ret = init();
> > -	if (ret)
> > -		return ret;
> > -
> > -	std::vector<Results> results;
> > +	std::shared_ptr<Camera> camera;
> >
> > -	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();
> > +	int ret = cm->start();
> >  	if (ret) {
> >  		std::cout << "Failed to start camera manager: "
> >  			  << strerror(-ret) << std::endl;
> >  		return ret;
> >  	}
> >
> > -	if (!options_.isSet(OptCamera)) {
> > +	if (!options.isSet(OptCamera)) {
> >  		std::cout << "No camera specified, available cameras:" << std::endl;
> > -		listCameras();
> > +		listCameras(cm);
> >  		return -ENODEV;
> >  	}
> >
> > -	const std::string &cameraId = options_[OptCamera];
> > -	camera_ = cm_->get(cameraId);
> > -	if (!camera_) {
> > +	const std::string &cameraId = options[OptCamera];
> > +	camera = cm->get(cameraId);
> > +	if (!camera) {
> >  		std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> > -		listCameras();
> > +		listCameras(cm);
> >  		return -ENODEV;
> >  	}
> >
> > -	if (camera_->acquire()) {
> > -		std::cout << "Failed to acquire camera" << std::endl;
> > -		return -EINVAL;
> > -	}
> > +	Environment::get()->setup(cm, cameraId);
> >
> >  	std::cout << "Using camera " << cameraId << std::endl;
> >
> >  	return 0;
> >  }
> >
> > -void Harness::listCameras()
> > -{
> > -	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > -		std::cout << "- " << cam.get()->id() << std::endl;
> > -}
> > -
> >  static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
> >  {
> >  	OptionsParser parser;
> > @@ -142,7 +106,17 @@ int main(int argc, char **argv)
> >  	if (ret < 0)
> >  		return EXIT_FAILURE;
> >
> > -	Harness harness(options);
> > +	std::unique_ptr<CameraManager> cm = std::make_unique<CameraManager>();
> > +
> > +	ret = initCamera(cm.get(), options);
> > +	if (ret)
> > +		return ret;
> > +
> > +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> > +
> > +	ret = RUN_ALL_TESTS();
> > +
> > +	cm->stop();
> >
> > -	return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;
> > +	return ret;
> >  }
> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > index 6dd49085569f..f3a79ae438a9 100644
> > --- a/src/lc-compliance/meson.build
> > +++ b/src/lc-compliance/meson.build
> > @@ -1,8 +1,9 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >
> >  libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> > +libgtest = dependency('gtest', required : get_option('lc-compliance'))
> >
> > -if not libevent.found()
> > +if not (libevent.found() and libgtest.found())
> >      lc_compliance_enabled = false
> >      subdir_done()
> >  endif
> > @@ -14,9 +15,8 @@ lc_compliance_sources = files([
> >      '../cam/options.cpp',
> >      'environment.cpp',
> >      'main.cpp',
> > -    'results.cpp',
> >      'simple_capture.cpp',
> > -    'single_stream.cpp',
> > +    'capture_test.cpp',
> >  ])
> >
> >  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> > @@ -24,5 +24,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> >                                  libatomic,
> >                                  libcamera_dep,
> >                                  libevent,
> > +                                libgtest,
> 
> After the recent split of libcamera-base this does not apply anymore.
> Rebase is trivial, but much easier for you since you have a branch.
> I'm afraid this would require a v10 to make things simpler on our side
> (fixing this is trivial, but I have conflicts also when applying the
> next patch and that would require manually applying the diff :(

Interesting, I didn't know a rebase was able to automatically resolve basic
conflicts while applying patches wasn't. But indeed I rebased it now on master
without any tweaks needed. Should be good for v10.

> 
> >                              ],
> >                              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 bfc91b26444e..ce0883672709 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,37 @@ 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" };
> > +	int count = allocator_->allocate(stream);
> >
> > -	if (camera_->start())
> > -		return { Results::Fail, "Failed to start camera" };
> > +	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > +	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> >
> > -	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> > +	ASSERT_TRUE(!camera_->start()) << "Failed to start camera";
> >
> > -	return { Results::Pass, "Started camera" };
> > +	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> 
> Shouldn't the signal be connected before starting the camera ?

I guess it doesn't matter as long as it's connected before any requests are
queued. But also doesn't hurt to move it :), and that makes it more symmetrical
to stop().

> 
> >  }
> >
> >  void SimpleCapture::stop()
> > @@ -74,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;
> > @@ -100,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";
> 
> ASSERT_EQ(.., 0)

Sure.

> 
> >
> > -		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));
> >  	}
> > @@ -118,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)
> > @@ -155,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);
> > @@ -171,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";
> 
> maybe it's just me being disappointed by the usage of FALSE() to test the
> resuls of a function is 0 ? If that's the case just ignore the
> comments on this topic :)

No, I do think it makes sense. I went ahead and changed some others.

Related to that, checking that pointers are valid is being done with
ASSERT_TRUE(ptr). Do you think it would be better to use ASSERT_NE(ptr, 0)
instead?

> 
> >
> > -		if (camera_->queueRequest(request.get()) < 0)
> > -			return { Results::Fail, "Failed to queue request" };
> > +		ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
> 
> ASSERT_EQ() ?

Sure.

> 
> All minors, I like this new setup and the naming scheme (at least I
> agree with myself time to time :) I'm sure we'll refactor later as you
> suggested but for now that's fine.
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks!

Nícolas

> 
> Thanks
>   j
> 
> 
> >
> >  		requests.push_back(std::move(request));
> >  	}
> > @@ -189,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
> > deleted file mode 100644
> > index 8318b42f42d6..000000000000
> > --- a/src/lc-compliance/single_stream.cpp
> > +++ /dev/null
> > @@ -1,97 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > -/*
> > - * Copyright (C) 2020, Google Inc.
> > - *
> > - * single_stream.cpp - Test a single camera stream
> > - */
> > -
> > -#include <iostream>
> > -
> > -#include "simple_capture.h"
> > -#include "tests.h"
> > -
> > -using namespace libcamera;
> > -
> > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
> > -				   StreamRole role, unsigned int startCycles,
> > -				   unsigned int numRequests)
> > -{
> > -	SimpleCaptureBalanced capture(camera);
> > -
> > -	Results::Result ret = capture.configure(role);
> > -	if (ret.first != Results::Pass)
> > -		return ret;
> > -
> > -	for (unsigned int starts = 0; starts < startCycles; starts++) {
> > -		ret = capture.capture(numRequests);
> > -		if (ret.first != Results::Pass)
> > -			return ret;
> > -	}
> > -
> > -	return { Results::Pass, "Balanced capture of " +
> > -		std::to_string(numRequests) + " requests with " +
> > -		std::to_string(startCycles) + " start cycles" };
> > -}
> > -
> > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> > -				     StreamRole role, unsigned int numRequests)
> > -{
> > -	SimpleCaptureUnbalanced capture(camera);
> > -
> > -	Results::Result ret = capture.configure(role);
> > -	if (ret.first != Results::Pass)
> > -		return ret;
> > -
> > -	return 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;
> > -}
> > 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.32.0
> >

Hi Paul,

On Mon, Jun 28, 2021 at 05:28:04PM +0900, paul.elder@ideasonboard.com wrote:
> Hi Nicolas,
> 
> On Fri, Jun 25, 2021 at 04:39:24PM -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 v9:
> > - Thanks to Jacopo:
> >   - Removed the Harness class, substituting methods with static helper functions
> >   - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()
> >   - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()
> >   - Changed tests naming:
> >     - TestSuite instance: RolesAndRequests -> CaptureTests
> >     - TestSuite: FixedRequestsTest -> SingleStream
> >     - TestCase: BalancedSingleCycle -> Capture
> >     - TestCase: BalancedMultiCycle -> CaptureStartStop
> >     - TestCase: Unbalanced -> UnbalancedStop
> >   - Renamed single_stream.cpp to capture_test.cpp
> > 
> > Changes in v8:
> > - Thanks to Laurent:
> >   - Fixed lc-compliance's meson.build to disable when gtest is not installed
> >   - Fixed compiling error in Clang
> > 
> > Changes in v7:
> > - Thanks to Jacopo:
> >   - Made CameraManager a unique_ptr and passed to Environment as raw pointer
> > 
> > No changes in v6
> > 
> > Changes in v5:
> > - Thanks to Laurent:
> >   - Fixed style issues
> >   - Added SetUp and TearDown functions to acquire and release the camera for
> >     each test
> > 
> > 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/capture_test.cpp   | 127 +++++++++++++++++++++++++++
> >  src/lc-compliance/main.cpp           | 106 +++++++++-------------
> >  src/lc-compliance/meson.build        |   7 +-
> >  src/lc-compliance/results.cpp        |  75 ----------------
> >  src/lc-compliance/results.h          |  47 ----------
> >  src/lc-compliance/simple_capture.cpp |  77 +++++++---------
> >  src/lc-compliance/simple_capture.h   |   9 +-
> >  src/lc-compliance/single_stream.cpp  |  97 --------------------
> >  src/lc-compliance/tests.h            |  16 ----
> >  9 files changed, 206 insertions(+), 355 deletions(-)
> >  create mode 100644 src/lc-compliance/capture_test.cpp
> >  delete mode 100644 src/lc-compliance/results.cpp
> >  delete mode 100644 src/lc-compliance/results.h
> >  delete mode 100644 src/lc-compliance/single_stream.cpp
> >  delete mode 100644 src/lc-compliance/tests.h
> > 
> > diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp
> > new file mode 100644
> > index 000000000000..116ae496fe04
> > --- /dev/null
> > +++ b/src/lc-compliance/capture_test.cpp
> > @@ -0,0 +1,127 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * single_stream.cpp - Test a single camera stream
> > + */
> > +
> > +#include <iostream>
> > +
> > +#include <gtest/gtest.h>
> > +
> > +#include "environment.h"
> > +#include "simple_capture.h"
> > +
> > +using namespace libcamera;
> > +
> > +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 SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> > +{
> > +public:
> > +	static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> > +
> > +protected:
> > +	void SetUp() override;
> > +	void TearDown() override;
> > +
> > +	std::shared_ptr<Camera> camera_;
> > +};
> > +
> > +/*
> > + * We use gtest's SetUp() and TearDown() instead of constructor and destructor
> > + * in order to be able to assert on them.
> > + */
> > +void SingleStream::SetUp()
> > +{
> > +	Environment *env = Environment::get();
> > +
> > +	camera_ = env->cm()->get(env->cameraId());
> > +
> > +	ASSERT_EQ(camera_->acquire(), 0);
> > +}
> > +
> > +void SingleStream::TearDown()
> > +{
> > +	if (!camera_)
> > +		return;
> > +
> > +	camera_->release();
> > +	camera_.reset();
> > +}
> > +
> > +std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::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(SingleStream, Capture)
> > +{
> > +	auto [role, numRequests] = GetParam();
> > +
> > +	SimpleCaptureBalanced capture(camera_);
> > +
> > +	capture.configure(role);
> > +
> > +	capture.capture(numRequests);
> > +}
> > +
> > +/*
> > + * Test multiple start/stop cycles
> > + *
> > + * Makes sure the camera supports multiple start/stop cycles. Example failure is
> > + * a camera that does not clean up correctly in its error path but is only
> > + * tested by single-capture applications.
> > + */
> > +TEST_P(SingleStream, CaptureStartStop)
> > +{
> > +	auto [role, numRequests] = GetParam();
> > +	unsigned int numRepeats = 3;
> > +
> > +	SimpleCaptureBalanced capture(camera_);
> > +
> > +	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(SingleStream, UnbalancedStop)
> > +{
> > +	auto [role, numRequests] = GetParam();
> > +
> > +	SimpleCaptureUnbalanced capture(camera_);
> > +
> > +	capture.configure(role);
> > +
> > +	capture.capture(numRequests);
> > +}
> > +
> > +INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > +			 SingleStream,
> > +			 testing::Combine(testing::ValuesIn(ROLES),
> > +					  testing::ValuesIn(NUMREQUESTS)),
> > +			 SingleStream::nameParameters);
> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {
> >  	OptHelp = 'h',
> >  };
> >  
> > -class Harness
> > +/*
> > + * 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
> >  {
> > -public:
> > -	Harness(const OptionsParser::Options &options);
> > -	~Harness();
> > -
> > -	int exec();
> > -
> > -private:
> > -	int init();
> > -	void listCameras();
> > -
> > -	OptionsParser::Options options_;
> > -	std::unique_ptr<CameraManager> cm_;
> > -	std::shared_ptr<Camera> camera_;
> > +	void OnTestPartResult(const testing::TestPartResult &result) override
> > +	{
> > +		if (result.type() == testing::TestPartResult::kFatalFailure ||
> > +		    result.type() == testing::TestPartResult::kSkip)
> > +			throw testing::AssertionException(result);
> 
> This fails compilation in cros_sdk, as exceptions are disabled. The
> behavior can be reproduced outside of the cros_sdk by adding
> -fno-exceptions and compiling on clang.
> 
> Adding this fixes the issue:
> 
> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> index 0fd2aca1..aa5852f6 100644
> --- a/src/lc-compliance/meson.build
> +++ b/src/lc-compliance/meson.build
> @@ -20,6 +20,7 @@ lc_compliance_sources = files([
>  ])
>  
>  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> +                            cpp_args : [ '-fexceptions' ],
>                              dependencies : [
>                                  libatomic,
>                                  libcamera_public,

Thank you for the feedback and the fix. Added for v10.

Nícolas

> 
> 
> Paul
> 
> > +	}
> >  };
> >  
> > -Harness::Harness(const OptionsParser::Options &options)
> > -	: options_(options)
> > -{
> > -	cm_ = std::make_unique<CameraManager>();
> > -}
> > -
> > -Harness::~Harness()
> > +static void listCameras(CameraManager *cm)
> >  {
> > -	if (camera_) {
> > -		camera_->release();
> > -		camera_.reset();
> > -	}
> > -
> > -	cm_->stop();
> > +	for (const std::shared_ptr<Camera> &cam : cm->cameras())
> > +		std::cout << "- " << cam.get()->id() << std::endl;
> >  }
> >  
> > -int Harness::exec()
> > +static int initCamera(CameraManager *cm, OptionsParser::Options options)
> >  {
> > -	int ret = init();
> > -	if (ret)
> > -		return ret;
> > -
> > -	std::vector<Results> results;
> > +	std::shared_ptr<Camera> camera;
> >  
> > -	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();
> > +	int ret = cm->start();
> >  	if (ret) {
> >  		std::cout << "Failed to start camera manager: "
> >  			  << strerror(-ret) << std::endl;
> >  		return ret;
> >  	}
> >  
> > -	if (!options_.isSet(OptCamera)) {
> > +	if (!options.isSet(OptCamera)) {
> >  		std::cout << "No camera specified, available cameras:" << std::endl;
> > -		listCameras();
> > +		listCameras(cm);
> >  		return -ENODEV;
> >  	}
> >  
> > -	const std::string &cameraId = options_[OptCamera];
> > -	camera_ = cm_->get(cameraId);
> > -	if (!camera_) {
> > +	const std::string &cameraId = options[OptCamera];
> > +	camera = cm->get(cameraId);
> > +	if (!camera) {
> >  		std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> > -		listCameras();
> > +		listCameras(cm);
> >  		return -ENODEV;
> >  	}
> >  
> > -	if (camera_->acquire()) {
> > -		std::cout << "Failed to acquire camera" << std::endl;
> > -		return -EINVAL;
> > -	}
> > +	Environment::get()->setup(cm, cameraId);
> >  
> >  	std::cout << "Using camera " << cameraId << std::endl;
> >  
> >  	return 0;
> >  }
> >  
> > -void Harness::listCameras()
> > -{
> > -	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > -		std::cout << "- " << cam.get()->id() << std::endl;
> > -}
> > -
> >  static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
> >  {
> >  	OptionsParser parser;
> > @@ -142,7 +106,17 @@ int main(int argc, char **argv)
> >  	if (ret < 0)
> >  		return EXIT_FAILURE;
> >  
> > -	Harness harness(options);
> > +	std::unique_ptr<CameraManager> cm = std::make_unique<CameraManager>();
> > +
> > +	ret = initCamera(cm.get(), options);
> > +	if (ret)
> > +		return ret;
> > +
> > +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> > +
> > +	ret = RUN_ALL_TESTS();
> > +
> > +	cm->stop();
> >  
> > -	return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;
> > +	return ret;
> >  }
> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > index 6dd49085569f..f3a79ae438a9 100644
> > --- a/src/lc-compliance/meson.build
> > +++ b/src/lc-compliance/meson.build
> > @@ -1,8 +1,9 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >  
> >  libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> > +libgtest = dependency('gtest', required : get_option('lc-compliance'))
> >  
> > -if not libevent.found()
> > +if not (libevent.found() and libgtest.found())
> >      lc_compliance_enabled = false
> >      subdir_done()
> >  endif
> > @@ -14,9 +15,8 @@ lc_compliance_sources = files([
> >      '../cam/options.cpp',
> >      'environment.cpp',
> >      'main.cpp',
> > -    'results.cpp',
> >      'simple_capture.cpp',
> > -    'single_stream.cpp',
> > +    'capture_test.cpp',
> >  ])
> >  
> >  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> > @@ -24,5 +24,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> >                                  libatomic,
> >                                  libcamera_dep,
> >                                  libevent,
> > +                                libgtest,
> >                              ],
> >                              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 bfc91b26444e..ce0883672709 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,37 @@ 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" };
> > +	int count = allocator_->allocate(stream);
> >  
> > -	if (camera_->start())
> > -		return { Results::Fail, "Failed to start camera" };
> > +	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > +	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> >  
> > -	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> > +	ASSERT_TRUE(!camera_->start()) << "Failed to start camera";
> >  
> > -	return { Results::Pass, "Started camera" };
> > +	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> >  }
> >  
> >  void SimpleCapture::stop()
> > @@ -74,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;
> > @@ -100,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));
> >  	}
> > @@ -118,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)
> > @@ -155,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);
> > @@ -171,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));
> >  	}
> > @@ -189,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
> > deleted file mode 100644
> > index 8318b42f42d6..000000000000
> > --- a/src/lc-compliance/single_stream.cpp
> > +++ /dev/null
> > @@ -1,97 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > -/*
> > - * Copyright (C) 2020, Google Inc.
> > - *
> > - * single_stream.cpp - Test a single camera stream
> > - */
> > -
> > -#include <iostream>
> > -
> > -#include "simple_capture.h"
> > -#include "tests.h"
> > -
> > -using namespace libcamera;
> > -
> > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
> > -				   StreamRole role, unsigned int startCycles,
> > -				   unsigned int numRequests)
> > -{
> > -	SimpleCaptureBalanced capture(camera);
> > -
> > -	Results::Result ret = capture.configure(role);
> > -	if (ret.first != Results::Pass)
> > -		return ret;
> > -
> > -	for (unsigned int starts = 0; starts < startCycles; starts++) {
> > -		ret = capture.capture(numRequests);
> > -		if (ret.first != Results::Pass)
> > -			return ret;
> > -	}
> > -
> > -	return { Results::Pass, "Balanced capture of " +
> > -		std::to_string(numRequests) + " requests with " +
> > -		std::to_string(startCycles) + " start cycles" };
> > -}
> > -
> > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> > -				     StreamRole role, unsigned int numRequests)
> > -{
> > -	SimpleCaptureUnbalanced capture(camera);
> > -
> > -	Results::Result ret = capture.configure(role);
> > -	if (ret.first != Results::Pass)
> > -		return ret;
> > -
> > -	return 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;
> > -}
> > 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.32.0
> >

Hi Nicolas,

On Mon, Jun 28, 2021 at 12:11:53PM -0300, Nícolas F. R. A. Prado wrote:
> Hi Jacopo,
>
> On Sat, Jun 26, 2021 at 10:12:31AM +0200, Jacopo Mondi wrote:
> > Hi Nicolas,
> >
> >    first of all: this is a great job! The introduction of
> >    lc-compliance in kernel-ci is an amazing step forward, thanks a lot
> >    for getting to this point so quickly!
>
> Glad you like it :)
>
> >
> > On Fri, Jun 25, 2021 at 04:39:24PM -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 v9:
> > > - Thanks to Jacopo:
> > >   - Removed the Harness class, substituting methods with static helper functions
> > >   - Added EXPECT() for the number of buffers allocated in SimpleCapture::start()
> > >   - Changed ASSERT_FALSE() to ASSERT_EQ(..., 0) in SetUp()
> > >   - Changed tests naming:
> > >     - TestSuite instance: RolesAndRequests -> CaptureTests
> > >     - TestSuite: FixedRequestsTest -> SingleStream
> > >     - TestCase: BalancedSingleCycle -> Capture
> > >     - TestCase: BalancedMultiCycle -> CaptureStartStop
> > >     - TestCase: Unbalanced -> UnbalancedStop
> > >   - Renamed single_stream.cpp to capture_test.cpp
> > >
> > > Changes in v8:
> > > - Thanks to Laurent:
> > >   - Fixed lc-compliance's meson.build to disable when gtest is not installed
> > >   - Fixed compiling error in Clang
> > >
> > > Changes in v7:
> > > - Thanks to Jacopo:
> > >   - Made CameraManager a unique_ptr and passed to Environment as raw pointer
> > >
> > > No changes in v6
> > >
> > > Changes in v5:
> > > - Thanks to Laurent:
> > >   - Fixed style issues
> > >   - Added SetUp and TearDown functions to acquire and release the camera for
> > >     each test
> > >
> > > 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/capture_test.cpp   | 127 +++++++++++++++++++++++++++
> > >  src/lc-compliance/main.cpp           | 106 +++++++++-------------
> > >  src/lc-compliance/meson.build        |   7 +-
> > >  src/lc-compliance/results.cpp        |  75 ----------------
> > >  src/lc-compliance/results.h          |  47 ----------
> > >  src/lc-compliance/simple_capture.cpp |  77 +++++++---------
> > >  src/lc-compliance/simple_capture.h   |   9 +-
> > >  src/lc-compliance/single_stream.cpp  |  97 --------------------
> > >  src/lc-compliance/tests.h            |  16 ----
> > >  9 files changed, 206 insertions(+), 355 deletions(-)
> > >  create mode 100644 src/lc-compliance/capture_test.cpp
> > >  delete mode 100644 src/lc-compliance/results.cpp
> > >  delete mode 100644 src/lc-compliance/results.h
> > >  delete mode 100644 src/lc-compliance/single_stream.cpp
> > >  delete mode 100644 src/lc-compliance/tests.h
> > >
> > > diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp
> > > new file mode 100644
> > > index 000000000000..116ae496fe04
> > > --- /dev/null
> > > +++ b/src/lc-compliance/capture_test.cpp
> > > @@ -0,0 +1,127 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Google Inc.
> >
> > Aaaah, time flies! Could you check the year for the other files too ?
>
> Now that you mention it, I wonder if the changes were big enough that I should
> add a
>
>     Copyright (C) 2021, Collabora Ltd.
>
> instead on this file. Maybe even for main.cpp as well?

I think it's fair to do so, this is quite a substantial change to the
existing code.

I'm not aware if there's a fixed rule for that or it is left to the
common sense...

> >
> > > + *
> > > + * single_stream.cpp - Test a single camera stream
> > > + */
> > > +
> > > +#include <iostream>
> > > +
> > > +#include <gtest/gtest.h>
> > > +
> > > +#include "environment.h"
> > > +#include "simple_capture.h"
> >
> > We tend to include the file header first to make sure it's
> > self-contained.
>
> This file (now named capture_test.cpp) doesn't have a header file though. But
> that shows that I forgot to update the file name and description in the comment
> a few lines above.

Right, I confused single_stream.h and simple_capture.h. Regardelss,
please update the file name in the initial comment block

>
> >
> > > +
> > > +using namespace libcamera;
> > > +
> > > +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 SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> > > +{
> > > +public:
> > > +	static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> > > +
> > > +protected:
> > > +	void SetUp() override;
> > > +	void TearDown() override;
> > > +
> > > +	std::shared_ptr<Camera> camera_;
> > > +};
> > > +
> > > +/*
> > > + * We use gtest's SetUp() and TearDown() instead of constructor and destructor
> > > + * in order to be able to assert on them.
> > > + */
> > > +void SingleStream::SetUp()
> > > +{
> > > +	Environment *env = Environment::get();
> > > +
> > > +	camera_ = env->cm()->get(env->cameraId());
> > > +
> > > +	ASSERT_EQ(camera_->acquire(), 0);
> > > +}
> > > +
> > > +void SingleStream::TearDown()
> > > +{
> > > +	if (!camera_)
> > > +		return;
> > > +
> > > +	camera_->release();
> > > +	camera_.reset();
> > > +}
> > > +
> > > +std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::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.
> >
> > I'm not sure I get the last statement (probably I had the same comment on the
> > previous version :) is "M" the queue depth ? Honestly I would drop it.
>
> I think that statement could be later improved to be less generic, but still I
> see some use in it. Regardless of what M means, if there are cases where a few
> extra requests are required to complete the intended amount, we should be on the
> lookout for them.

Now I probably better get what was meant there. As we skip testing a
number of requests < than the queue depth a camera that completes N
requests if you queue M+N means not all requets are completed and
that's why it is faulty :)

I would drop or say "an example failure is a camera that completes
less requests that the queued ones".

Or maybe I didn't get it properly yet :)

>
> >
> > > + */
> > > +TEST_P(SingleStream, Capture)
> > > +{
> > > +	auto [role, numRequests] = GetParam();
> > > +
> > > +	SimpleCaptureBalanced capture(camera_);
> > > +
> > > +	capture.configure(role);
> > > +
> > > +	capture.capture(numRequests);
> > > +}
> > > +
> > > +/*
> > > + * Test multiple start/stop cycles
> > > + *
> > > + * Makes sure the camera supports multiple start/stop cycles. Example failure is
> > > + * a camera that does not clean up correctly in its error path but is only
> > > + * tested by single-capture applications.
> > > + */
> > > +TEST_P(SingleStream, CaptureStartStop)
> > > +{
> > > +	auto [role, numRequests] = GetParam();
> > > +	unsigned int numRepeats = 3;
> > > +
> > > +	SimpleCaptureBalanced capture(camera_);
> > > +
> > > +	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(SingleStream, UnbalancedStop)
> > > +{
> > > +	auto [role, numRequests] = GetParam();
> > > +
> > > +	SimpleCaptureUnbalanced capture(camera_);
> > > +
> > > +	capture.configure(role);
> > > +
> > > +	capture.capture(numRequests);
> > > +}
> > > +
> > > +INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > > +			 SingleStream,
> > > +			 testing::Combine(testing::ValuesIn(ROLES),
> > > +					  testing::ValuesIn(NUMREQUESTS)),
> > > +			 SingleStream::nameParameters);
> > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > > index 54cee54aa978..b01768b5fc0b 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,97 +23,59 @@ enum {
> > >  	OptHelp = 'h',
> > >  };
> > >
> > > -class Harness
> > > +/*
> > > + * 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
> > >  {
> > > -public:
> > > -	Harness(const OptionsParser::Options &options);
> > > -	~Harness();
> > > -
> > > -	int exec();
> > > -
> > > -private:
> > > -	int init();
> > > -	void listCameras();
> > > -
> > > -	OptionsParser::Options options_;
> > > -	std::unique_ptr<CameraManager> cm_;
> > > -	std::shared_ptr<Camera> camera_;
> > > +	void OnTestPartResult(const testing::TestPartResult &result) override
> > > +	{
> > > +		if (result.type() == testing::TestPartResult::kFatalFailure ||
> > > +		    result.type() == testing::TestPartResult::kSkip)
> > > +			throw testing::AssertionException(result);
> > > +	}
> > >  };
> > >
> > > -Harness::Harness(const OptionsParser::Options &options)
> > > -	: options_(options)
> > > -{
> > > -	cm_ = std::make_unique<CameraManager>();
> > > -}
> > > -
> > > -Harness::~Harness()
> > > +static void listCameras(CameraManager *cm)
> > >  {
> > > -	if (camera_) {
> > > -		camera_->release();
> > > -		camera_.reset();
> > > -	}
> > > -
> > > -	cm_->stop();
> > > +	for (const std::shared_ptr<Camera> &cam : cm->cameras())
> > > +		std::cout << "- " << cam.get()->id() << std::endl;
> > >  }
> > >
> > > -int Harness::exec()
> > > +static int initCamera(CameraManager *cm, OptionsParser::Options options)
> > >  {
> > > -	int ret = init();
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	std::vector<Results> results;
> > > +	std::shared_ptr<Camera> camera;
> > >
> > > -	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();
> > > +	int ret = cm->start();
> > >  	if (ret) {
> > >  		std::cout << "Failed to start camera manager: "
> > >  			  << strerror(-ret) << std::endl;
> > >  		return ret;
> > >  	}
> > >
> > > -	if (!options_.isSet(OptCamera)) {
> > > +	if (!options.isSet(OptCamera)) {
> > >  		std::cout << "No camera specified, available cameras:" << std::endl;
> > > -		listCameras();
> > > +		listCameras(cm);
> > >  		return -ENODEV;
> > >  	}
> > >
> > > -	const std::string &cameraId = options_[OptCamera];
> > > -	camera_ = cm_->get(cameraId);
> > > -	if (!camera_) {
> > > +	const std::string &cameraId = options[OptCamera];
> > > +	camera = cm->get(cameraId);
> > > +	if (!camera) {
> > >  		std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> > > -		listCameras();
> > > +		listCameras(cm);
> > >  		return -ENODEV;
> > >  	}
> > >
> > > -	if (camera_->acquire()) {
> > > -		std::cout << "Failed to acquire camera" << std::endl;
> > > -		return -EINVAL;
> > > -	}
> > > +	Environment::get()->setup(cm, cameraId);
> > >
> > >  	std::cout << "Using camera " << cameraId << std::endl;
> > >
> > >  	return 0;
> > >  }
> > >
> > > -void Harness::listCameras()
> > > -{
> > > -	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > > -		std::cout << "- " << cam.get()->id() << std::endl;
> > > -}
> > > -
> > >  static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
> > >  {
> > >  	OptionsParser parser;
> > > @@ -142,7 +106,17 @@ int main(int argc, char **argv)
> > >  	if (ret < 0)
> > >  		return EXIT_FAILURE;
> > >
> > > -	Harness harness(options);
> > > +	std::unique_ptr<CameraManager> cm = std::make_unique<CameraManager>();
> > > +
> > > +	ret = initCamera(cm.get(), options);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> > > +
> > > +	ret = RUN_ALL_TESTS();
> > > +
> > > +	cm->stop();
> > >
> > > -	return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;
> > > +	return ret;
> > >  }
> > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > > index 6dd49085569f..f3a79ae438a9 100644
> > > --- a/src/lc-compliance/meson.build
> > > +++ b/src/lc-compliance/meson.build
> > > @@ -1,8 +1,9 @@
> > >  # SPDX-License-Identifier: CC0-1.0
> > >
> > >  libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> > > +libgtest = dependency('gtest', required : get_option('lc-compliance'))
> > >
> > > -if not libevent.found()
> > > +if not (libevent.found() and libgtest.found())
> > >      lc_compliance_enabled = false
> > >      subdir_done()
> > >  endif
> > > @@ -14,9 +15,8 @@ lc_compliance_sources = files([
> > >      '../cam/options.cpp',
> > >      'environment.cpp',
> > >      'main.cpp',
> > > -    'results.cpp',
> > >      'simple_capture.cpp',
> > > -    'single_stream.cpp',
> > > +    'capture_test.cpp',
> > >  ])
> > >
> > >  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> > > @@ -24,5 +24,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> > >                                  libatomic,
> > >                                  libcamera_dep,
> > >                                  libevent,
> > > +                                libgtest,
> >
> > After the recent split of libcamera-base this does not apply anymore.
> > Rebase is trivial, but much easier for you since you have a branch.
> > I'm afraid this would require a v10 to make things simpler on our side
> > (fixing this is trivial, but I have conflicts also when applying the
> > next patch and that would require manually applying the diff :(
>
> Interesting, I didn't know a rebase was able to automatically resolve basic
> conflicts while applying patches wasn't. But indeed I rebased it now on master
> without any tweaks needed. Should be good for v10.
>
> >
> > >                              ],
> > >                              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 bfc91b26444e..ce0883672709 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,37 @@ 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" };
> > > +	int count = allocator_->allocate(stream);
> > >
> > > -	if (camera_->start())
> > > -		return { Results::Fail, "Failed to start camera" };
> > > +	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > > +	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> > >
> > > -	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> > > +	ASSERT_TRUE(!camera_->start()) << "Failed to start camera";
> > >
> > > -	return { Results::Pass, "Started camera" };
> > > +	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> >
> > Shouldn't the signal be connected before starting the camera ?
>
> I guess it doesn't matter as long as it's connected before any requests are
> queued. But also doesn't hurt to move it :), and that makes it more symmetrical
> to stop().
>

Simmetry is nice indeed!

> >
> > >  }
> > >
> > >  void SimpleCapture::stop()
> > > @@ -74,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;
> > > @@ -100,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";
> >
> > ASSERT_EQ(.., 0)
>
> Sure.
>
> >
> > >
> > > -		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));
> > >  	}
> > > @@ -118,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)
> > > @@ -155,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);
> > > @@ -171,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";
> >
> > maybe it's just me being disappointed by the usage of FALSE() to test the
> > resuls of a function is 0 ? If that's the case just ignore the
> > comments on this topic :)
>
> No, I do think it makes sense. I went ahead and changed some others.

Thank you!

>
> Related to that, checking that pointers are valid is being done with
> ASSERT_TRUE(ptr). Do you think it would be better to use ASSERT_NE(ptr, 0)
> instead?
>

As we usually do if(!ptr) I think ASSERT_TRUE is fine. Whatever,
really :)

> >
> > >
> > > -		if (camera_->queueRequest(request.get()) < 0)
> > > -			return { Results::Fail, "Failed to queue request" };
> > > +		ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
> >
> > ASSERT_EQ() ?
>
> Sure.
>
> >
> > All minors, I like this new setup and the naming scheme (at least I
> > agree with myself time to time :) I'm sure we'll refactor later as you
> > suggested but for now that's fine.
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks!
>

Thank you for pushing this up to v10!

> Nícolas
>
> >
> > Thanks
> >   j
> >
> >
> > >
> > >  		requests.push_back(std::move(request));
> > >  	}
> > > @@ -189,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
> > > deleted file mode 100644
> > > index 8318b42f42d6..000000000000
> > > --- a/src/lc-compliance/single_stream.cpp
> > > +++ /dev/null
> > > @@ -1,97 +0,0 @@
> > > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > -/*
> > > - * Copyright (C) 2020, Google Inc.
> > > - *
> > > - * single_stream.cpp - Test a single camera stream
> > > - */
> > > -
> > > -#include <iostream>
> > > -
> > > -#include "simple_capture.h"
> > > -#include "tests.h"
> > > -
> > > -using namespace libcamera;
> > > -
> > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
> > > -				   StreamRole role, unsigned int startCycles,
> > > -				   unsigned int numRequests)
> > > -{
> > > -	SimpleCaptureBalanced capture(camera);
> > > -
> > > -	Results::Result ret = capture.configure(role);
> > > -	if (ret.first != Results::Pass)
> > > -		return ret;
> > > -
> > > -	for (unsigned int starts = 0; starts < startCycles; starts++) {
> > > -		ret = capture.capture(numRequests);
> > > -		if (ret.first != Results::Pass)
> > > -			return ret;
> > > -	}
> > > -
> > > -	return { Results::Pass, "Balanced capture of " +
> > > -		std::to_string(numRequests) + " requests with " +
> > > -		std::to_string(startCycles) + " start cycles" };
> > > -}
> > > -
> > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> > > -				     StreamRole role, unsigned int numRequests)
> > > -{
> > > -	SimpleCaptureUnbalanced capture(camera);
> > > -
> > > -	Results::Result ret = capture.configure(role);
> > > -	if (ret.first != Results::Pass)
> > > -		return ret;
> > > -
> > > -	return 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;
> > > -}
> > > 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.32.0
> > >