[libcamera-devel] [PATCH v2 01/24] test: Extract CameraTest class out of camera tests to libtest
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 18 01:36:52 CET 2019
Hello,
On Fri, Nov 15, 2019 at 04:51:18PM +0100, Jacopo Mondi wrote:
> On Thu, Nov 14, 2019 at 09:07:52AM +0100, Niklas Söderlund wrote:
> > On 2019-11-08 22:53:46 +0200, Laurent Pinchart wrote:
> > > Many tests other than the camera/ tests use a camera. To increase code
> > > sharing, move the base CameraTest class to the test library. The class
> > > becomes a helper that doesn't inherit from Test anymore (to avoid
> > > diamond inheritance issues when more such helpers will exist).
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > I'm not overly found of how status_ is checked in users, but I like the
> > overall change more and this is test code.
>
> I agree this requires to be handled in the sub-classes' init() and
> it's not super nice
I don't like it much either, I think we'll need to refactor the tests in
the not too distant future, and this is something I believe we should
address then.
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >
> > > ---
> > > test/camera/buffer_import.cpp | 14 ++++-----
> > > test/camera/capture.cpp | 15 ++++++---
> > > test/camera/configuration_default.cpp | 16 ++++++++--
> > > test/camera/configuration_set.cpp | 15 ++++++---
> > > test/camera/meson.build | 2 +-
> > > test/camera/statemachine.cpp | 15 ++++++---
> > > test/controls/control_list.cpp | 39 +++++-------------------
> > > test/{camera => libtest}/camera_test.cpp | 24 +++++++++------
> > > test/{camera => libtest}/camera_test.h | 12 +++-----
> > > test/libtest/meson.build | 1 +
> > > 10 files changed, 77 insertions(+), 76 deletions(-)
> > > rename test/{camera => libtest}/camera_test.cpp (55%)
> > > rename test/{camera => libtest}/camera_test.h (77%)
> > >
> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > > index bbc5a25c4019..3efe02704c02 100644
> > > --- a/test/camera/buffer_import.cpp
> > > +++ b/test/camera/buffer_import.cpp
> > > @@ -18,6 +18,7 @@
> > > #include "v4l2_videodevice.h"
> > >
> > > #include "camera_test.h"
> > > +#include "test.h"
> > >
> > > using namespace libcamera;
> > >
> > > @@ -254,11 +255,11 @@ private:
> > > bool done_;
> > > };
> > >
> > > -class BufferImportTest : public CameraTest
> > > +class BufferImportTest : public CameraTest, public Test
> > > {
> > > public:
> > > BufferImportTest()
> > > - : CameraTest()
> > > + : CameraTest("VIMC Sensor B")
> > > {
> > > }
> > >
> > > @@ -350,11 +351,10 @@ protected:
> > >
> > > int init()
> > > {
> > > - int ret = CameraTest::init();
> > > - if (ret)
> > > - return ret;
> > > + if (status_ != TestPass)
> > > + return status_;
> > >
> > > - ret = sink_.init();
> > > + int ret = sink_.init();
> > > if (ret != TestPass) {
> > > cleanup();
> > > return ret;
> > > @@ -422,8 +422,6 @@ protected:
> > >
> > > camera_->stop();
> > > camera_->freeBuffers();
> > > -
> > > - CameraTest::cleanup();
> > > }
> > >
> > > private:
> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > > index 791ccad15f70..08cce9c7cbaf 100644
> > > --- a/test/camera/capture.cpp
> > > +++ b/test/camera/capture.cpp
> > > @@ -8,13 +8,20 @@
> > > #include <iostream>
> > >
> > > #include "camera_test.h"
> > > +#include "test.h"
> > >
> > > using namespace std;
> > >
> > > namespace {
> > >
> > > -class Capture : public CameraTest
> > > +class Capture : public CameraTest, public Test
> > > {
> > > +public:
> > > + Capture()
> > > + : CameraTest("VIMC Sensor B")
> > > + {
> > > + }
> > > +
> > > protected:
> > > unsigned int completeBuffersCount_;
> > > unsigned int completeRequestsCount_;
> > > @@ -46,14 +53,12 @@ protected:
> > >
> > > int init() override
> > > {
> > > - int ret = CameraTest::init();
> > > - if (ret)
> > > - return ret;
> > > + if (status_ != TestPass)
> > > + return status_;
> > >
> > > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > > if (!config_ || config_->size() != 1) {
> > > cout << "Failed to generate default configuration" << endl;
> > > - CameraTest::cleanup();
> > > return TestFail;
> > > }
> > >
> > > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> > > index ce2ec5d02e7b..31c908d2449e 100644
> > > --- a/test/camera/configuration_default.cpp
> > > +++ b/test/camera/configuration_default.cpp
> > > @@ -8,15 +8,27 @@
> > > #include <iostream>
> > >
> > > #include "camera_test.h"
> > > +#include "test.h"
> > >
> > > using namespace std;
> > >
> > > namespace {
> > >
> > > -class ConfigurationDefault : public CameraTest
> > > +class ConfigurationDefault : public CameraTest, public Test
> > > {
> > > +public:
> > > + ConfigurationDefault()
> > > + : CameraTest("VIMC Sensor B")
> > > + {
> > > + }
> > > +
> > > protected:
> > > - int run()
> > > + int init() override
> > > + {
> > > + return status_;
> > > + }
> > > +
> > > + int run() override
> > > {
> > > std::unique_ptr<CameraConfiguration> config;
> > >
> > > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > > index f88da96ca2b7..b4b5968115e8 100644
> > > --- a/test/camera/configuration_set.cpp
> > > +++ b/test/camera/configuration_set.cpp
> > > @@ -8,24 +8,29 @@
> > > #include <iostream>
> > >
> > > #include "camera_test.h"
> > > +#include "test.h"
> > >
> > > using namespace std;
> > >
> > > namespace {
> > >
> > > -class ConfigurationSet : public CameraTest
> > > +class ConfigurationSet : public CameraTest, public Test
> > > {
> > > +public:
> > > + ConfigurationSet()
> > > + : CameraTest("VIMC Sensor B")
> > > + {
> > > + }
> > > +
> > > protected:
> > > int init() override
> > > {
> > > - int ret = CameraTest::init();
> > > - if (ret)
> > > - return ret;
> > > + if (status_ != TestPass)
> > > + return status_;
> > >
> > > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > > if (!config_ || config_->size() != 1) {
> > > cout << "Failed to generate default configuration" << endl;
> > > - CameraTest::cleanup();
> > > return TestFail;
> > > }
> > >
> > > diff --git a/test/camera/meson.build b/test/camera/meson.build
> > > index d6fd66b8f89e..e2a6660a7a92 100644
> > > --- a/test/camera/meson.build
> > > +++ b/test/camera/meson.build
> > > @@ -9,7 +9,7 @@ camera_tests = [
> > > ]
> > >
> > > foreach t : camera_tests
> > > - exe = executable(t[0], [t[1], 'camera_test.cpp'],
> > > + exe = executable(t[0], t[1],
> > > dependencies : libcamera_dep,
> > > link_with : test_libraries,
> > > include_directories : test_includes_internal)
> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > > index 12d5e0e1d534..afa13ba77b0b 100644
> > > --- a/test/camera/statemachine.cpp
> > > +++ b/test/camera/statemachine.cpp
> > > @@ -8,13 +8,20 @@
> > > #include <iostream>
> > >
> > > #include "camera_test.h"
> > > +#include "test.h"
> > >
> > > using namespace std;
> > >
> > > namespace {
> > >
> > > -class Statemachine : public CameraTest
> > > +class Statemachine : public CameraTest, public Test
> > > {
> > > +public:
> > > + Statemachine()
> > > + : CameraTest("VIMC Sensor B")
> > > + {
> > > + }
> > > +
> > > protected:
> > > int testAvailable()
> > > {
> > > @@ -233,14 +240,12 @@ protected:
> > >
> > > int init() override
> > > {
> > > - int ret = CameraTest::init();
> > > - if (ret)
> > > - return ret;
> > > + if (status_ != TestPass)
> > > + return status_;
> > >
> > > defconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > > if (!defconf_) {
> > > cout << "Failed to generate default configuration" << endl;
> > > - CameraTest::cleanup();
> > > return TestFail;
> > > }
> > >
> > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
> > > index 5af53f64bb6c..4d212abd09e6 100644
> > > --- a/test/controls/control_list.cpp
> > > +++ b/test/controls/control_list.cpp
> > > @@ -13,32 +13,22 @@
> > > #include <libcamera/controls.h>
> > >
> > > #include "camera_controls.h"
> > > +
> > > +#include "camera_test.h"
> > > #include "test.h"
> > >
> > > using namespace std;
> > > using namespace libcamera;
> > >
> > > -class ControlListTest : public Test
> > > +class ControlListTest : public CameraTest, public Test
> > > {
> > > -protected:
> > > - int init()
> > > +public:
> > > + ControlListTest()
> > > + : CameraTest("VIMC Sensor B")
> > > {
> > > - cm_ = new CameraManager();
> > > -
> > > - if (cm_->start()) {
> > > - cout << "Failed to start camera manager" << endl;
> > > - return TestFail;
> > > - }
> > > -
> > > - camera_ = cm_->get("VIMC Sensor B");
> > > - if (!camera_) {
> > > - cout << "Can not find VIMC camera" << endl;
> > > - return TestSkip;
> > > - }
> > > -
> > > - return TestPass;
> > > }
> > >
> > > +protected:
>
> Don't we need to check status_ in init() ?
>
> Apart from this:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
> j
>
> > > int run()
> > > {
> > > CameraControlValidator validator(camera_.get());
> > > @@ -156,21 +146,6 @@ protected:
> > >
> > > return TestPass;
> > > }
> > > -
> > > - void cleanup()
> > > - {
> > > - if (camera_) {
> > > - camera_->release();
> > > - camera_.reset();
> > > - }
> > > -
> > > - cm_->stop();
> > > - delete cm_;
> > > - }
> > > -
> > > -private:
> > > - CameraManager *cm_;
> > > - std::shared_ptr<Camera> camera_;
> > > };
> > >
> > > TEST_REGISTER(ControlListTest)
> > > diff --git a/test/camera/camera_test.cpp b/test/libtest/camera_test.cpp
> > > similarity index 55%
> > > rename from test/camera/camera_test.cpp
> > > rename to test/libtest/camera_test.cpp
> > > index 101e31fbce79..2ae4d6776f2e 100644
> > > --- a/test/camera/camera_test.cpp
> > > +++ b/test/libtest/camera_test.cpp
> > > @@ -8,35 +8,39 @@
> > > #include <iostream>
> > >
> > > #include "camera_test.h"
> > > +#include "test.h"
> > >
> > > using namespace libcamera;
> > > using namespace std;
> > >
> > > -int CameraTest::init()
> > > +CameraTest::CameraTest(const char *name)
> > > {
> > > cm_ = new CameraManager();
> > >
> > > if (cm_->start()) {
> > > - cout << "Failed to start camera manager" << endl;
> > > - return TestFail;
> > > + cerr << "Failed to start camera manager" << endl;
> > > + status_ = TestFail;
> > > + return;
> > > }
> > >
> > > - camera_ = cm_->get("VIMC Sensor B");
> > > + camera_ = cm_->get(name);
> > > if (!camera_) {
> > > - cout << "Can not find VIMC camera" << endl;
> > > - return TestSkip;
> > > + cerr << "Can not find '" << name << "' camera" << endl;
> > > + status_ = TestSkip;
> > > + return;
> > > }
> > >
> > > /* Sanity check that the camera has streams. */
> > > if (camera_->streams().empty()) {
> > > - cout << "Camera has no stream" << endl;
> > > - return TestFail;
> > > + cerr << "Camera has no stream" << endl;
> > > + status_ = TestFail;
> > > + return;
> > > }
> > >
> > > - return TestPass;
> > > + status_ = TestPass;
> > > }
> > >
> > > -void CameraTest::cleanup()
> > > +CameraTest::~CameraTest()
> > > {
> > > if (camera_) {
> > > camera_->release();
> > > diff --git a/test/camera/camera_test.h b/test/libtest/camera_test.h
> > > similarity index 77%
> > > rename from test/camera/camera_test.h
> > > rename to test/libtest/camera_test.h
> > > index e57b05eb28a9..0b6bad05e37c 100644
> > > --- a/test/camera/camera_test.h
> > > +++ b/test/libtest/camera_test.h
> > > @@ -9,22 +9,18 @@
> > >
> > > #include <libcamera/libcamera.h>
> > >
> > > -#include "test.h"
> > > -
> > > using namespace libcamera;
> > >
> > > -class CameraTest : public Test
> > > +class CameraTest
> > > {
> > > public:
> > > - CameraTest()
> > > - : cm_(nullptr) {}
> > > + CameraTest(const char *name);
> > > + ~CameraTest();
> > >
> > > protected:
> > > - int init();
> > > - void cleanup();
> > > -
> > > CameraManager *cm_;
> > > std::shared_ptr<Camera> camera_;
> > > + int status_;
> > > };
> > >
> > > #endif /* __LIBCAMERA_CAMERA_TEST_H__ */
> > > diff --git a/test/libtest/meson.build b/test/libtest/meson.build
> > > index ca762b4421c2..3e798ef3810e 100644
> > > --- a/test/libtest/meson.build
> > > +++ b/test/libtest/meson.build
> > > @@ -1,4 +1,5 @@
> > > libtest_sources = files([
> > > + 'camera_test.cpp',
> > > 'test.cpp',
> > > ])
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list