| Message ID | 20210422210609.425987-3-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hello Nícolas, Thanks for your work. On 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote: > Refactor lc-compliance using Googletest as the test framework. I really like the gtest rework from a first reading of this patch. Unfortunately it no longer applies to master so I have not been able to take it for a test drive. I like that we get most if not all features we discussed such as skip and test suite/case selection form the CLI more or less out-for-the box for free. Some random comments below. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > src/lc-compliance/main.cpp | 60 +++++------ > src/lc-compliance/meson.build | 3 + > src/lc-compliance/simple_capture.cpp | 73 +++++--------- > src/lc-compliance/simple_capture.h | 8 +- > src/lc-compliance/single_stream.cpp | 142 ++++++++++++++------------- > 5 files changed, 132 insertions(+), 154 deletions(-) > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > index 54cee54aa978..04cd0732ee3d 100644 > --- a/src/lc-compliance/main.cpp > +++ b/src/lc-compliance/main.cpp > @@ -14,8 +14,12 @@ > #include "../cam/options.h" > #include "tests.h" > > +#include "gtest/gtest.h" As gtest is pulled in as an dependably and not added to the libcamera tee should this not be <gtest/gtest.h> ? > + > using namespace libcamera; > > +std::shared_ptr<Camera> cam; > + Would it be possible to factor the code so that the Camera instance is not a global variable? I'm thinking that this will not scale when tests requiring more then one camera are needed. > enum { > OptCamera = 'c', > OptHelp = 'h', > @@ -28,14 +32,13 @@ public: > ~Harness(); > > int exec(); > + int init(); > > private: > - int init(); > void listCameras(); > > OptionsParser::Options options_; > std::unique_ptr<CameraManager> cm_; > - std::shared_ptr<Camera> camera_; > }; > > Harness::Harness(const OptionsParser::Options &options) > @@ -46,33 +49,14 @@ Harness::Harness(const OptionsParser::Options &options) > > Harness::~Harness() > { > - if (camera_) { > - camera_->release(); > - camera_.reset(); > + if (cam) { > + cam->release(); > + cam.reset(); > } > > cm_->stop(); > } > > -int Harness::exec() > -{ > - int ret = init(); > - if (ret) > - return ret; > - > - std::vector<Results> results; > - > - results.push_back(testSingleStream(camera_)); > - > - for (const Results &result : results) { > - ret = result.summary(); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > int Harness::init() > { > int ret = cm_->start(); > @@ -89,14 +73,14 @@ int Harness::init() > } > > const std::string &cameraId = options_[OptCamera]; > - camera_ = cm_->get(cameraId); > - if (!camera_) { > + cam = cm_->get(cameraId); > + if (!cam) { > std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > listCameras(); > return -ENODEV; > } > > - if (camera_->acquire()) { > + if (cam->acquire()) { > std::cout << "Failed to acquire camera" << std::endl; > return -EINVAL; > } > @@ -108,8 +92,8 @@ int Harness::init() > > void Harness::listCameras() > { > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > - std::cout << "- " << cam.get()->id() << std::endl; > + for (const std::shared_ptr<Camera> &c : cm_->cameras()) > + std::cout << "- " << c.get()->id() << std::endl; > } > > static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > @@ -133,6 +117,17 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > return 0; > } > > +/* Make asserts act like exceptions, otherwise they only fail (or skip) the > + * current function */ nit: /* * Multieline comments start * with a blank line. */ > +class ThrowListener : public testing::EmptyTestEventListener { > + void OnTestPartResult(const testing::TestPartResult& result) override { > + if (result.type() == testing::TestPartResult::kFatalFailure > + || result.type() == testing::TestPartResult::kSkip) { > + throw testing::AssertionException(result); > + } > + } > +}; Indentation looks odd here. > + > int main(int argc, char **argv) > { > OptionsParser::Options options; > @@ -143,6 +138,11 @@ int main(int argc, char **argv) > return EXIT_FAILURE; > > Harness harness(options); > + ret = harness.init(); > + if (ret) > + return ret; > > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; > + ::testing::InitGoogleTest(&argc, argv); > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); > + return RUN_ALL_TESTS(); > } > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > index a2bfcceb1259..4c41ce6da43a 100644 > --- a/src/lc-compliance/meson.build > +++ b/src/lc-compliance/meson.build > @@ -18,10 +18,13 @@ lc_compliance_sources = files([ > 'single_stream.cpp', > ]) > > +gtest_dep = dependency('gtest', main: true) > + > lc_compliance = executable('lc-compliance', lc_compliance_sources, > dependencies : [ > libatomic, > libcamera_dep, > libevent, > + gtest_dep Appaned a trailing ','. This is done to prepare for the next time this file needs to be edited as the diff will then be easier to read as it will only touch new lines. > ], > install : true) > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > index e6715ac07f12..24abbff138a3 100644 > --- a/src/lc-compliance/simple_capture.cpp > +++ b/src/lc-compliance/simple_capture.cpp > @@ -6,6 +6,7 @@ > */ > > #include "simple_capture.h" > +#include "gtest/gtest.h" > > using namespace libcamera; > > @@ -20,35 +21,29 @@ SimpleCapture::~SimpleCapture() > stop(); > } > > -Results::Result SimpleCapture::configure(StreamRole role) > +void SimpleCapture::configure(StreamRole role) > { > config_ = camera_->generateConfiguration({ role }); > > if (config_->validate() != CameraConfiguration::Valid) { > config_.reset(); > - return { Results::Fail, "Configuration not valid" }; > + FAIL() << "Configuration not valid"; > } > > if (camera_->configure(config_.get())) { > config_.reset(); > - return { Results::Fail, "Failed to configure camera" }; > + FAIL() << "Failed to configure camera"; > } > - > - return { Results::Pass, "Configure camera" }; > } > > -Results::Result SimpleCapture::start() > +void SimpleCapture::start() > { > Stream *stream = config_->at(0).stream(); > - if (allocator_->allocate(stream) < 0) > - return { Results::Fail, "Failed to allocate buffers" }; > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; I like these asserts instead of the return type I used in the initial version ;-) > > - if (camera_->start()) > - return { Results::Fail, "Failed to start camera" }; > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > - > - return { Results::Pass, "Started camera" }; > } > > void SimpleCapture::stop() > @@ -73,22 +68,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) > { > } > > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > +void SimpleCaptureBalanced::capture(unsigned int numRequests) > { > - Results::Result ret = start(); > - if (ret.first != Results::Pass) > - return ret; > + start(); > > Stream *stream = config_->at(0).stream(); > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > /* No point in testing less requests then the camera depth. */ > if (buffers.size() > numRequests) { > - /* Cache buffers.size() before we destroy it in stop() */ > - int buffers_size = buffers.size(); > - > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > - + " requests, can't test only " + std::to_string(numRequests) }; > + std::cout << "Camera needs " + std::to_string(buffers.size()) > + + " requests, can't test only " > + + std::to_string(numRequests) << std::endl; > + GTEST_SKIP(); > } > > queueCount_ = 0; > @@ -99,17 +91,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > std::vector<std::unique_ptr<libcamera::Request>> requests; > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > std::unique_ptr<Request> request = camera_->createRequest(); > - if (!request) { > - return { Results::Fail, "Can't create request" }; > - } > + ASSERT_TRUE(request) << "Can't create request"; > > - if (request->addBuffer(stream, buffer.get())) { > - return { Results::Fail, "Can't set buffer for request" }; > - } > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > - if (queueRequest(request.get()) < 0) { > - return { Results::Fail, "Failed to queue request" }; > - } > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; > > requests.push_back(std::move(request)); > } > @@ -120,12 +106,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > stop(); > delete loop_; > > - if (captureCount_ != captureLimit_) > - return { Results::Fail, "Got " + std::to_string(captureCount_) + > - " request, wanted " + std::to_string(captureLimit_) }; > - > - return { Results::Pass, "Balanced capture of " + > - std::to_string(numRequests) + " requests" }; > + ASSERT_EQ(captureCount_, captureLimit_); > } > > int SimpleCaptureBalanced::queueRequest(Request *request) > @@ -157,11 +138,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) > { > } > > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) > { > - Results::Result ret = start(); > - if (ret.first != Results::Pass) > - return ret; > + start(); > > Stream *stream = config_->at(0).stream(); > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > @@ -173,17 +152,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > std::vector<std::unique_ptr<libcamera::Request>> requests; > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > std::unique_ptr<Request> request = camera_->createRequest(); > - if (!request) { > - return { Results::Fail, "Can't create request" }; > - } > + ASSERT_TRUE(request) << "Can't create request"; > > - if (request->addBuffer(stream, buffer.get())) { > - return { Results::Fail, "Can't set buffer for request" }; > - } > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > - if (camera_->queueRequest(request.get()) < 0) { > - return { Results::Fail, "Failed to queue request" }; > - } > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > requests.push_back(std::move(request)); > } > @@ -194,7 +167,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > stop(); > delete loop_; > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > + ASSERT_FALSE(status); > } > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > index d9de53fb63a3..0f8465083456 100644 > --- a/src/lc-compliance/simple_capture.h > +++ b/src/lc-compliance/simple_capture.h > @@ -17,13 +17,13 @@ > class SimpleCapture > { > public: > - Results::Result configure(libcamera::StreamRole role); > + void configure(libcamera::StreamRole role); > > protected: > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > virtual ~SimpleCapture(); > > - Results::Result start(); > + void start(); > void stop(); > > virtual void requestComplete(libcamera::Request *request) = 0; > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture > public: > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); > > - Results::Result capture(unsigned int numRequests); > + void capture(unsigned int numRequests); > > private: > int queueRequest(libcamera::Request *request); > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture > public: > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); > > - Results::Result capture(unsigned int numRequests); > + void capture(unsigned int numRequests); > > private: > void requestComplete(libcamera::Request *request) override; > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > index 8318b42f42d6..18830cd9bce5 100644 > --- a/src/lc-compliance/single_stream.cpp > +++ b/src/lc-compliance/single_stream.cpp > @@ -6,92 +6,94 @@ > */ > > #include <iostream> > +#include <sstream> > > #include "simple_capture.h" > #include "tests.h" > +#include "gtest/gtest.h" > > using namespace libcamera; > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > - StreamRole role, unsigned int startCycles, > - unsigned int numRequests) > +extern std::shared_ptr<Camera> cam; > + > +std::vector<int> NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; > + > +class FixedRequestsTest : > + public testing::TestWithParam<std::tuple<StreamRole, int>> { > +}; > + > +/* > + * Test single capture cycles > + * > + * Makes sure the camera completes the exact number of requests queued. Example > + * failure is a camera that needs N+M requests queued to complete N requests to > + * the application. > + */ > +TEST_P(FixedRequestsTest, BalancedSingleCycle) > { > - SimpleCaptureBalanced capture(camera); > + auto [role, numRequests] = GetParam(); > > - Results::Result ret = capture.configure(role); > - if (ret.first != Results::Pass) > - return ret; > + SimpleCaptureBalanced capture(cam); > > - for (unsigned int starts = 0; starts < startCycles; starts++) { > - ret = capture.capture(numRequests); > - if (ret.first != Results::Pass) > - return ret; > - } > + capture.configure(role); > > - return { Results::Pass, "Balanced capture of " + > - std::to_string(numRequests) + " requests with " + > - std::to_string(startCycles) + " start cycles" }; > + capture.capture(numRequests); > } > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > - StreamRole role, unsigned int numRequests) > +/* > + * Test multiple start/stop cycles > + * > + * Makes sure the camera supports multiple start/stop cycles. Example failure is > + * a camera that does not clean up correctly in its error path but is only > + * tested by single-capture applications. > + */ > +TEST_P(FixedRequestsTest, BalancedMultiCycle) > { > - SimpleCaptureUnbalanced capture(camera); > + auto [role, numRequests] = GetParam(); > + unsigned int numRepeats = 3; > > - Results::Result ret = capture.configure(role); > - if (ret.first != Results::Pass) > - return ret; > + SimpleCaptureBalanced capture(cam); > > - return capture.capture(numRequests); > + capture.configure(role); > + > + for (unsigned int starts = 0; starts < numRepeats; starts++) > + capture.capture(numRequests); > } > > -Results testSingleStream(std::shared_ptr<Camera> camera) > +/* > + * Test unbalanced stop > + * > + * Makes sure the camera supports a stop with requests queued. Example failure > + * is a camera that does not handle cancelation of buffers coming back from the > + * video device while stopping. > + */ > +TEST_P(FixedRequestsTest, Unbalanced) > { > - static const std::vector<std::pair<std::string, StreamRole>> roles = { > - { "raw", Raw }, > - { "still", StillCapture }, > - { "video", VideoRecording }, > - { "viewfinder", Viewfinder }, > - }; > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > - > - Results results(numRequests.size() * roles.size() * 3); > - > - for (const auto &role : roles) { > - std::cout << "= Test role " << role.first << std::endl; > - /* > - * Test single capture cycles > - * > - * Makes sure the camera completes the exact number of requests queued. > - * Example failure is a camera that needs N+M requests queued to > - * complete N requests to the application. > - */ > - std::cout << "* Test single capture cycles" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestBalance(camera, role.second, 1, num)); > - > - /* > - * Test multiple start/stop cycles > - * > - * Makes sure the camera supports multiple start/stop cycles. > - * Example failure is a camera that does not clean up correctly in its > - * error path but is only tested by single-capture applications. > - */ > - std::cout << "* Test multiple start/stop cycles" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestBalance(camera, role.second, 3, num)); > - > - /* > - * Test unbalanced stop > - * > - * Makes sure the camera supports a stop with requests queued. > - * Example failure is a camera that does not handle cancelation > - * of buffers coming back from the video device while stopping. > - */ > - std::cout << "* Test unbalanced stop" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestUnbalance(camera, role.second, num)); > - } > - > - return results; > + auto [role, numRequests] = GetParam(); > + > + SimpleCaptureUnbalanced capture(cam); > + > + capture.configure(role); > + > + capture.capture(numRequests); > } > + > +INSTANTIATE_TEST_SUITE_P(Raw, > + FixedRequestsTest, > + testing::Combine(testing::Values(Raw), > + testing::ValuesIn(NUMREQUESTS))); > + > +INSTANTIATE_TEST_SUITE_P(StillCapture, > + FixedRequestsTest, > + testing::Combine(testing::Values(StillCapture), > + testing::ValuesIn(NUMREQUESTS))); > + > +INSTANTIATE_TEST_SUITE_P(VideoRecording, > + FixedRequestsTest, > + testing::Combine(testing::Values(VideoRecording), > + testing::ValuesIn(NUMREQUESTS))); > + > +INSTANTIATE_TEST_SUITE_P(Viewfinder, > + FixedRequestsTest, > + testing::Combine(testing::Values(Viewfinder), > + testing::ValuesIn(NUMREQUESTS))); > -- > 2.31.1 >
Hi Niklas, thanks for the feedback. Em 2021-04-29 08:48, Niklas Söderlund escreveu: > Hello Nícolas, > > Thanks for your work. > > On 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote: > > Refactor lc-compliance using Googletest as the test framework. > > I really like the gtest rework from a first reading of this patch. > Unfortunately it no longer applies to master so I have not been able to > take it for a test drive. > > I like that we get most if not all features we discussed such as skip > and test suite/case selection form the CLI more or less out-for-the box > for free. > > Some random comments below. > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > src/lc-compliance/main.cpp | 60 +++++------ > > src/lc-compliance/meson.build | 3 + > > src/lc-compliance/simple_capture.cpp | 73 +++++--------- > > src/lc-compliance/simple_capture.h | 8 +- > > src/lc-compliance/single_stream.cpp | 142 ++++++++++++++------------- > > 5 files changed, 132 insertions(+), 154 deletions(-) > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > index 54cee54aa978..04cd0732ee3d 100644 > > --- a/src/lc-compliance/main.cpp > > +++ b/src/lc-compliance/main.cpp > > @@ -14,8 +14,12 @@ > > #include "../cam/options.h" > > #include "tests.h" > > > > +#include "gtest/gtest.h" > > As gtest is pulled in as an dependably and not added to the libcamera > tee should this not be <gtest/gtest.h> ? It should indeed. Fixed for v2. > > > + > > using namespace libcamera; > > > > +std::shared_ptr<Camera> cam; > > + > > Would it be possible to factor the code so that the Camera instance is > not a global variable? I'm thinking that this will not scale when tests > requiring more then one camera are needed. So... This is something that annoyed me as well. The short answer is I think it could, but the test registration code wouldn't be as clean. The not so short answer: The only way I think this could be done is by adding a parameter to every test case with the camera instance. But I'm currently registering the test suites statically: INSTANTIATE_TEST_SUITE_P(Raw, FixedRequestsTest, testing::Combine(testing::Values(Raw), testing::ValuesIn(NUMREQUESTS))); So in order to pass the camera instance, they would need to be registered dynamically, as the example in [1] shows, after the camera to use is parsed from the command line. [1] https://google.github.io/googletest/advanced.html#registering-tests-programmatically But indeed, this seems to be needed to support working with multiple cameras. The other solutions I was thinking about: making the global variable a vector of cameras and/or running gtest multiple times and each time with the camera variable set differently, wouldn't give a single report at the end with the results from all cameras which would make it harder to parse the results. I'm not sure which tests would need multiple cameras (if you think of examples let me know), but at least being able to run each of the tests for each of the cameras on a system in a single lc-compliance run would be important I think. So I'll take a stab at it and see how things go. Regarding the other issues, I'll have them fixed for v2. Thanks, Nícolas
Hello Nícolas, Thanks for your efforts! On 2021-04-30 10:19:03 -0300, Nícolas F. R. A. Prado wrote: > Hi Niklas, > > thanks for the feedback. > > Em 2021-04-29 08:48, Niklas Söderlund escreveu: > > > Hello Nícolas, > > > > Thanks for your work. > > > > On 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote: > > > Refactor lc-compliance using Googletest as the test framework. > > > > I really like the gtest rework from a first reading of this patch. > > Unfortunately it no longer applies to master so I have not been able to > > take it for a test drive. > > > > I like that we get most if not all features we discussed such as skip > > and test suite/case selection form the CLI more or less out-for-the box > > for free. > > > > Some random comments below. > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > --- > > > src/lc-compliance/main.cpp | 60 +++++------ > > > src/lc-compliance/meson.build | 3 + > > > src/lc-compliance/simple_capture.cpp | 73 +++++--------- > > > src/lc-compliance/simple_capture.h | 8 +- > > > src/lc-compliance/single_stream.cpp | 142 ++++++++++++++------------- > > > 5 files changed, 132 insertions(+), 154 deletions(-) > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > index 54cee54aa978..04cd0732ee3d 100644 > > > --- a/src/lc-compliance/main.cpp > > > +++ b/src/lc-compliance/main.cpp > > > @@ -14,8 +14,12 @@ > > > #include "../cam/options.h" > > > #include "tests.h" > > > > > > +#include "gtest/gtest.h" > > > > As gtest is pulled in as an dependably and not added to the libcamera > > tee should this not be <gtest/gtest.h> ? > > It should indeed. Fixed for v2. > > > > > > + > > > using namespace libcamera; > > > > > > +std::shared_ptr<Camera> cam; > > > + > > > > Would it be possible to factor the code so that the Camera instance is > > not a global variable? I'm thinking that this will not scale when tests > > requiring more then one camera are needed. > > So... This is something that annoyed me as well. The short answer is I think it > could, but the test registration code wouldn't be as clean. IMHO almost any mess that removes a global variable is worth it :-) > > The not so short answer: The only way I think this could be done is by adding a > parameter to every test case with the camera instance. But I'm currently > registering the test suites statically: > > INSTANTIATE_TEST_SUITE_P(Raw, > FixedRequestsTest, > testing::Combine(testing::Values(Raw), > testing::ValuesIn(NUMREQUESTS))); > > So in order to pass the camera instance, they would need to be registered > dynamically, as the example in [1] shows, after the camera to use is parsed from > the command line. > > [1] https://google.github.io/googletest/advanced.html#registering-tests-programmatically > > But indeed, this seems to be needed to support working with multiple cameras. > The other solutions I was thinking about: making the global variable a vector of > cameras and/or running gtest multiple times and each time with the camera > variable set differently, wouldn't give a single report at the end with the > results from all cameras which would make it harder to parse the results. > > I'm not sure which tests would need multiple cameras (if you think of examples > let me know), but at least being able to run each of the tests for each of the > cameras on a system in a single lc-compliance run would be important I think. It's a tricky question and its implementation may be even tricker. The first 'problem' I have in mind for this type of test are pipelines where some part of the capture pipeline are shared between two discrete libcamera cameras. If we look closer at the RkISP1 pipeline handler it has a activeCamera_ variable that designates witch camera is currently using the shared ISP. It would be cool if lc-compliance had a test case that somehow could figure out (either thru the API or informed using cli arguments) which cameras share resources and test that they error our correctly when tried to be used at the same time. > > So I'll take a stab at it and see how things go. > > Regarding the other issues, I'll have them fixed for v2. Nice! Remember the on-top definition. As long as your work only uses a 'previous bad idea' and not introduces a new one, it's fine to fix things on-top :-) > > Thanks, > Nícolas
diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp index 54cee54aa978..04cd0732ee3d 100644 --- a/src/lc-compliance/main.cpp +++ b/src/lc-compliance/main.cpp @@ -14,8 +14,12 @@ #include "../cam/options.h" #include "tests.h" +#include "gtest/gtest.h" + using namespace libcamera; +std::shared_ptr<Camera> cam; + enum { OptCamera = 'c', OptHelp = 'h', @@ -28,14 +32,13 @@ public: ~Harness(); int exec(); + int init(); private: - int init(); void listCameras(); OptionsParser::Options options_; std::unique_ptr<CameraManager> cm_; - std::shared_ptr<Camera> camera_; }; Harness::Harness(const OptionsParser::Options &options) @@ -46,33 +49,14 @@ Harness::Harness(const OptionsParser::Options &options) Harness::~Harness() { - if (camera_) { - camera_->release(); - camera_.reset(); + if (cam) { + cam->release(); + cam.reset(); } cm_->stop(); } -int Harness::exec() -{ - int ret = init(); - if (ret) - return ret; - - std::vector<Results> results; - - results.push_back(testSingleStream(camera_)); - - for (const Results &result : results) { - ret = result.summary(); - if (ret) - return ret; - } - - return 0; -} - int Harness::init() { int ret = cm_->start(); @@ -89,14 +73,14 @@ int Harness::init() } const std::string &cameraId = options_[OptCamera]; - camera_ = cm_->get(cameraId); - if (!camera_) { + cam = cm_->get(cameraId); + if (!cam) { std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; listCameras(); return -ENODEV; } - if (camera_->acquire()) { + if (cam->acquire()) { std::cout << "Failed to acquire camera" << std::endl; return -EINVAL; } @@ -108,8 +92,8 @@ int Harness::init() void Harness::listCameras() { - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) - std::cout << "- " << cam.get()->id() << std::endl; + for (const std::shared_ptr<Camera> &c : cm_->cameras()) + std::cout << "- " << c.get()->id() << std::endl; } static int parseOptions(int argc, char **argv, OptionsParser::Options *options) @@ -133,6 +117,17 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) return 0; } +/* Make asserts act like exceptions, otherwise they only fail (or skip) the + * current function */ +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); + } + } +}; + int main(int argc, char **argv) { OptionsParser::Options options; @@ -143,6 +138,11 @@ int main(int argc, char **argv) return EXIT_FAILURE; Harness harness(options); + ret = harness.init(); + if (ret) + return ret; - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; + ::testing::InitGoogleTest(&argc, argv); + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); + return RUN_ALL_TESTS(); } diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build index a2bfcceb1259..4c41ce6da43a 100644 --- a/src/lc-compliance/meson.build +++ b/src/lc-compliance/meson.build @@ -18,10 +18,13 @@ lc_compliance_sources = files([ 'single_stream.cpp', ]) +gtest_dep = dependency('gtest', main: true) + lc_compliance = executable('lc-compliance', lc_compliance_sources, dependencies : [ libatomic, libcamera_dep, libevent, + gtest_dep ], install : true) diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp index e6715ac07f12..24abbff138a3 100644 --- a/src/lc-compliance/simple_capture.cpp +++ b/src/lc-compliance/simple_capture.cpp @@ -6,6 +6,7 @@ */ #include "simple_capture.h" +#include "gtest/gtest.h" using namespace libcamera; @@ -20,35 +21,29 @@ SimpleCapture::~SimpleCapture() stop(); } -Results::Result SimpleCapture::configure(StreamRole role) +void SimpleCapture::configure(StreamRole role) { config_ = camera_->generateConfiguration({ role }); if (config_->validate() != CameraConfiguration::Valid) { config_.reset(); - return { Results::Fail, "Configuration not valid" }; + FAIL() << "Configuration not valid"; } if (camera_->configure(config_.get())) { config_.reset(); - return { Results::Fail, "Failed to configure camera" }; + FAIL() << "Failed to configure camera"; } - - return { Results::Pass, "Configure camera" }; } -Results::Result SimpleCapture::start() +void SimpleCapture::start() { Stream *stream = config_->at(0).stream(); - if (allocator_->allocate(stream) < 0) - return { Results::Fail, "Failed to allocate buffers" }; + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; - if (camera_->start()) - return { Results::Fail, "Failed to start camera" }; + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); - - return { Results::Pass, "Started camera" }; } void SimpleCapture::stop() @@ -73,22 +68,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) { } -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) +void SimpleCaptureBalanced::capture(unsigned int numRequests) { - Results::Result ret = start(); - if (ret.first != Results::Pass) - return ret; + start(); Stream *stream = config_->at(0).stream(); const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); /* No point in testing less requests then the camera depth. */ if (buffers.size() > numRequests) { - /* Cache buffers.size() before we destroy it in stop() */ - int buffers_size = buffers.size(); - - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) - + " requests, can't test only " + std::to_string(numRequests) }; + std::cout << "Camera needs " + std::to_string(buffers.size()) + + " requests, can't test only " + + std::to_string(numRequests) << std::endl; + GTEST_SKIP(); } queueCount_ = 0; @@ -99,17 +91,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) std::vector<std::unique_ptr<libcamera::Request>> requests; for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { std::unique_ptr<Request> request = camera_->createRequest(); - if (!request) { - return { Results::Fail, "Can't create request" }; - } + ASSERT_TRUE(request) << "Can't create request"; - if (request->addBuffer(stream, buffer.get())) { - return { Results::Fail, "Can't set buffer for request" }; - } + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; - if (queueRequest(request.get()) < 0) { - return { Results::Fail, "Failed to queue request" }; - } + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; requests.push_back(std::move(request)); } @@ -120,12 +106,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) stop(); delete loop_; - if (captureCount_ != captureLimit_) - return { Results::Fail, "Got " + std::to_string(captureCount_) + - " request, wanted " + std::to_string(captureLimit_) }; - - return { Results::Pass, "Balanced capture of " + - std::to_string(numRequests) + " requests" }; + ASSERT_EQ(captureCount_, captureLimit_); } int SimpleCaptureBalanced::queueRequest(Request *request) @@ -157,11 +138,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) { } -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) { - Results::Result ret = start(); - if (ret.first != Results::Pass) - return ret; + start(); Stream *stream = config_->at(0).stream(); const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); @@ -173,17 +152,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) std::vector<std::unique_ptr<libcamera::Request>> requests; for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { std::unique_ptr<Request> request = camera_->createRequest(); - if (!request) { - return { Results::Fail, "Can't create request" }; - } + ASSERT_TRUE(request) << "Can't create request"; - if (request->addBuffer(stream, buffer.get())) { - return { Results::Fail, "Can't set buffer for request" }; - } + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; - if (camera_->queueRequest(request.get()) < 0) { - return { Results::Fail, "Failed to queue request" }; - } + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; requests.push_back(std::move(request)); } @@ -194,7 +167,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) stop(); delete loop_; - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; + ASSERT_FALSE(status); } void SimpleCaptureUnbalanced::requestComplete(Request *request) diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h index d9de53fb63a3..0f8465083456 100644 --- a/src/lc-compliance/simple_capture.h +++ b/src/lc-compliance/simple_capture.h @@ -17,13 +17,13 @@ class SimpleCapture { public: - Results::Result configure(libcamera::StreamRole role); + void configure(libcamera::StreamRole role); protected: SimpleCapture(std::shared_ptr<libcamera::Camera> camera); virtual ~SimpleCapture(); - Results::Result start(); + void start(); void stop(); virtual void requestComplete(libcamera::Request *request) = 0; @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture public: SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); - Results::Result capture(unsigned int numRequests); + void capture(unsigned int numRequests); private: int queueRequest(libcamera::Request *request); @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture public: SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); - Results::Result capture(unsigned int numRequests); + void capture(unsigned int numRequests); private: void requestComplete(libcamera::Request *request) override; diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp index 8318b42f42d6..18830cd9bce5 100644 --- a/src/lc-compliance/single_stream.cpp +++ b/src/lc-compliance/single_stream.cpp @@ -6,92 +6,94 @@ */ #include <iostream> +#include <sstream> #include "simple_capture.h" #include "tests.h" +#include "gtest/gtest.h" using namespace libcamera; -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, - StreamRole role, unsigned int startCycles, - unsigned int numRequests) +extern std::shared_ptr<Camera> cam; + +std::vector<int> NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; + +class FixedRequestsTest : + public testing::TestWithParam<std::tuple<StreamRole, int>> { +}; + +/* + * Test single capture cycles + * + * Makes sure the camera completes the exact number of requests queued. Example + * failure is a camera that needs N+M requests queued to complete N requests to + * the application. + */ +TEST_P(FixedRequestsTest, BalancedSingleCycle) { - SimpleCaptureBalanced capture(camera); + auto [role, numRequests] = GetParam(); - Results::Result ret = capture.configure(role); - if (ret.first != Results::Pass) - return ret; + SimpleCaptureBalanced capture(cam); - for (unsigned int starts = 0; starts < startCycles; starts++) { - ret = capture.capture(numRequests); - if (ret.first != Results::Pass) - return ret; - } + capture.configure(role); - return { Results::Pass, "Balanced capture of " + - std::to_string(numRequests) + " requests with " + - std::to_string(startCycles) + " start cycles" }; + capture.capture(numRequests); } -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, - StreamRole role, unsigned int numRequests) +/* + * Test multiple start/stop cycles + * + * Makes sure the camera supports multiple start/stop cycles. Example failure is + * a camera that does not clean up correctly in its error path but is only + * tested by single-capture applications. + */ +TEST_P(FixedRequestsTest, BalancedMultiCycle) { - SimpleCaptureUnbalanced capture(camera); + auto [role, numRequests] = GetParam(); + unsigned int numRepeats = 3; - Results::Result ret = capture.configure(role); - if (ret.first != Results::Pass) - return ret; + SimpleCaptureBalanced capture(cam); - return capture.capture(numRequests); + capture.configure(role); + + for (unsigned int starts = 0; starts < numRepeats; starts++) + capture.capture(numRequests); } -Results testSingleStream(std::shared_ptr<Camera> camera) +/* + * Test unbalanced stop + * + * Makes sure the camera supports a stop with requests queued. Example failure + * is a camera that does not handle cancelation of buffers coming back from the + * video device while stopping. + */ +TEST_P(FixedRequestsTest, Unbalanced) { - static const std::vector<std::pair<std::string, StreamRole>> roles = { - { "raw", Raw }, - { "still", StillCapture }, - { "video", VideoRecording }, - { "viewfinder", Viewfinder }, - }; - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; - - Results results(numRequests.size() * roles.size() * 3); - - for (const auto &role : roles) { - std::cout << "= Test role " << role.first << std::endl; - /* - * Test single capture cycles - * - * Makes sure the camera completes the exact number of requests queued. - * Example failure is a camera that needs N+M requests queued to - * complete N requests to the application. - */ - std::cout << "* Test single capture cycles" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestBalance(camera, role.second, 1, num)); - - /* - * Test multiple start/stop cycles - * - * Makes sure the camera supports multiple start/stop cycles. - * Example failure is a camera that does not clean up correctly in its - * error path but is only tested by single-capture applications. - */ - std::cout << "* Test multiple start/stop cycles" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestBalance(camera, role.second, 3, num)); - - /* - * Test unbalanced stop - * - * Makes sure the camera supports a stop with requests queued. - * Example failure is a camera that does not handle cancelation - * of buffers coming back from the video device while stopping. - */ - std::cout << "* Test unbalanced stop" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestUnbalance(camera, role.second, num)); - } - - return results; + auto [role, numRequests] = GetParam(); + + SimpleCaptureUnbalanced capture(cam); + + capture.configure(role); + + capture.capture(numRequests); } + +INSTANTIATE_TEST_SUITE_P(Raw, + FixedRequestsTest, + testing::Combine(testing::Values(Raw), + testing::ValuesIn(NUMREQUESTS))); + +INSTANTIATE_TEST_SUITE_P(StillCapture, + FixedRequestsTest, + testing::Combine(testing::Values(StillCapture), + testing::ValuesIn(NUMREQUESTS))); + +INSTANTIATE_TEST_SUITE_P(VideoRecording, + FixedRequestsTest, + testing::Combine(testing::Values(VideoRecording), + testing::ValuesIn(NUMREQUESTS))); + +INSTANTIATE_TEST_SUITE_P(Viewfinder, + FixedRequestsTest, + testing::Combine(testing::Values(Viewfinder), + testing::ValuesIn(NUMREQUESTS)));
Refactor lc-compliance using Googletest as the test framework. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- src/lc-compliance/main.cpp | 60 +++++------ src/lc-compliance/meson.build | 3 + src/lc-compliance/simple_capture.cpp | 73 +++++--------- src/lc-compliance/simple_capture.h | 8 +- src/lc-compliance/single_stream.cpp | 142 ++++++++++++++------------- 5 files changed, 132 insertions(+), 154 deletions(-)