| Message ID | 20210514131652.345486-4-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nícolas, Thanks for your work! On 2021-05-14 10:16:51 -0300, Nícolas F. R. A. Prado wrote: > Refactor lc-compliance using Googletest as the test framework. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > Changes in v3: > - Thanks to Niklas: > - Went back to static test registration, and created a Singleton Environment > class to store the camera global variable > - Added a nameParameters() function to give meaningful names to the test > parameters, removing the need to register each test suite for every role > > src/lc-compliance/main.cpp | 88 ++++++++++------ > src/lc-compliance/meson.build | 3 + > src/lc-compliance/simple_capture.cpp | 74 +++++--------- > src/lc-compliance/simple_capture.h | 8 +- > src/lc-compliance/single_stream.cpp | 148 ++++++++++++++------------- > src/lc-compliance/tests.h | 19 +++- > 6 files changed, 187 insertions(+), 153 deletions(-) > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > index 54cee54aa978..d5ff93f514df 100644 > --- a/src/lc-compliance/main.cpp > +++ b/src/lc-compliance/main.cpp > @@ -9,6 +9,8 @@ > #include <iostream> > #include <string.h> > > +#include <gtest/gtest.h> > + > #include <libcamera/libcamera.h> > > #include "../cam/options.h" > @@ -21,21 +23,43 @@ enum { > OptHelp = 'h', > }; > > +bool Environment::create(std::shared_ptr<Camera> camera) > +{ > + if (instance_) > + return false; > + > + camera_ = camera; > + instance_ = new Environment; > + return true; > +} > + > +void Environment::destroy() > +{ > + if (!camera_) > + return; > + > + camera_->release(); > + camera_.reset(); > +} > + > +Environment *Environment::instance() > +{ > + return instance_; > +} I think this and the corresponding class declaration should be moved to a separate environment.{cpp,h} file. It could possibly be added in a separate patch just previous to this one. > + > class Harness > { > public: > Harness(const OptionsParser::Options &options); > ~Harness(); > > - int exec(); > + int init(); > > private: > - int init(); > void listCameras(); > > OptionsParser::Options options_; > std::unique_ptr<CameraManager> cm_; > - std::shared_ptr<Camera> camera_; > }; > > Harness::Harness(const OptionsParser::Options &options) > @@ -46,35 +70,15 @@ Harness::Harness(const OptionsParser::Options &options) > > Harness::~Harness() > { > - if (camera_) { > - camera_->release(); > - camera_.reset(); > - } > + Environment::instance()->destroy(); > > cm_->stop(); > } > > -int Harness::exec() > -{ > - int ret = init(); > - if (ret) > - return ret; > - > - std::vector<Results> results; > - > - results.push_back(testSingleStream(camera_)); > - > - for (const Results &result : results) { > - ret = result.summary(); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > int Harness::init() > { > + std::shared_ptr<Camera> camera; > + > int ret = cm_->start(); > if (ret) { > std::cout << "Failed to start camera manager: " > @@ -89,18 +93,20 @@ int Harness::init() > } > > const std::string &cameraId = options_[OptCamera]; > - camera_ = cm_->get(cameraId); > - if (!camera_) { > + camera = cm_->get(cameraId); > + if (!camera) { > std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > listCameras(); > return -ENODEV; > } > > - if (camera_->acquire()) { > + if (camera->acquire()) { > std::cout << "Failed to acquire camera" << std::endl; > return -EINVAL; > } > > + Environment::create(camera); > + > std::cout << "Using camera " << cameraId << std::endl; > > return 0; > @@ -133,6 +139,23 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > return 0; > } > > +/* > + * Make asserts act like exceptions, otherwise they only fail (or skip) the > + * current function. From gtest documentation: > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception > + */ > +class ThrowListener : public testing::EmptyTestEventListener { > + void OnTestPartResult(const testing::TestPartResult& result) override { > + if (result.type() == testing::TestPartResult::kFatalFailure > + || result.type() == testing::TestPartResult::kSkip) { > + throw testing::AssertionException(result); > + } > + } > +}; > + > +Environment *Environment::instance_ = nullptr; > +std::shared_ptr<Camera> Environment::camera_ = nullptr; > + > int main(int argc, char **argv) > { > OptionsParser::Options options; > @@ -143,6 +166,11 @@ int main(int argc, char **argv) > return EXIT_FAILURE; > > Harness harness(options); > + ret = harness.init(); > + if (ret) > + return ret; > > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; > + ::testing::InitGoogleTest(&argc, argv); > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); > + return RUN_ALL_TESTS(); > } > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > index a2bfcceb1259..704bc18af3e1 100644 > --- a/src/lc-compliance/meson.build > +++ b/src/lc-compliance/meson.build > @@ -18,10 +18,13 @@ lc_compliance_sources = files([ > 'single_stream.cpp', > ]) > > +gtest_dep = dependency('gtest') > + > lc_compliance = executable('lc-compliance', lc_compliance_sources, > dependencies : [ > libatomic, > libcamera_dep, > libevent, > + gtest_dep, > ], > install : true) > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > index f90fe6d0f9aa..7731eb16f8c2 100644 > --- a/src/lc-compliance/simple_capture.cpp > +++ b/src/lc-compliance/simple_capture.cpp > @@ -5,6 +5,8 @@ > * simple_capture.cpp - Simple capture helper > */ > > +#include <gtest/gtest.h> > + > #include "simple_capture.h" > > using namespace libcamera; > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() > stop(); > } > > -Results::Result SimpleCapture::configure(StreamRole role) > +void SimpleCapture::configure(StreamRole role) > { > config_ = camera_->generateConfiguration({ role }); > > - if (!config_) > - return { Results::Skip, "Role not supported by camera" }; > + if (!config_) { > + std::cout << "Role not supported by camera" << std::endl; > + GTEST_SKIP(); > + } > > if (config_->validate() != CameraConfiguration::Valid) { > config_.reset(); > - return { Results::Fail, "Configuration not valid" }; > + FAIL() << "Configuration not valid"; > } > > if (camera_->configure(config_.get())) { > config_.reset(); > - return { Results::Fail, "Failed to configure camera" }; > + FAIL() << "Failed to configure camera"; > } > - > - return { Results::Pass, "Configure camera" }; > } > > -Results::Result SimpleCapture::start() > +void SimpleCapture::start() > { > Stream *stream = config_->at(0).stream(); > - if (allocator_->allocate(stream) < 0) > - return { Results::Fail, "Failed to allocate buffers" }; > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; > > - if (camera_->start()) > - return { Results::Fail, "Failed to start camera" }; > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > - > - return { Results::Pass, "Started camera" }; > } > > void SimpleCapture::stop() > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) > { > } > > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > +void SimpleCaptureBalanced::capture(unsigned int numRequests) > { > - Results::Result ret = start(); > - if (ret.first != Results::Pass) > - return ret; > + start(); > > Stream *stream = config_->at(0).stream(); > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > /* No point in testing less requests then the camera depth. */ > if (buffers.size() > numRequests) { > - /* Cache buffers.size() before we destroy it in stop() */ > - int buffers_size = buffers.size(); > - > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > - + " requests, can't test only " + std::to_string(numRequests) }; > + std::cout << "Camera needs " + std::to_string(buffers.size()) > + + " requests, can't test only " > + + std::to_string(numRequests) << std::endl; > + GTEST_SKIP(); > } > > queueCount_ = 0; > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > std::vector<std::unique_ptr<libcamera::Request>> requests; > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > std::unique_ptr<Request> request = camera_->createRequest(); > - if (!request) > - return { Results::Fail, "Can't create request" }; > + ASSERT_TRUE(request) << "Can't create request"; > > - if (request->addBuffer(stream, buffer.get())) > - return { Results::Fail, "Can't set buffer for request" }; > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > - if (queueRequest(request.get()) < 0) > - return { Results::Fail, "Failed to queue request" }; > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; > > requests.push_back(std::move(request)); > } > @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > stop(); > delete loop_; > > - if (captureCount_ != captureLimit_) > - return { Results::Fail, "Got " + std::to_string(captureCount_) + > - " request, wanted " + std::to_string(captureLimit_) }; > - > - return { Results::Pass, "Balanced capture of " + > - std::to_string(numRequests) + " requests" }; > + ASSERT_EQ(captureCount_, captureLimit_); > } > > int SimpleCaptureBalanced::queueRequest(Request *request) > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) > { > } > > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) > { > - Results::Result ret = start(); > - if (ret.first != Results::Pass) > - return ret; > + start(); > > Stream *stream = config_->at(0).stream(); > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > std::vector<std::unique_ptr<libcamera::Request>> requests; > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > std::unique_ptr<Request> request = camera_->createRequest(); > - if (!request) > - return { Results::Fail, "Can't create request" }; > + ASSERT_TRUE(request) << "Can't create request"; > > - if (request->addBuffer(stream, buffer.get())) > - return { Results::Fail, "Can't set buffer for request" }; > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > - if (camera_->queueRequest(request.get()) < 0) > - return { Results::Fail, "Failed to queue request" }; > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > requests.push_back(std::move(request)); > } > @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > stop(); > delete loop_; > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > + ASSERT_FALSE(status); > } > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > index d9de53fb63a3..0f8465083456 100644 > --- a/src/lc-compliance/simple_capture.h > +++ b/src/lc-compliance/simple_capture.h > @@ -17,13 +17,13 @@ > class SimpleCapture > { > public: > - Results::Result configure(libcamera::StreamRole role); > + void configure(libcamera::StreamRole role); > > protected: > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > virtual ~SimpleCapture(); > > - Results::Result start(); > + void start(); > void stop(); > > virtual void requestComplete(libcamera::Request *request) = 0; > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture > public: > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); > > - Results::Result capture(unsigned int numRequests); > + void capture(unsigned int numRequests); > > private: > int queueRequest(libcamera::Request *request); > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture > public: > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); > > - Results::Result capture(unsigned int numRequests); > + void capture(unsigned int numRequests); > > private: > void requestComplete(libcamera::Request *request) override; > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > index 8318b42f42d6..aad32ecd051e 100644 > --- a/src/lc-compliance/single_stream.cpp > +++ b/src/lc-compliance/single_stream.cpp > @@ -7,91 +7,95 @@ > > #include <iostream> > > +#include <gtest/gtest.h> > + > #include "simple_capture.h" > #include "tests.h" > > using namespace libcamera; > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > - StreamRole role, unsigned int startCycles, > - unsigned int numRequests) > +const std::vector<int> NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; > +const std::vector<StreamRole> ROLES = {Raw, StillCapture, VideoRecording, Viewfinder}; > + > +class FixedRequestsTest : > + public testing::TestWithParam<std::tuple<StreamRole, int>> { > +}; > + > +std::string nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType>& info) > { > + std::map<StreamRole, std::string> rolesMap = {{Raw, "raw"}, > + {StillCapture, "still"}, > + {VideoRecording, "video"}, > + {Viewfinder, "viewfinder"}}; > + > + std::string roleName = rolesMap[std::get<0>(info.param)]; > + std::string numRequestsName = std::to_string(std::get<1>(info.param)); > + > + return roleName + numRequestsName; Are there some guidelines or recommendations around gtest to these names? I really like that you make it possible to name each one in a more human readable format. While testing this series I noticed tests like BalancedMultiCycle/video2 and incorrectly thought "This is testing the V4L2 device /dev/video2" not this is testing the video recording role with 2 requests. Would it make sens to name it BalancedMultiCycle/VideoRecording/2 ? I fear this could turn into a bikeshedding discussion and we can always work it on-top. But if others have ideas about naming please speak up. I think for example in the CrOS test suite uses '.' as separator instead of '/'? So maybe there are some best-practices to be found. > +} > + > +/* > + * Test single capture cycles > + * > + * Makes sure the camera completes the exact number of requests queued. Example > + * failure is a camera that needs N+M requests queued to complete N requests to > + * the application. > + */ > +TEST_P(FixedRequestsTest, BalancedSingleCycle) > +{ > + auto [role, numRequests] = GetParam(); > + std::shared_ptr<Camera> camera = Environment::instance()->camera(); > + > SimpleCaptureBalanced capture(camera); > > - Results::Result ret = capture.configure(role); > - if (ret.first != Results::Pass) > - return ret; > + capture.configure(role); > > - for (unsigned int starts = 0; starts < startCycles; starts++) { > - ret = capture.capture(numRequests); > - if (ret.first != Results::Pass) > - return ret; > - } > + capture.capture(numRequests); > +}; > > - return { Results::Pass, "Balanced capture of " + > - std::to_string(numRequests) + " requests with " + > - std::to_string(startCycles) + " start cycles" }; > -} > +/* > + * Test multiple start/stop cycles > + * > + * Makes sure the camera supports multiple start/stop cycles. Example failure is > + * a camera that does not clean up correctly in its error path but is only > + * tested by single-capture applications. > + */ > +TEST_P(FixedRequestsTest, BalancedMultiCycle) > +{ > + auto [role, numRequests] = GetParam(); > + std::shared_ptr<Camera> camera = Environment::instance()->camera(); > + unsigned int numRepeats = 3; > + > + SimpleCaptureBalanced capture(camera); > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > - StreamRole role, unsigned int numRequests) > + capture.configure(role); > + > + for (unsigned int starts = 0; starts < numRepeats; starts++) > + capture.capture(numRequests); > +}; > + > +/* > + * Test unbalanced stop > + * > + * Makes sure the camera supports a stop with requests queued. Example failure > + * is a camera that does not handle cancelation of buffers coming back from the > + * video device while stopping. > + */ > +TEST_P(FixedRequestsTest, Unbalanced) > { > + auto [role, numRequests] = GetParam(); > + std::shared_ptr<Camera> camera = Environment::instance()->camera(); > + > SimpleCaptureUnbalanced capture(camera); > > - Results::Result ret = capture.configure(role); > - if (ret.first != Results::Pass) > - return ret; > + capture.configure(role); > > - return capture.capture(numRequests); > -} > + capture.capture(numRequests); > +}; > > -Results testSingleStream(std::shared_ptr<Camera> camera) > -{ > - static const std::vector<std::pair<std::string, StreamRole>> roles = { > - { "raw", Raw }, > - { "still", StillCapture }, > - { "video", VideoRecording }, > - { "viewfinder", Viewfinder }, > - }; > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > - > - Results results(numRequests.size() * roles.size() * 3); > - > - for (const auto &role : roles) { > - std::cout << "= Test role " << role.first << std::endl; > - /* > - * Test single capture cycles > - * > - * Makes sure the camera completes the exact number of requests queued. > - * Example failure is a camera that needs N+M requests queued to > - * complete N requests to the application. > - */ > - std::cout << "* Test single capture cycles" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestBalance(camera, role.second, 1, num)); > - > - /* > - * Test multiple start/stop cycles > - * > - * Makes sure the camera supports multiple start/stop cycles. > - * Example failure is a camera that does not clean up correctly in its > - * error path but is only tested by single-capture applications. > - */ > - std::cout << "* Test multiple start/stop cycles" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestBalance(camera, role.second, 3, num)); > - > - /* > - * Test unbalanced stop > - * > - * Makes sure the camera supports a stop with requests queued. > - * Example failure is a camera that does not handle cancelation > - * of buffers coming back from the video device while stopping. > - */ > - std::cout << "* Test unbalanced stop" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestUnbalance(camera, role.second, num)); > - } > - > - return results; > -} > + > +INSTANTIATE_TEST_SUITE_P(RolesAndRequests, > + FixedRequestsTest, > + testing::Combine(testing::ValuesIn(ROLES), > + testing::ValuesIn(NUMREQUESTS)), > + nameParameters); > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h > index 396605214e4b..076785a7185f 100644 > --- a/src/lc-compliance/tests.h > +++ b/src/lc-compliance/tests.h > @@ -11,6 +11,23 @@ > > #include "results.h" > > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); > +using namespace libcamera; > + > +class Environment > +{ > +public: > + static bool create(std::shared_ptr<Camera> camera); > + void destroy(); > + > + static Environment *instance(); > + > + std::shared_ptr<Camera> camera() const { return camera_; } > + > +private: > + Environment() {}; > + > + static Environment *instance_; > + static std::shared_ptr<Camera> camera_; > +}; > > #endif /* __LC_COMPLIANCE_TESTS_H__ */ > -- > 2.31.1 >
Hi Niklas, thank you for the feedback. Em 2021-05-16 11:01, Niklas Söderlund escreveu: > Hi Nícolas, > > Thanks for your work! > > On 2021-05-14 10:16:51 -0300, Nícolas F. R. A. Prado wrote: > > Refactor lc-compliance using Googletest as the test framework. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > Changes in v3: > > - Thanks to Niklas: > > - Went back to static test registration, and created a Singleton Environment > > class to store the camera global variable > > - Added a nameParameters() function to give meaningful names to the test > > parameters, removing the need to register each test suite for every role > > > > src/lc-compliance/main.cpp | 88 ++++++++++------ > > src/lc-compliance/meson.build | 3 + > > src/lc-compliance/simple_capture.cpp | 74 +++++--------- > > src/lc-compliance/simple_capture.h | 8 +- > > src/lc-compliance/single_stream.cpp | 148 ++++++++++++++------------- > > src/lc-compliance/tests.h | 19 +++- > > 6 files changed, 187 insertions(+), 153 deletions(-) > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > index 54cee54aa978..d5ff93f514df 100644 > > --- a/src/lc-compliance/main.cpp > > +++ b/src/lc-compliance/main.cpp > > @@ -9,6 +9,8 @@ > > #include <iostream> > > #include <string.h> > > > > +#include <gtest/gtest.h> > > + > > #include <libcamera/libcamera.h> > > > > #include "../cam/options.h" > > @@ -21,21 +23,43 @@ enum { > > OptHelp = 'h', > > }; > > > > +bool Environment::create(std::shared_ptr<Camera> camera) > > +{ > > + if (instance_) > > + return false; > > + > > + camera_ = camera; > > + instance_ = new Environment; > > + return true; > > +} > > + > > +void Environment::destroy() > > +{ > > + if (!camera_) > > + return; > > + > > + camera_->release(); > > + camera_.reset(); > > +} > > + > > +Environment *Environment::instance() > > +{ > > + return instance_; > > +} > > I think this and the corresponding class declaration should be moved to > a separate environment.{cpp,h} file. It could possibly be added in a > separate patch just previous to this one. Yep, that makes sense. Will do for v4. > > > + > > class Harness > > { > > public: > > Harness(const OptionsParser::Options &options); > > ~Harness(); > > > > - int exec(); > > + int init(); > > > > private: > > - int init(); > > void listCameras(); > > > > OptionsParser::Options options_; > > std::unique_ptr<CameraManager> cm_; > > - std::shared_ptr<Camera> camera_; > > }; > > > > Harness::Harness(const OptionsParser::Options &options) > > @@ -46,35 +70,15 @@ Harness::Harness(const OptionsParser::Options &options) > > > > Harness::~Harness() > > { > > - if (camera_) { > > - camera_->release(); > > - camera_.reset(); > > - } > > + Environment::instance()->destroy(); > > > > cm_->stop(); > > } > > > > -int Harness::exec() > > -{ > > - int ret = init(); > > - if (ret) > > - return ret; > > - > > - std::vector<Results> results; > > - > > - results.push_back(testSingleStream(camera_)); > > - > > - for (const Results &result : results) { > > - ret = result.summary(); > > - if (ret) > > - return ret; > > - } > > - > > - return 0; > > -} > > - > > int Harness::init() > > { > > + std::shared_ptr<Camera> camera; > > + > > int ret = cm_->start(); > > if (ret) { > > std::cout << "Failed to start camera manager: " > > @@ -89,18 +93,20 @@ int Harness::init() > > } > > > > const std::string &cameraId = options_[OptCamera]; > > - camera_ = cm_->get(cameraId); > > - if (!camera_) { > > + camera = cm_->get(cameraId); > > + if (!camera) { > > std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > listCameras(); > > return -ENODEV; > > } > > > > - if (camera_->acquire()) { > > + if (camera->acquire()) { > > std::cout << "Failed to acquire camera" << std::endl; > > return -EINVAL; > > } > > > > + Environment::create(camera); > > + > > std::cout << "Using camera " << cameraId << std::endl; > > > > return 0; > > @@ -133,6 +139,23 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > return 0; > > } > > > > +/* > > + * Make asserts act like exceptions, otherwise they only fail (or skip) the > > + * current function. From gtest documentation: > > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception > > + */ > > +class ThrowListener : public testing::EmptyTestEventListener { > > + void OnTestPartResult(const testing::TestPartResult& result) override { > > + if (result.type() == testing::TestPartResult::kFatalFailure > > + || result.type() == testing::TestPartResult::kSkip) { > > + throw testing::AssertionException(result); > > + } > > + } > > +}; > > + > > +Environment *Environment::instance_ = nullptr; > > +std::shared_ptr<Camera> Environment::camera_ = nullptr; > > + > > int main(int argc, char **argv) > > { > > OptionsParser::Options options; > > @@ -143,6 +166,11 @@ int main(int argc, char **argv) > > return EXIT_FAILURE; > > > > Harness harness(options); > > + ret = harness.init(); > > + if (ret) > > + return ret; > > > > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; > > + ::testing::InitGoogleTest(&argc, argv); > > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); > > + return RUN_ALL_TESTS(); > > } > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > > index a2bfcceb1259..704bc18af3e1 100644 > > --- a/src/lc-compliance/meson.build > > +++ b/src/lc-compliance/meson.build > > @@ -18,10 +18,13 @@ lc_compliance_sources = files([ > > 'single_stream.cpp', > > ]) > > > > +gtest_dep = dependency('gtest') > > + > > lc_compliance = executable('lc-compliance', lc_compliance_sources, > > dependencies : [ > > libatomic, > > libcamera_dep, > > libevent, > > + gtest_dep, > > ], > > install : true) > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > > index f90fe6d0f9aa..7731eb16f8c2 100644 > > --- a/src/lc-compliance/simple_capture.cpp > > +++ b/src/lc-compliance/simple_capture.cpp > > @@ -5,6 +5,8 @@ > > * simple_capture.cpp - Simple capture helper > > */ > > > > +#include <gtest/gtest.h> > > + > > #include "simple_capture.h" > > > > using namespace libcamera; > > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() > > stop(); > > } > > > > -Results::Result SimpleCapture::configure(StreamRole role) > > +void SimpleCapture::configure(StreamRole role) > > { > > config_ = camera_->generateConfiguration({ role }); > > > > - if (!config_) > > - return { Results::Skip, "Role not supported by camera" }; > > + if (!config_) { > > + std::cout << "Role not supported by camera" << std::endl; > > + GTEST_SKIP(); > > + } > > > > if (config_->validate() != CameraConfiguration::Valid) { > > config_.reset(); > > - return { Results::Fail, "Configuration not valid" }; > > + FAIL() << "Configuration not valid"; > > } > > > > if (camera_->configure(config_.get())) { > > config_.reset(); > > - return { Results::Fail, "Failed to configure camera" }; > > + FAIL() << "Failed to configure camera"; > > } > > - > > - return { Results::Pass, "Configure camera" }; > > } > > > > -Results::Result SimpleCapture::start() > > +void SimpleCapture::start() > > { > > Stream *stream = config_->at(0).stream(); > > - if (allocator_->allocate(stream) < 0) > > - return { Results::Fail, "Failed to allocate buffers" }; > > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; > > > > - if (camera_->start()) > > - return { Results::Fail, "Failed to start camera" }; > > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; > > > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > > - > > - return { Results::Pass, "Started camera" }; > > } > > > > void SimpleCapture::stop() > > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) > > { > > } > > > > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > +void SimpleCaptureBalanced::capture(unsigned int numRequests) > > { > > - Results::Result ret = start(); > > - if (ret.first != Results::Pass) > > - return ret; > > + start(); > > > > Stream *stream = config_->at(0).stream(); > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > /* No point in testing less requests then the camera depth. */ > > if (buffers.size() > numRequests) { > > - /* Cache buffers.size() before we destroy it in stop() */ > > - int buffers_size = buffers.size(); > > - > > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > > - + " requests, can't test only " + std::to_string(numRequests) }; > > + std::cout << "Camera needs " + std::to_string(buffers.size()) > > + + " requests, can't test only " > > + + std::to_string(numRequests) << std::endl; > > + GTEST_SKIP(); > > } > > > > queueCount_ = 0; > > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > std::unique_ptr<Request> request = camera_->createRequest(); > > - if (!request) > > - return { Results::Fail, "Can't create request" }; > > + ASSERT_TRUE(request) << "Can't create request"; > > > > - if (request->addBuffer(stream, buffer.get())) > > - return { Results::Fail, "Can't set buffer for request" }; > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > - if (queueRequest(request.get()) < 0) > > - return { Results::Fail, "Failed to queue request" }; > > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; > > > > requests.push_back(std::move(request)); > > } > > @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > stop(); > > delete loop_; > > > > - if (captureCount_ != captureLimit_) > > - return { Results::Fail, "Got " + std::to_string(captureCount_) + > > - " request, wanted " + std::to_string(captureLimit_) }; > > - > > - return { Results::Pass, "Balanced capture of " + > > - std::to_string(numRequests) + " requests" }; > > + ASSERT_EQ(captureCount_, captureLimit_); > > } > > > > int SimpleCaptureBalanced::queueRequest(Request *request) > > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) > > { > > } > > > > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > { > > - Results::Result ret = start(); > > - if (ret.first != Results::Pass) > > - return ret; > > + start(); > > > > Stream *stream = config_->at(0).stream(); > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > std::unique_ptr<Request> request = camera_->createRequest(); > > - if (!request) > > - return { Results::Fail, "Can't create request" }; > > + ASSERT_TRUE(request) << "Can't create request"; > > > > - if (request->addBuffer(stream, buffer.get())) > > - return { Results::Fail, "Can't set buffer for request" }; > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > - if (camera_->queueRequest(request.get()) < 0) > > - return { Results::Fail, "Failed to queue request" }; > > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > > > requests.push_back(std::move(request)); > > } > > @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > stop(); > > delete loop_; > > > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > > + ASSERT_FALSE(status); > > } > > > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > > index d9de53fb63a3..0f8465083456 100644 > > --- a/src/lc-compliance/simple_capture.h > > +++ b/src/lc-compliance/simple_capture.h > > @@ -17,13 +17,13 @@ > > class SimpleCapture > > { > > public: > > - Results::Result configure(libcamera::StreamRole role); > > + void configure(libcamera::StreamRole role); > > > > protected: > > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > > virtual ~SimpleCapture(); > > > > - Results::Result start(); > > + void start(); > > void stop(); > > > > virtual void requestComplete(libcamera::Request *request) = 0; > > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture > > public: > > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); > > > > - Results::Result capture(unsigned int numRequests); > > + void capture(unsigned int numRequests); > > > > private: > > int queueRequest(libcamera::Request *request); > > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture > > public: > > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); > > > > - Results::Result capture(unsigned int numRequests); > > + void capture(unsigned int numRequests); > > > > private: > > void requestComplete(libcamera::Request *request) override; > > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > > index 8318b42f42d6..aad32ecd051e 100644 > > --- a/src/lc-compliance/single_stream.cpp > > +++ b/src/lc-compliance/single_stream.cpp > > @@ -7,91 +7,95 @@ > > > > #include <iostream> > > > > +#include <gtest/gtest.h> > > + > > #include "simple_capture.h" > > #include "tests.h" > > > > using namespace libcamera; > > > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > > - StreamRole role, unsigned int startCycles, > > - unsigned int numRequests) > > +const std::vector<int> NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; > > +const std::vector<StreamRole> ROLES = {Raw, StillCapture, VideoRecording, Viewfinder}; > > + > > +class FixedRequestsTest : > > + public testing::TestWithParam<std::tuple<StreamRole, int>> { > > +}; > > + > > +std::string nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType>& info) > > { > > + std::map<StreamRole, std::string> rolesMap = {{Raw, "raw"}, > > + {StillCapture, "still"}, > > + {VideoRecording, "video"}, > > + {Viewfinder, "viewfinder"}}; > > + > > + std::string roleName = rolesMap[std::get<0>(info.param)]; > > + std::string numRequestsName = std::to_string(std::get<1>(info.param)); > > + > > + return roleName + numRequestsName; > > Are there some guidelines or recommendations around gtest to these > names? I really like that you make it possible to name each one in a > more human readable format. While testing this series I noticed tests > like BalancedMultiCycle/video2 and incorrectly thought "This is testing > the V4L2 device /dev/video2" not this is testing the video recording > role with 2 requests. There aren't any guidelines, but there are big restrictions on the names. Namely, they should be valid C++ identifiers. Take this for example: RolesAndRequests/FixedRequestsTest.BalancedSingleCycle/raw1 "RolesAndRequests" is the instantiation name (since different instantiations of the same test suite using different parameter ranges are possible). It is appended with a "/". "FixedRequestsTest" is the test suite. It is appended with a ".". "BalancedSingleCycle" is the test case. It is appended with a "/". "raw1" is the custom string I returned to represent the parameters for this particular parametrized test case. The complete test name reported is all of those joined with those special characters in-between. But internally they form identifiers, so each one should be a valid identifier. > > Would it make sens to name it BalancedMultiCycle/VideoRecording/2 ? Which means this isn't possible unfortunately. We could however use '_' to separate the parameter values: VideoRecording_2 to get: RolesAndRequests/FixedRequestsTest.BalancedSingleCycle/VideoRecording_2 Thanks, Nícolas > > I fear this could turn into a bikeshedding discussion and we can always > work it on-top. But if others have ideas about naming please speak up. I > think for example in the CrOS test suite uses '.' as separator instead > of '/'? So maybe there are some best-practices to be found. > > > +} > > + > > +/* > > + * Test single capture cycles > > + * > > + * Makes sure the camera completes the exact number of requests queued. Example > > + * failure is a camera that needs N+M requests queued to complete N requests to > > + * the application. > > + */ > > +TEST_P(FixedRequestsTest, BalancedSingleCycle) > > +{ > > + auto [role, numRequests] = GetParam(); > > + std::shared_ptr<Camera> camera = Environment::instance()->camera(); > > + > > SimpleCaptureBalanced capture(camera); > > > > - Results::Result ret = capture.configure(role); > > - if (ret.first != Results::Pass) > > - return ret; > > + capture.configure(role); > > > > - for (unsigned int starts = 0; starts < startCycles; starts++) { > > - ret = capture.capture(numRequests); > > - if (ret.first != Results::Pass) > > - return ret; > > - } > > + capture.capture(numRequests); > > +}; > > > > - return { Results::Pass, "Balanced capture of " + > > - std::to_string(numRequests) + " requests with " + > > - std::to_string(startCycles) + " start cycles" }; > > -} > > +/* > > + * Test multiple start/stop cycles > > + * > > + * Makes sure the camera supports multiple start/stop cycles. Example failure is > > + * a camera that does not clean up correctly in its error path but is only > > + * tested by single-capture applications. > > + */ > > +TEST_P(FixedRequestsTest, BalancedMultiCycle) > > +{ > > + auto [role, numRequests] = GetParam(); > > + std::shared_ptr<Camera> camera = Environment::instance()->camera(); > > + unsigned int numRepeats = 3; > > + > > + SimpleCaptureBalanced capture(camera); > > > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > > - StreamRole role, unsigned int numRequests) > > + capture.configure(role); > > + > > + for (unsigned int starts = 0; starts < numRepeats; starts++) > > + capture.capture(numRequests); > > +}; > > + > > +/* > > + * Test unbalanced stop > > + * > > + * Makes sure the camera supports a stop with requests queued. Example failure > > + * is a camera that does not handle cancelation of buffers coming back from the > > + * video device while stopping. > > + */ > > +TEST_P(FixedRequestsTest, Unbalanced) > > { > > + auto [role, numRequests] = GetParam(); > > + std::shared_ptr<Camera> camera = Environment::instance()->camera(); > > + > > SimpleCaptureUnbalanced capture(camera); > > > > - Results::Result ret = capture.configure(role); > > - if (ret.first != Results::Pass) > > - return ret; > > + capture.configure(role); > > > > - return capture.capture(numRequests); > > -} > > + capture.capture(numRequests); > > +}; > > > > -Results testSingleStream(std::shared_ptr<Camera> camera) > > -{ > > - static const std::vector<std::pair<std::string, StreamRole>> roles = { > > - { "raw", Raw }, > > - { "still", StillCapture }, > > - { "video", VideoRecording }, > > - { "viewfinder", Viewfinder }, > > - }; > > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > > - > > - Results results(numRequests.size() * roles.size() * 3); > > - > > - for (const auto &role : roles) { > > - std::cout << "= Test role " << role.first << std::endl; > > - /* > > - * Test single capture cycles > > - * > > - * Makes sure the camera completes the exact number of requests queued. > > - * Example failure is a camera that needs N+M requests queued to > > - * complete N requests to the application. > > - */ > > - std::cout << "* Test single capture cycles" << std::endl; > > - for (unsigned int num : numRequests) > > - results.add(testRequestBalance(camera, role.second, 1, num)); > > - > > - /* > > - * Test multiple start/stop cycles > > - * > > - * Makes sure the camera supports multiple start/stop cycles. > > - * Example failure is a camera that does not clean up correctly in its > > - * error path but is only tested by single-capture applications. > > - */ > > - std::cout << "* Test multiple start/stop cycles" << std::endl; > > - for (unsigned int num : numRequests) > > - results.add(testRequestBalance(camera, role.second, 3, num)); > > - > > - /* > > - * Test unbalanced stop > > - * > > - * Makes sure the camera supports a stop with requests queued. > > - * Example failure is a camera that does not handle cancelation > > - * of buffers coming back from the video device while stopping. > > - */ > > - std::cout << "* Test unbalanced stop" << std::endl; > > - for (unsigned int num : numRequests) > > - results.add(testRequestUnbalance(camera, role.second, num)); > > - } > > - > > - return results; > > -} > > + > > +INSTANTIATE_TEST_SUITE_P(RolesAndRequests, > > + FixedRequestsTest, > > + testing::Combine(testing::ValuesIn(ROLES), > > + testing::ValuesIn(NUMREQUESTS)), > > + nameParameters); > > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h > > index 396605214e4b..076785a7185f 100644 > > --- a/src/lc-compliance/tests.h > > +++ b/src/lc-compliance/tests.h > > @@ -11,6 +11,23 @@ > > > > #include "results.h" > > > > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); > > +using namespace libcamera; > > + > > +class Environment > > +{ > > +public: > > + static bool create(std::shared_ptr<Camera> camera); > > + void destroy(); > > + > > + static Environment *instance(); > > + > > + std::shared_ptr<Camera> camera() const { return camera_; } > > + > > +private: > > + Environment() {}; > > + > > + static Environment *instance_; > > + static std::shared_ptr<Camera> camera_; > > +}; > > > > #endif /* __LC_COMPLIANCE_TESTS_H__ */ > > -- > > 2.31.1 > > > > -- > Regards, > Niklas Söderlund
diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp index 54cee54aa978..d5ff93f514df 100644 --- a/src/lc-compliance/main.cpp +++ b/src/lc-compliance/main.cpp @@ -9,6 +9,8 @@ #include <iostream> #include <string.h> +#include <gtest/gtest.h> + #include <libcamera/libcamera.h> #include "../cam/options.h" @@ -21,21 +23,43 @@ enum { OptHelp = 'h', }; +bool Environment::create(std::shared_ptr<Camera> camera) +{ + if (instance_) + return false; + + camera_ = camera; + instance_ = new Environment; + return true; +} + +void Environment::destroy() +{ + if (!camera_) + return; + + camera_->release(); + camera_.reset(); +} + +Environment *Environment::instance() +{ + return instance_; +} + class Harness { public: Harness(const OptionsParser::Options &options); ~Harness(); - int exec(); + int init(); private: - int init(); void listCameras(); OptionsParser::Options options_; std::unique_ptr<CameraManager> cm_; - std::shared_ptr<Camera> camera_; }; Harness::Harness(const OptionsParser::Options &options) @@ -46,35 +70,15 @@ Harness::Harness(const OptionsParser::Options &options) Harness::~Harness() { - if (camera_) { - camera_->release(); - camera_.reset(); - } + Environment::instance()->destroy(); cm_->stop(); } -int Harness::exec() -{ - int ret = init(); - if (ret) - return ret; - - std::vector<Results> results; - - results.push_back(testSingleStream(camera_)); - - for (const Results &result : results) { - ret = result.summary(); - if (ret) - return ret; - } - - return 0; -} - int Harness::init() { + std::shared_ptr<Camera> camera; + int ret = cm_->start(); if (ret) { std::cout << "Failed to start camera manager: " @@ -89,18 +93,20 @@ int Harness::init() } const std::string &cameraId = options_[OptCamera]; - camera_ = cm_->get(cameraId); - if (!camera_) { + camera = cm_->get(cameraId); + if (!camera) { std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; listCameras(); return -ENODEV; } - if (camera_->acquire()) { + if (camera->acquire()) { std::cout << "Failed to acquire camera" << std::endl; return -EINVAL; } + Environment::create(camera); + std::cout << "Using camera " << cameraId << std::endl; return 0; @@ -133,6 +139,23 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) return 0; } +/* + * Make asserts act like exceptions, otherwise they only fail (or skip) the + * current function. From gtest documentation: + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception + */ +class ThrowListener : public testing::EmptyTestEventListener { + void OnTestPartResult(const testing::TestPartResult& result) override { + if (result.type() == testing::TestPartResult::kFatalFailure + || result.type() == testing::TestPartResult::kSkip) { + throw testing::AssertionException(result); + } + } +}; + +Environment *Environment::instance_ = nullptr; +std::shared_ptr<Camera> Environment::camera_ = nullptr; + int main(int argc, char **argv) { OptionsParser::Options options; @@ -143,6 +166,11 @@ int main(int argc, char **argv) return EXIT_FAILURE; Harness harness(options); + ret = harness.init(); + if (ret) + return ret; - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; + ::testing::InitGoogleTest(&argc, argv); + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); + return RUN_ALL_TESTS(); } diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build index a2bfcceb1259..704bc18af3e1 100644 --- a/src/lc-compliance/meson.build +++ b/src/lc-compliance/meson.build @@ -18,10 +18,13 @@ lc_compliance_sources = files([ 'single_stream.cpp', ]) +gtest_dep = dependency('gtest') + lc_compliance = executable('lc-compliance', lc_compliance_sources, dependencies : [ libatomic, libcamera_dep, libevent, + gtest_dep, ], install : true) diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp index f90fe6d0f9aa..7731eb16f8c2 100644 --- a/src/lc-compliance/simple_capture.cpp +++ b/src/lc-compliance/simple_capture.cpp @@ -5,6 +5,8 @@ * simple_capture.cpp - Simple capture helper */ +#include <gtest/gtest.h> + #include "simple_capture.h" using namespace libcamera; @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() stop(); } -Results::Result SimpleCapture::configure(StreamRole role) +void SimpleCapture::configure(StreamRole role) { config_ = camera_->generateConfiguration({ role }); - if (!config_) - return { Results::Skip, "Role not supported by camera" }; + if (!config_) { + std::cout << "Role not supported by camera" << std::endl; + GTEST_SKIP(); + } if (config_->validate() != CameraConfiguration::Valid) { config_.reset(); - return { Results::Fail, "Configuration not valid" }; + FAIL() << "Configuration not valid"; } if (camera_->configure(config_.get())) { config_.reset(); - return { Results::Fail, "Failed to configure camera" }; + FAIL() << "Failed to configure camera"; } - - return { Results::Pass, "Configure camera" }; } -Results::Result SimpleCapture::start() +void SimpleCapture::start() { Stream *stream = config_->at(0).stream(); - if (allocator_->allocate(stream) < 0) - return { Results::Fail, "Failed to allocate buffers" }; + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; - if (camera_->start()) - return { Results::Fail, "Failed to start camera" }; + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); - - return { Results::Pass, "Started camera" }; } void SimpleCapture::stop() @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) { } -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) +void SimpleCaptureBalanced::capture(unsigned int numRequests) { - Results::Result ret = start(); - if (ret.first != Results::Pass) - return ret; + start(); Stream *stream = config_->at(0).stream(); const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); /* No point in testing less requests then the camera depth. */ if (buffers.size() > numRequests) { - /* Cache buffers.size() before we destroy it in stop() */ - int buffers_size = buffers.size(); - - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) - + " requests, can't test only " + std::to_string(numRequests) }; + std::cout << "Camera needs " + std::to_string(buffers.size()) + + " requests, can't test only " + + std::to_string(numRequests) << std::endl; + GTEST_SKIP(); } queueCount_ = 0; @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) std::vector<std::unique_ptr<libcamera::Request>> requests; for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { std::unique_ptr<Request> request = camera_->createRequest(); - if (!request) - return { Results::Fail, "Can't create request" }; + ASSERT_TRUE(request) << "Can't create request"; - if (request->addBuffer(stream, buffer.get())) - return { Results::Fail, "Can't set buffer for request" }; + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; - if (queueRequest(request.get()) < 0) - return { Results::Fail, "Failed to queue request" }; + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; requests.push_back(std::move(request)); } @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) stop(); delete loop_; - if (captureCount_ != captureLimit_) - return { Results::Fail, "Got " + std::to_string(captureCount_) + - " request, wanted " + std::to_string(captureLimit_) }; - - return { Results::Pass, "Balanced capture of " + - std::to_string(numRequests) + " requests" }; + ASSERT_EQ(captureCount_, captureLimit_); } int SimpleCaptureBalanced::queueRequest(Request *request) @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) { } -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) { - Results::Result ret = start(); - if (ret.first != Results::Pass) - return ret; + start(); Stream *stream = config_->at(0).stream(); const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) std::vector<std::unique_ptr<libcamera::Request>> requests; for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { std::unique_ptr<Request> request = camera_->createRequest(); - if (!request) - return { Results::Fail, "Can't create request" }; + ASSERT_TRUE(request) << "Can't create request"; - if (request->addBuffer(stream, buffer.get())) - return { Results::Fail, "Can't set buffer for request" }; + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; - if (camera_->queueRequest(request.get()) < 0) - return { Results::Fail, "Failed to queue request" }; + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; requests.push_back(std::move(request)); } @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) stop(); delete loop_; - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; + ASSERT_FALSE(status); } void SimpleCaptureUnbalanced::requestComplete(Request *request) diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h index d9de53fb63a3..0f8465083456 100644 --- a/src/lc-compliance/simple_capture.h +++ b/src/lc-compliance/simple_capture.h @@ -17,13 +17,13 @@ class SimpleCapture { public: - Results::Result configure(libcamera::StreamRole role); + void configure(libcamera::StreamRole role); protected: SimpleCapture(std::shared_ptr<libcamera::Camera> camera); virtual ~SimpleCapture(); - Results::Result start(); + void start(); void stop(); virtual void requestComplete(libcamera::Request *request) = 0; @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture public: SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); - Results::Result capture(unsigned int numRequests); + void capture(unsigned int numRequests); private: int queueRequest(libcamera::Request *request); @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture public: SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); - Results::Result capture(unsigned int numRequests); + void capture(unsigned int numRequests); private: void requestComplete(libcamera::Request *request) override; diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp index 8318b42f42d6..aad32ecd051e 100644 --- a/src/lc-compliance/single_stream.cpp +++ b/src/lc-compliance/single_stream.cpp @@ -7,91 +7,95 @@ #include <iostream> +#include <gtest/gtest.h> + #include "simple_capture.h" #include "tests.h" using namespace libcamera; -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, - StreamRole role, unsigned int startCycles, - unsigned int numRequests) +const std::vector<int> NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; +const std::vector<StreamRole> ROLES = {Raw, StillCapture, VideoRecording, Viewfinder}; + +class FixedRequestsTest : + public testing::TestWithParam<std::tuple<StreamRole, int>> { +}; + +std::string nameParameters(const testing::TestParamInfo<FixedRequestsTest::ParamType>& info) { + std::map<StreamRole, std::string> rolesMap = {{Raw, "raw"}, + {StillCapture, "still"}, + {VideoRecording, "video"}, + {Viewfinder, "viewfinder"}}; + + std::string roleName = rolesMap[std::get<0>(info.param)]; + std::string numRequestsName = std::to_string(std::get<1>(info.param)); + + return roleName + numRequestsName; +} + +/* + * Test single capture cycles + * + * Makes sure the camera completes the exact number of requests queued. Example + * failure is a camera that needs N+M requests queued to complete N requests to + * the application. + */ +TEST_P(FixedRequestsTest, BalancedSingleCycle) +{ + auto [role, numRequests] = GetParam(); + std::shared_ptr<Camera> camera = Environment::instance()->camera(); + SimpleCaptureBalanced capture(camera); - Results::Result ret = capture.configure(role); - if (ret.first != Results::Pass) - return ret; + capture.configure(role); - for (unsigned int starts = 0; starts < startCycles; starts++) { - ret = capture.capture(numRequests); - if (ret.first != Results::Pass) - return ret; - } + capture.capture(numRequests); +}; - return { Results::Pass, "Balanced capture of " + - std::to_string(numRequests) + " requests with " + - std::to_string(startCycles) + " start cycles" }; -} +/* + * Test multiple start/stop cycles + * + * Makes sure the camera supports multiple start/stop cycles. Example failure is + * a camera that does not clean up correctly in its error path but is only + * tested by single-capture applications. + */ +TEST_P(FixedRequestsTest, BalancedMultiCycle) +{ + auto [role, numRequests] = GetParam(); + std::shared_ptr<Camera> camera = Environment::instance()->camera(); + unsigned int numRepeats = 3; + + SimpleCaptureBalanced capture(camera); -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, - StreamRole role, unsigned int numRequests) + capture.configure(role); + + for (unsigned int starts = 0; starts < numRepeats; starts++) + capture.capture(numRequests); +}; + +/* + * Test unbalanced stop + * + * Makes sure the camera supports a stop with requests queued. Example failure + * is a camera that does not handle cancelation of buffers coming back from the + * video device while stopping. + */ +TEST_P(FixedRequestsTest, Unbalanced) { + auto [role, numRequests] = GetParam(); + std::shared_ptr<Camera> camera = Environment::instance()->camera(); + SimpleCaptureUnbalanced capture(camera); - Results::Result ret = capture.configure(role); - if (ret.first != Results::Pass) - return ret; + capture.configure(role); - return capture.capture(numRequests); -} + capture.capture(numRequests); +}; -Results testSingleStream(std::shared_ptr<Camera> camera) -{ - static const std::vector<std::pair<std::string, StreamRole>> roles = { - { "raw", Raw }, - { "still", StillCapture }, - { "video", VideoRecording }, - { "viewfinder", Viewfinder }, - }; - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; - - Results results(numRequests.size() * roles.size() * 3); - - for (const auto &role : roles) { - std::cout << "= Test role " << role.first << std::endl; - /* - * Test single capture cycles - * - * Makes sure the camera completes the exact number of requests queued. - * Example failure is a camera that needs N+M requests queued to - * complete N requests to the application. - */ - std::cout << "* Test single capture cycles" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestBalance(camera, role.second, 1, num)); - - /* - * Test multiple start/stop cycles - * - * Makes sure the camera supports multiple start/stop cycles. - * Example failure is a camera that does not clean up correctly in its - * error path but is only tested by single-capture applications. - */ - std::cout << "* Test multiple start/stop cycles" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestBalance(camera, role.second, 3, num)); - - /* - * Test unbalanced stop - * - * Makes sure the camera supports a stop with requests queued. - * Example failure is a camera that does not handle cancelation - * of buffers coming back from the video device while stopping. - */ - std::cout << "* Test unbalanced stop" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestUnbalance(camera, role.second, num)); - } - - return results; -} + +INSTANTIATE_TEST_SUITE_P(RolesAndRequests, + FixedRequestsTest, + testing::Combine(testing::ValuesIn(ROLES), + testing::ValuesIn(NUMREQUESTS)), + nameParameters); diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h index 396605214e4b..076785a7185f 100644 --- a/src/lc-compliance/tests.h +++ b/src/lc-compliance/tests.h @@ -11,6 +11,23 @@ #include "results.h" -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); +using namespace libcamera; + +class Environment +{ +public: + static bool create(std::shared_ptr<Camera> camera); + void destroy(); + + static Environment *instance(); + + std::shared_ptr<Camera> camera() const { return camera_; } + +private: + Environment() {}; + + static Environment *instance_; + static std::shared_ptr<Camera> camera_; +}; #endif /* __LC_COMPLIANCE_TESTS_H__ */
Refactor lc-compliance using Googletest as the test framework. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- Changes in v3: - Thanks to Niklas: - Went back to static test registration, and created a Singleton Environment class to store the camera global variable - Added a nameParameters() function to give meaningful names to the test parameters, removing the need to register each test suite for every role src/lc-compliance/main.cpp | 88 ++++++++++------ src/lc-compliance/meson.build | 3 + src/lc-compliance/simple_capture.cpp | 74 +++++--------- src/lc-compliance/simple_capture.h | 8 +- src/lc-compliance/single_stream.cpp | 148 ++++++++++++++------------- src/lc-compliance/tests.h | 19 +++- 6 files changed, 187 insertions(+), 153 deletions(-)