[{"id":3014,"web_url":"https://patchwork.libcamera.org/comment/3014/","msgid":"<20191114080752.GF7353@bigcity.dyn.berto.se>","date":"2019-11-14T08:07:52","subject":"Re: [libcamera-devel] [PATCH v2 01/24] test: Extract CameraTest\n\tclass out of camera tests to libtest","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-11-08 22:53:46 +0200, Laurent Pinchart wrote:\n> Many tests other than the camera/ tests use a camera. To increase code\n> sharing, move the base CameraTest class to the test library. The class\n> becomes a helper that doesn't inherit from Test anymore (to avoid\n> diamond inheritance issues when more such helpers will exist).\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nI'm not overly found of how status_ is checked in users, but I like the \noverall change more and this is test code.\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  test/camera/buffer_import.cpp            | 14 ++++-----\n>  test/camera/capture.cpp                  | 15 ++++++---\n>  test/camera/configuration_default.cpp    | 16 ++++++++--\n>  test/camera/configuration_set.cpp        | 15 ++++++---\n>  test/camera/meson.build                  |  2 +-\n>  test/camera/statemachine.cpp             | 15 ++++++---\n>  test/controls/control_list.cpp           | 39 +++++-------------------\n>  test/{camera => libtest}/camera_test.cpp | 24 +++++++++------\n>  test/{camera => libtest}/camera_test.h   | 12 +++-----\n>  test/libtest/meson.build                 |  1 +\n>  10 files changed, 77 insertions(+), 76 deletions(-)\n>  rename test/{camera => libtest}/camera_test.cpp (55%)\n>  rename test/{camera => libtest}/camera_test.h (77%)\n> \n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index bbc5a25c4019..3efe02704c02 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -18,6 +18,7 @@\n>  #include \"v4l2_videodevice.h\"\n>  \n>  #include \"camera_test.h\"\n> +#include \"test.h\"\n>  \n>  using namespace libcamera;\n>  \n> @@ -254,11 +255,11 @@ private:\n>  \tbool done_;\n>  };\n>  \n> -class BufferImportTest : public CameraTest\n> +class BufferImportTest : public CameraTest, public Test\n>  {\n>  public:\n>  \tBufferImportTest()\n> -\t\t: CameraTest()\n> +\t\t: CameraTest(\"VIMC Sensor B\")\n>  \t{\n>  \t}\n>  \n> @@ -350,11 +351,10 @@ protected:\n>  \n>  \tint init()\n>  \t{\n> -\t\tint ret = CameraTest::init();\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n>  \n> -\t\tret = sink_.init();\n> +\t\tint ret = sink_.init();\n>  \t\tif (ret != TestPass) {\n>  \t\t\tcleanup();\n>  \t\t\treturn ret;\n> @@ -422,8 +422,6 @@ protected:\n>  \n>  \t\tcamera_->stop();\n>  \t\tcamera_->freeBuffers();\n> -\n> -\t\tCameraTest::cleanup();\n>  \t}\n>  \n>  private:\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index 791ccad15f70..08cce9c7cbaf 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -8,13 +8,20 @@\n>  #include <iostream>\n>  \n>  #include \"camera_test.h\"\n> +#include \"test.h\"\n>  \n>  using namespace std;\n>  \n>  namespace {\n>  \n> -class Capture : public CameraTest\n> +class Capture : public CameraTest, public Test\n>  {\n> +public:\n> +\tCapture()\n> +\t\t: CameraTest(\"VIMC Sensor B\")\n> +\t{\n> +\t}\n> +\n>  protected:\n>  \tunsigned int completeBuffersCount_;\n>  \tunsigned int completeRequestsCount_;\n> @@ -46,14 +53,12 @@ protected:\n>  \n>  \tint init() override\n>  \t{\n> -\t\tint ret = CameraTest::init();\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n>  \n>  \t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n>  \t\tif (!config_ || config_->size() != 1) {\n>  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> -\t\t\tCameraTest::cleanup();\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp\n> index ce2ec5d02e7b..31c908d2449e 100644\n> --- a/test/camera/configuration_default.cpp\n> +++ b/test/camera/configuration_default.cpp\n> @@ -8,15 +8,27 @@\n>  #include <iostream>\n>  \n>  #include \"camera_test.h\"\n> +#include \"test.h\"\n>  \n>  using namespace std;\n>  \n>  namespace {\n>  \n> -class ConfigurationDefault : public CameraTest\n> +class ConfigurationDefault : public CameraTest, public Test\n>  {\n> +public:\n> +\tConfigurationDefault()\n> +\t\t: CameraTest(\"VIMC Sensor B\")\n> +\t{\n> +\t}\n> +\n>  protected:\n> -\tint run()\n> +\tint init() override\n> +\t{\n> +\t\treturn status_;\n> +\t}\n> +\n> +\tint run() override\n>  \t{\n>  \t\tstd::unique_ptr<CameraConfiguration> config;\n>  \n> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> index f88da96ca2b7..b4b5968115e8 100644\n> --- a/test/camera/configuration_set.cpp\n> +++ b/test/camera/configuration_set.cpp\n> @@ -8,24 +8,29 @@\n>  #include <iostream>\n>  \n>  #include \"camera_test.h\"\n> +#include \"test.h\"\n>  \n>  using namespace std;\n>  \n>  namespace {\n>  \n> -class ConfigurationSet : public CameraTest\n> +class ConfigurationSet : public CameraTest, public Test\n>  {\n> +public:\n> +\tConfigurationSet()\n> +\t\t: CameraTest(\"VIMC Sensor B\")\n> +\t{\n> +\t}\n> +\n>  protected:\n>  \tint init() override\n>  \t{\n> -\t\tint ret = CameraTest::init();\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n>  \n>  \t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n>  \t\tif (!config_ || config_->size() != 1) {\n>  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> -\t\t\tCameraTest::cleanup();\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> diff --git a/test/camera/meson.build b/test/camera/meson.build\n> index d6fd66b8f89e..e2a6660a7a92 100644\n> --- a/test/camera/meson.build\n> +++ b/test/camera/meson.build\n> @@ -9,7 +9,7 @@ camera_tests = [\n>  ]\n>  \n>  foreach t : camera_tests\n> -    exe = executable(t[0], [t[1], 'camera_test.cpp'],\n> +    exe = executable(t[0], t[1],\n>                       dependencies : libcamera_dep,\n>                       link_with : test_libraries,\n>                       include_directories : test_includes_internal)\n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index 12d5e0e1d534..afa13ba77b0b 100644\n> --- a/test/camera/statemachine.cpp\n> +++ b/test/camera/statemachine.cpp\n> @@ -8,13 +8,20 @@\n>  #include <iostream>\n>  \n>  #include \"camera_test.h\"\n> +#include \"test.h\"\n>  \n>  using namespace std;\n>  \n>  namespace {\n>  \n> -class Statemachine : public CameraTest\n> +class Statemachine : public CameraTest, public Test\n>  {\n> +public:\n> +\tStatemachine()\n> +\t\t: CameraTest(\"VIMC Sensor B\")\n> +\t{\n> +\t}\n> +\n>  protected:\n>  \tint testAvailable()\n>  \t{\n> @@ -233,14 +240,12 @@ protected:\n>  \n>  \tint init() override\n>  \t{\n> -\t\tint ret = CameraTest::init();\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n>  \n>  \t\tdefconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n>  \t\tif (!defconf_) {\n>  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> -\t\t\tCameraTest::cleanup();\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index 5af53f64bb6c..4d212abd09e6 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -13,32 +13,22 @@\n>  #include <libcamera/controls.h>\n>  \n>  #include \"camera_controls.h\"\n> +\n> +#include \"camera_test.h\"\n>  #include \"test.h\"\n>  \n>  using namespace std;\n>  using namespace libcamera;\n>  \n> -class ControlListTest : public Test\n> +class ControlListTest : public CameraTest, public Test\n>  {\n> -protected:\n> -\tint init()\n> +public:\n> +\tControlListTest()\n> +\t\t: CameraTest(\"VIMC Sensor B\")\n>  \t{\n> -\t\tcm_ = new CameraManager();\n> -\n> -\t\tif (cm_->start()) {\n> -\t\t\tcout << \"Failed to start camera manager\" << endl;\n> -\t\t\treturn TestFail;\n> -\t\t}\n> -\n> -\t\tcamera_ = cm_->get(\"VIMC Sensor B\");\n> -\t\tif (!camera_) {\n> -\t\t\tcout << \"Can not find VIMC camera\" << endl;\n> -\t\t\treturn TestSkip;\n> -\t\t}\n> -\n> -\t\treturn TestPass;\n>  \t}\n>  \n> +protected:\n>  \tint run()\n>  \t{\n>  \t\tCameraControlValidator validator(camera_.get());\n> @@ -156,21 +146,6 @@ protected:\n>  \n>  \t\treturn TestPass;\n>  \t}\n> -\n> -\tvoid cleanup()\n> -\t{\n> -\t\tif (camera_) {\n> -\t\t\tcamera_->release();\n> -\t\t\tcamera_.reset();\n> -\t\t}\n> -\n> -\t\tcm_->stop();\n> -\t\tdelete cm_;\n> -\t}\n> -\n> -private:\n> -\tCameraManager *cm_;\n> -\tstd::shared_ptr<Camera> camera_;\n>  };\n>  \n>  TEST_REGISTER(ControlListTest)\n> diff --git a/test/camera/camera_test.cpp b/test/libtest/camera_test.cpp\n> similarity index 55%\n> rename from test/camera/camera_test.cpp\n> rename to test/libtest/camera_test.cpp\n> index 101e31fbce79..2ae4d6776f2e 100644\n> --- a/test/camera/camera_test.cpp\n> +++ b/test/libtest/camera_test.cpp\n> @@ -8,35 +8,39 @@\n>  #include <iostream>\n>  \n>  #include \"camera_test.h\"\n> +#include \"test.h\"\n>  \n>  using namespace libcamera;\n>  using namespace std;\n>  \n> -int CameraTest::init()\n> +CameraTest::CameraTest(const char *name)\n>  {\n>  \tcm_ = new CameraManager();\n>  \n>  \tif (cm_->start()) {\n> -\t\tcout << \"Failed to start camera manager\" << endl;\n> -\t\treturn TestFail;\n> +\t\tcerr << \"Failed to start camera manager\" << endl;\n> +\t\tstatus_ = TestFail;\n> +\t\treturn;\n>  \t}\n>  \n> -\tcamera_ = cm_->get(\"VIMC Sensor B\");\n> +\tcamera_ = cm_->get(name);\n>  \tif (!camera_) {\n> -\t\tcout << \"Can not find VIMC camera\" << endl;\n> -\t\treturn TestSkip;\n> +\t\tcerr << \"Can not find '\" << name << \"' camera\" << endl;\n> +\t\tstatus_ = TestSkip;\n> +\t\treturn;\n>  \t}\n>  \n>  \t/* Sanity check that the camera has streams. */\n>  \tif (camera_->streams().empty()) {\n> -\t\tcout << \"Camera has no stream\" << endl;\n> -\t\treturn TestFail;\n> +\t\tcerr << \"Camera has no stream\" << endl;\n> +\t\tstatus_ = TestFail;\n> +\t\treturn;\n>  \t}\n>  \n> -\treturn TestPass;\n> +\tstatus_ = TestPass;\n>  }\n>  \n> -void CameraTest::cleanup()\n> +CameraTest::~CameraTest()\n>  {\n>  \tif (camera_) {\n>  \t\tcamera_->release();\n> diff --git a/test/camera/camera_test.h b/test/libtest/camera_test.h\n> similarity index 77%\n> rename from test/camera/camera_test.h\n> rename to test/libtest/camera_test.h\n> index e57b05eb28a9..0b6bad05e37c 100644\n> --- a/test/camera/camera_test.h\n> +++ b/test/libtest/camera_test.h\n> @@ -9,22 +9,18 @@\n>  \n>  #include <libcamera/libcamera.h>\n>  \n> -#include \"test.h\"\n> -\n>  using namespace libcamera;\n>  \n> -class CameraTest : public Test\n> +class CameraTest\n>  {\n>  public:\n> -\tCameraTest()\n> -\t\t: cm_(nullptr) {}\n> +\tCameraTest(const char *name);\n> +\t~CameraTest();\n>  \n>  protected:\n> -\tint init();\n> -\tvoid cleanup();\n> -\n>  \tCameraManager *cm_;\n>  \tstd::shared_ptr<Camera> camera_;\n> +\tint status_;\n>  };\n>  \n>  #endif /* __LIBCAMERA_CAMERA_TEST_H__ */\n> diff --git a/test/libtest/meson.build b/test/libtest/meson.build\n> index ca762b4421c2..3e798ef3810e 100644\n> --- a/test/libtest/meson.build\n> +++ b/test/libtest/meson.build\n> @@ -1,4 +1,5 @@\n>  libtest_sources = files([\n> +    'camera_test.cpp',\n>      'test.cpp',\n>  ])\n>  \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26E2A6136F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 14 Nov 2019 09:07:54 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id 139so5635973ljf.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 14 Nov 2019 00:07:54 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tj23sm2050040lji.41.2019.11.14.00.07.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 14 Nov 2019 00:07:52 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=93vZ5e1pCs/g2NsE0WiMfRTwFjvNoqto2m7RjsN02kI=;\n\tb=wfEVEKJuhTmrZ3LYDAMa8OtNnHEja/RU/LJMcnNBaYMfANkEpR2qq9pUCYikzwjNsB\n\tyh6AXljhOsiweQxHAvx8kffjVAwluIsrIsrP1pa1rsc/N4lBbs2pAOwLYyDS0qJkWKo6\n\tIPkafx0ivmII0v02iAL/S5A7LgDWtIV7z6duZmW0kd6d14G7wjN2hRY3gLs4TXr0VfBF\n\tptcSf8F7AUGSpcsR3+7reEtZICCY3qCHTIXnVNvvk5luWpHHtFK7ul0pkQjt3ZiEm8rP\n\tcpHiuS/c/wpH6l3LOf+1/f0HCvrNOfffUTCmPpr4RlNJ7yp1w08pR8UxyiDsvq7Cn++D\n\tJbZw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=93vZ5e1pCs/g2NsE0WiMfRTwFjvNoqto2m7RjsN02kI=;\n\tb=uS12m4PQPl7a4pulWIaQSfPGDtH+vHCBX52TPKBJKZoBZfrfTrqmRXVB/GVO38MANI\n\tVyDPdB4MHnYZ+uXkDOrIAFRvqQBqgBf248catMVTSIXtV7qD7QP1YI/AIs/VXB3An095\n\t8ONIVlvgwTV2asrFKZcX/ikB+JwqlFwHS0MTufGCjTYd0gwFBGy1XDlgfuforOCQlV6W\n\tb9iDYpRqOTTeqSQvdBo9Wqx0ohwul8U5YWLXgf0WfMPF3AjMEg9zEnD3rtrIaDiK2Dhz\n\tFgP2kgwhx93F3yXDqHHKlFE5lsd6HknBTY6vzq+Y1di2cEFINESMj3dDruI07KpUK7uS\n\tGFGg==","X-Gm-Message-State":"APjAAAW2gRcjaw7oC837+WWg+ZiA+jVhNCcFnPeHwGyXe13sb120Wnfb\n\tdRu8iCiZdV6dakNmu9ym6T/kS4XIN4E=","X-Google-Smtp-Source":"APXvYqzlvvGcN9Lm1snIAsOkx7m4XHr3xdTyPLf96PTs3B4Y43M32ORe+L5LUoD02c4AoNWAUGGaRg==","X-Received":"by 2002:a2e:898a:: with SMTP id\n\tc10mr5524187lji.177.1573718873358; \n\tThu, 14 Nov 2019 00:07:53 -0800 (PST)","Date":"Thu, 14 Nov 2019 09:07:52 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191114080752.GF7353@bigcity.dyn.berto.se>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191108205409.18845-2-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 01/24] test: Extract CameraTest\n\tclass out of camera tests to libtest","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 14 Nov 2019 08:07:54 -0000"}},{"id":3021,"web_url":"https://patchwork.libcamera.org/comment/3021/","msgid":"<20191115155118.wo3gniummxwqlkyy@uno.localdomain>","date":"2019-11-15T15:51:18","subject":"Re: [libcamera-devel] [PATCH v2 01/24] test: Extract CameraTest\n\tclass out of camera tests to libtest","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"HI Laurent,\n\nOn Thu, Nov 14, 2019 at 09:07:52AM +0100, Niklas Söderlund wrote:\n> Hi Laurent,\n>\n> Thanks for your work.\n>\n> On 2019-11-08 22:53:46 +0200, Laurent Pinchart wrote:\n> > Many tests other than the camera/ tests use a camera. To increase code\n> > sharing, move the base CameraTest class to the test library. The class\n> > becomes a helper that doesn't inherit from Test anymore (to avoid\n> > diamond inheritance issues when more such helpers will exist).\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>\n> I'm not overly found of how status_ is checked in users, but I like the\n> overall change more and this is test code.\n\nI agree this requires to be handled in the sub-classes' init() and\nit's not super nice\n\n>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n>\n> > ---\n> >  test/camera/buffer_import.cpp            | 14 ++++-----\n> >  test/camera/capture.cpp                  | 15 ++++++---\n> >  test/camera/configuration_default.cpp    | 16 ++++++++--\n> >  test/camera/configuration_set.cpp        | 15 ++++++---\n> >  test/camera/meson.build                  |  2 +-\n> >  test/camera/statemachine.cpp             | 15 ++++++---\n> >  test/controls/control_list.cpp           | 39 +++++-------------------\n> >  test/{camera => libtest}/camera_test.cpp | 24 +++++++++------\n> >  test/{camera => libtest}/camera_test.h   | 12 +++-----\n> >  test/libtest/meson.build                 |  1 +\n> >  10 files changed, 77 insertions(+), 76 deletions(-)\n> >  rename test/{camera => libtest}/camera_test.cpp (55%)\n> >  rename test/{camera => libtest}/camera_test.h (77%)\n> >\n> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > index bbc5a25c4019..3efe02704c02 100644\n> > --- a/test/camera/buffer_import.cpp\n> > +++ b/test/camera/buffer_import.cpp\n> > @@ -18,6 +18,7 @@\n> >  #include \"v4l2_videodevice.h\"\n> >\n> >  #include \"camera_test.h\"\n> > +#include \"test.h\"\n> >\n> >  using namespace libcamera;\n> >\n> > @@ -254,11 +255,11 @@ private:\n> >  \tbool done_;\n> >  };\n> >\n> > -class BufferImportTest : public CameraTest\n> > +class BufferImportTest : public CameraTest, public Test\n> >  {\n> >  public:\n> >  \tBufferImportTest()\n> > -\t\t: CameraTest()\n> > +\t\t: CameraTest(\"VIMC Sensor B\")\n> >  \t{\n> >  \t}\n> >\n> > @@ -350,11 +351,10 @@ protected:\n> >\n> >  \tint init()\n> >  \t{\n> > -\t\tint ret = CameraTest::init();\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> > +\t\tif (status_ != TestPass)\n> > +\t\t\treturn status_;\n> >\n> > -\t\tret = sink_.init();\n> > +\t\tint ret = sink_.init();\n> >  \t\tif (ret != TestPass) {\n> >  \t\t\tcleanup();\n> >  \t\t\treturn ret;\n> > @@ -422,8 +422,6 @@ protected:\n> >\n> >  \t\tcamera_->stop();\n> >  \t\tcamera_->freeBuffers();\n> > -\n> > -\t\tCameraTest::cleanup();\n> >  \t}\n> >\n> >  private:\n> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > index 791ccad15f70..08cce9c7cbaf 100644\n> > --- a/test/camera/capture.cpp\n> > +++ b/test/camera/capture.cpp\n> > @@ -8,13 +8,20 @@\n> >  #include <iostream>\n> >\n> >  #include \"camera_test.h\"\n> > +#include \"test.h\"\n> >\n> >  using namespace std;\n> >\n> >  namespace {\n> >\n> > -class Capture : public CameraTest\n> > +class Capture : public CameraTest, public Test\n> >  {\n> > +public:\n> > +\tCapture()\n> > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > +\t{\n> > +\t}\n> > +\n> >  protected:\n> >  \tunsigned int completeBuffersCount_;\n> >  \tunsigned int completeRequestsCount_;\n> > @@ -46,14 +53,12 @@ protected:\n> >\n> >  \tint init() override\n> >  \t{\n> > -\t\tint ret = CameraTest::init();\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> > +\t\tif (status_ != TestPass)\n> > +\t\t\treturn status_;\n> >\n> >  \t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> >  \t\tif (!config_ || config_->size() != 1) {\n> >  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > -\t\t\tCameraTest::cleanup();\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >\n> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp\n> > index ce2ec5d02e7b..31c908d2449e 100644\n> > --- a/test/camera/configuration_default.cpp\n> > +++ b/test/camera/configuration_default.cpp\n> > @@ -8,15 +8,27 @@\n> >  #include <iostream>\n> >\n> >  #include \"camera_test.h\"\n> > +#include \"test.h\"\n> >\n> >  using namespace std;\n> >\n> >  namespace {\n> >\n> > -class ConfigurationDefault : public CameraTest\n> > +class ConfigurationDefault : public CameraTest, public Test\n> >  {\n> > +public:\n> > +\tConfigurationDefault()\n> > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > +\t{\n> > +\t}\n> > +\n> >  protected:\n> > -\tint run()\n> > +\tint init() override\n> > +\t{\n> > +\t\treturn status_;\n> > +\t}\n> > +\n> > +\tint run() override\n> >  \t{\n> >  \t\tstd::unique_ptr<CameraConfiguration> config;\n> >\n> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> > index f88da96ca2b7..b4b5968115e8 100644\n> > --- a/test/camera/configuration_set.cpp\n> > +++ b/test/camera/configuration_set.cpp\n> > @@ -8,24 +8,29 @@\n> >  #include <iostream>\n> >\n> >  #include \"camera_test.h\"\n> > +#include \"test.h\"\n> >\n> >  using namespace std;\n> >\n> >  namespace {\n> >\n> > -class ConfigurationSet : public CameraTest\n> > +class ConfigurationSet : public CameraTest, public Test\n> >  {\n> > +public:\n> > +\tConfigurationSet()\n> > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > +\t{\n> > +\t}\n> > +\n> >  protected:\n> >  \tint init() override\n> >  \t{\n> > -\t\tint ret = CameraTest::init();\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> > +\t\tif (status_ != TestPass)\n> > +\t\t\treturn status_;\n> >\n> >  \t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> >  \t\tif (!config_ || config_->size() != 1) {\n> >  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > -\t\t\tCameraTest::cleanup();\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >\n> > diff --git a/test/camera/meson.build b/test/camera/meson.build\n> > index d6fd66b8f89e..e2a6660a7a92 100644\n> > --- a/test/camera/meson.build\n> > +++ b/test/camera/meson.build\n> > @@ -9,7 +9,7 @@ camera_tests = [\n> >  ]\n> >\n> >  foreach t : camera_tests\n> > -    exe = executable(t[0], [t[1], 'camera_test.cpp'],\n> > +    exe = executable(t[0], t[1],\n> >                       dependencies : libcamera_dep,\n> >                       link_with : test_libraries,\n> >                       include_directories : test_includes_internal)\n> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > index 12d5e0e1d534..afa13ba77b0b 100644\n> > --- a/test/camera/statemachine.cpp\n> > +++ b/test/camera/statemachine.cpp\n> > @@ -8,13 +8,20 @@\n> >  #include <iostream>\n> >\n> >  #include \"camera_test.h\"\n> > +#include \"test.h\"\n> >\n> >  using namespace std;\n> >\n> >  namespace {\n> >\n> > -class Statemachine : public CameraTest\n> > +class Statemachine : public CameraTest, public Test\n> >  {\n> > +public:\n> > +\tStatemachine()\n> > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > +\t{\n> > +\t}\n> > +\n> >  protected:\n> >  \tint testAvailable()\n> >  \t{\n> > @@ -233,14 +240,12 @@ protected:\n> >\n> >  \tint init() override\n> >  \t{\n> > -\t\tint ret = CameraTest::init();\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> > +\t\tif (status_ != TestPass)\n> > +\t\t\treturn status_;\n> >\n> >  \t\tdefconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> >  \t\tif (!defconf_) {\n> >  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > -\t\t\tCameraTest::cleanup();\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >\n> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > index 5af53f64bb6c..4d212abd09e6 100644\n> > --- a/test/controls/control_list.cpp\n> > +++ b/test/controls/control_list.cpp\n> > @@ -13,32 +13,22 @@\n> >  #include <libcamera/controls.h>\n> >\n> >  #include \"camera_controls.h\"\n> > +\n> > +#include \"camera_test.h\"\n> >  #include \"test.h\"\n> >\n> >  using namespace std;\n> >  using namespace libcamera;\n> >\n> > -class ControlListTest : public Test\n> > +class ControlListTest : public CameraTest, public Test\n> >  {\n> > -protected:\n> > -\tint init()\n> > +public:\n> > +\tControlListTest()\n> > +\t\t: CameraTest(\"VIMC Sensor B\")\n> >  \t{\n> > -\t\tcm_ = new CameraManager();\n> > -\n> > -\t\tif (cm_->start()) {\n> > -\t\t\tcout << \"Failed to start camera manager\" << endl;\n> > -\t\t\treturn TestFail;\n> > -\t\t}\n> > -\n> > -\t\tcamera_ = cm_->get(\"VIMC Sensor B\");\n> > -\t\tif (!camera_) {\n> > -\t\t\tcout << \"Can not find VIMC camera\" << endl;\n> > -\t\t\treturn TestSkip;\n> > -\t\t}\n> > -\n> > -\t\treturn TestPass;\n> >  \t}\n> >\n> > +protected:\n\nDon't we need to check status_ in init() ?\n\nApart from this:\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n   j\n\n> >  \tint run()\n> >  \t{\n> >  \t\tCameraControlValidator validator(camera_.get());\n> > @@ -156,21 +146,6 @@ protected:\n> >\n> >  \t\treturn TestPass;\n> >  \t}\n> > -\n> > -\tvoid cleanup()\n> > -\t{\n> > -\t\tif (camera_) {\n> > -\t\t\tcamera_->release();\n> > -\t\t\tcamera_.reset();\n> > -\t\t}\n> > -\n> > -\t\tcm_->stop();\n> > -\t\tdelete cm_;\n> > -\t}\n> > -\n> > -private:\n> > -\tCameraManager *cm_;\n> > -\tstd::shared_ptr<Camera> camera_;\n> >  };\n> >\n> >  TEST_REGISTER(ControlListTest)\n> > diff --git a/test/camera/camera_test.cpp b/test/libtest/camera_test.cpp\n> > similarity index 55%\n> > rename from test/camera/camera_test.cpp\n> > rename to test/libtest/camera_test.cpp\n> > index 101e31fbce79..2ae4d6776f2e 100644\n> > --- a/test/camera/camera_test.cpp\n> > +++ b/test/libtest/camera_test.cpp\n> > @@ -8,35 +8,39 @@\n> >  #include <iostream>\n> >\n> >  #include \"camera_test.h\"\n> > +#include \"test.h\"\n> >\n> >  using namespace libcamera;\n> >  using namespace std;\n> >\n> > -int CameraTest::init()\n> > +CameraTest::CameraTest(const char *name)\n> >  {\n> >  \tcm_ = new CameraManager();\n> >\n> >  \tif (cm_->start()) {\n> > -\t\tcout << \"Failed to start camera manager\" << endl;\n> > -\t\treturn TestFail;\n> > +\t\tcerr << \"Failed to start camera manager\" << endl;\n> > +\t\tstatus_ = TestFail;\n> > +\t\treturn;\n> >  \t}\n> >\n> > -\tcamera_ = cm_->get(\"VIMC Sensor B\");\n> > +\tcamera_ = cm_->get(name);\n> >  \tif (!camera_) {\n> > -\t\tcout << \"Can not find VIMC camera\" << endl;\n> > -\t\treturn TestSkip;\n> > +\t\tcerr << \"Can not find '\" << name << \"' camera\" << endl;\n> > +\t\tstatus_ = TestSkip;\n> > +\t\treturn;\n> >  \t}\n> >\n> >  \t/* Sanity check that the camera has streams. */\n> >  \tif (camera_->streams().empty()) {\n> > -\t\tcout << \"Camera has no stream\" << endl;\n> > -\t\treturn TestFail;\n> > +\t\tcerr << \"Camera has no stream\" << endl;\n> > +\t\tstatus_ = TestFail;\n> > +\t\treturn;\n> >  \t}\n> >\n> > -\treturn TestPass;\n> > +\tstatus_ = TestPass;\n> >  }\n> >\n> > -void CameraTest::cleanup()\n> > +CameraTest::~CameraTest()\n> >  {\n> >  \tif (camera_) {\n> >  \t\tcamera_->release();\n> > diff --git a/test/camera/camera_test.h b/test/libtest/camera_test.h\n> > similarity index 77%\n> > rename from test/camera/camera_test.h\n> > rename to test/libtest/camera_test.h\n> > index e57b05eb28a9..0b6bad05e37c 100644\n> > --- a/test/camera/camera_test.h\n> > +++ b/test/libtest/camera_test.h\n> > @@ -9,22 +9,18 @@\n> >\n> >  #include <libcamera/libcamera.h>\n> >\n> > -#include \"test.h\"\n> > -\n> >  using namespace libcamera;\n> >\n> > -class CameraTest : public Test\n> > +class CameraTest\n> >  {\n> >  public:\n> > -\tCameraTest()\n> > -\t\t: cm_(nullptr) {}\n> > +\tCameraTest(const char *name);\n> > +\t~CameraTest();\n> >\n> >  protected:\n> > -\tint init();\n> > -\tvoid cleanup();\n> > -\n> >  \tCameraManager *cm_;\n> >  \tstd::shared_ptr<Camera> camera_;\n> > +\tint status_;\n> >  };\n> >\n> >  #endif /* __LIBCAMERA_CAMERA_TEST_H__ */\n> > diff --git a/test/libtest/meson.build b/test/libtest/meson.build\n> > index ca762b4421c2..3e798ef3810e 100644\n> > --- a/test/libtest/meson.build\n> > +++ b/test/libtest/meson.build\n> > @@ -1,4 +1,5 @@\n> >  libtest_sources = files([\n> > +    'camera_test.cpp',\n> >      'test.cpp',\n> >  ])\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B23C36136F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Nov 2019 16:49:17 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 18852FF802;\n\tFri, 15 Nov 2019 15:49:16 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 15 Nov 2019 16:51:18 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191115155118.wo3gniummxwqlkyy@uno.localdomain>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-2-laurent.pinchart@ideasonboard.com>\n\t<20191114080752.GF7353@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"xila2durwprcfpb7\"","Content-Disposition":"inline","In-Reply-To":"<20191114080752.GF7353@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 01/24] test: Extract CameraTest\n\tclass out of camera tests to libtest","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 15 Nov 2019 15:49:17 -0000"}},{"id":3036,"web_url":"https://patchwork.libcamera.org/comment/3036/","msgid":"<20191118003652.GH4853@pendragon.ideasonboard.com>","date":"2019-11-18T00:36:52","subject":"Re: [libcamera-devel] [PATCH v2 01/24] test: Extract CameraTest\n\tclass out of camera tests to libtest","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Nov 15, 2019 at 04:51:18PM +0100, Jacopo Mondi wrote:\n> On Thu, Nov 14, 2019 at 09:07:52AM +0100, Niklas Söderlund wrote:\n> > On 2019-11-08 22:53:46 +0200, Laurent Pinchart wrote:\n> > > Many tests other than the camera/ tests use a camera. To increase code\n> > > sharing, move the base CameraTest class to the test library. The class\n> > > becomes a helper that doesn't inherit from Test anymore (to avoid\n> > > diamond inheritance issues when more such helpers will exist).\n> > >\n> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >\n> > I'm not overly found of how status_ is checked in users, but I like the\n> > overall change more and this is test code.\n> \n> I agree this requires to be handled in the sub-classes' init() and\n> it's not super nice\n\nI don't like it much either, I think we'll need to refactor the tests in\nthe not too distant future, and this is something I believe we should\naddress then.\n\n> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> >\n> > > ---\n> > >  test/camera/buffer_import.cpp            | 14 ++++-----\n> > >  test/camera/capture.cpp                  | 15 ++++++---\n> > >  test/camera/configuration_default.cpp    | 16 ++++++++--\n> > >  test/camera/configuration_set.cpp        | 15 ++++++---\n> > >  test/camera/meson.build                  |  2 +-\n> > >  test/camera/statemachine.cpp             | 15 ++++++---\n> > >  test/controls/control_list.cpp           | 39 +++++-------------------\n> > >  test/{camera => libtest}/camera_test.cpp | 24 +++++++++------\n> > >  test/{camera => libtest}/camera_test.h   | 12 +++-----\n> > >  test/libtest/meson.build                 |  1 +\n> > >  10 files changed, 77 insertions(+), 76 deletions(-)\n> > >  rename test/{camera => libtest}/camera_test.cpp (55%)\n> > >  rename test/{camera => libtest}/camera_test.h (77%)\n> > >\n> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > > index bbc5a25c4019..3efe02704c02 100644\n> > > --- a/test/camera/buffer_import.cpp\n> > > +++ b/test/camera/buffer_import.cpp\n> > > @@ -18,6 +18,7 @@\n> > >  #include \"v4l2_videodevice.h\"\n> > >\n> > >  #include \"camera_test.h\"\n> > > +#include \"test.h\"\n> > >\n> > >  using namespace libcamera;\n> > >\n> > > @@ -254,11 +255,11 @@ private:\n> > >  \tbool done_;\n> > >  };\n> > >\n> > > -class BufferImportTest : public CameraTest\n> > > +class BufferImportTest : public CameraTest, public Test\n> > >  {\n> > >  public:\n> > >  \tBufferImportTest()\n> > > -\t\t: CameraTest()\n> > > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > >  \t{\n> > >  \t}\n> > >\n> > > @@ -350,11 +351,10 @@ protected:\n> > >\n> > >  \tint init()\n> > >  \t{\n> > > -\t\tint ret = CameraTest::init();\n> > > -\t\tif (ret)\n> > > -\t\t\treturn ret;\n> > > +\t\tif (status_ != TestPass)\n> > > +\t\t\treturn status_;\n> > >\n> > > -\t\tret = sink_.init();\n> > > +\t\tint ret = sink_.init();\n> > >  \t\tif (ret != TestPass) {\n> > >  \t\t\tcleanup();\n> > >  \t\t\treturn ret;\n> > > @@ -422,8 +422,6 @@ protected:\n> > >\n> > >  \t\tcamera_->stop();\n> > >  \t\tcamera_->freeBuffers();\n> > > -\n> > > -\t\tCameraTest::cleanup();\n> > >  \t}\n> > >\n> > >  private:\n> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > > index 791ccad15f70..08cce9c7cbaf 100644\n> > > --- a/test/camera/capture.cpp\n> > > +++ b/test/camera/capture.cpp\n> > > @@ -8,13 +8,20 @@\n> > >  #include <iostream>\n> > >\n> > >  #include \"camera_test.h\"\n> > > +#include \"test.h\"\n> > >\n> > >  using namespace std;\n> > >\n> > >  namespace {\n> > >\n> > > -class Capture : public CameraTest\n> > > +class Capture : public CameraTest, public Test\n> > >  {\n> > > +public:\n> > > +\tCapture()\n> > > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > > +\t{\n> > > +\t}\n> > > +\n> > >  protected:\n> > >  \tunsigned int completeBuffersCount_;\n> > >  \tunsigned int completeRequestsCount_;\n> > > @@ -46,14 +53,12 @@ protected:\n> > >\n> > >  \tint init() override\n> > >  \t{\n> > > -\t\tint ret = CameraTest::init();\n> > > -\t\tif (ret)\n> > > -\t\t\treturn ret;\n> > > +\t\tif (status_ != TestPass)\n> > > +\t\t\treturn status_;\n> > >\n> > >  \t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> > >  \t\tif (!config_ || config_->size() != 1) {\n> > >  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > > -\t\t\tCameraTest::cleanup();\n> > >  \t\t\treturn TestFail;\n> > >  \t\t}\n> > >\n> > > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp\n> > > index ce2ec5d02e7b..31c908d2449e 100644\n> > > --- a/test/camera/configuration_default.cpp\n> > > +++ b/test/camera/configuration_default.cpp\n> > > @@ -8,15 +8,27 @@\n> > >  #include <iostream>\n> > >\n> > >  #include \"camera_test.h\"\n> > > +#include \"test.h\"\n> > >\n> > >  using namespace std;\n> > >\n> > >  namespace {\n> > >\n> > > -class ConfigurationDefault : public CameraTest\n> > > +class ConfigurationDefault : public CameraTest, public Test\n> > >  {\n> > > +public:\n> > > +\tConfigurationDefault()\n> > > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > > +\t{\n> > > +\t}\n> > > +\n> > >  protected:\n> > > -\tint run()\n> > > +\tint init() override\n> > > +\t{\n> > > +\t\treturn status_;\n> > > +\t}\n> > > +\n> > > +\tint run() override\n> > >  \t{\n> > >  \t\tstd::unique_ptr<CameraConfiguration> config;\n> > >\n> > > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> > > index f88da96ca2b7..b4b5968115e8 100644\n> > > --- a/test/camera/configuration_set.cpp\n> > > +++ b/test/camera/configuration_set.cpp\n> > > @@ -8,24 +8,29 @@\n> > >  #include <iostream>\n> > >\n> > >  #include \"camera_test.h\"\n> > > +#include \"test.h\"\n> > >\n> > >  using namespace std;\n> > >\n> > >  namespace {\n> > >\n> > > -class ConfigurationSet : public CameraTest\n> > > +class ConfigurationSet : public CameraTest, public Test\n> > >  {\n> > > +public:\n> > > +\tConfigurationSet()\n> > > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > > +\t{\n> > > +\t}\n> > > +\n> > >  protected:\n> > >  \tint init() override\n> > >  \t{\n> > > -\t\tint ret = CameraTest::init();\n> > > -\t\tif (ret)\n> > > -\t\t\treturn ret;\n> > > +\t\tif (status_ != TestPass)\n> > > +\t\t\treturn status_;\n> > >\n> > >  \t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> > >  \t\tif (!config_ || config_->size() != 1) {\n> > >  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > > -\t\t\tCameraTest::cleanup();\n> > >  \t\t\treturn TestFail;\n> > >  \t\t}\n> > >\n> > > diff --git a/test/camera/meson.build b/test/camera/meson.build\n> > > index d6fd66b8f89e..e2a6660a7a92 100644\n> > > --- a/test/camera/meson.build\n> > > +++ b/test/camera/meson.build\n> > > @@ -9,7 +9,7 @@ camera_tests = [\n> > >  ]\n> > >\n> > >  foreach t : camera_tests\n> > > -    exe = executable(t[0], [t[1], 'camera_test.cpp'],\n> > > +    exe = executable(t[0], t[1],\n> > >                       dependencies : libcamera_dep,\n> > >                       link_with : test_libraries,\n> > >                       include_directories : test_includes_internal)\n> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > > index 12d5e0e1d534..afa13ba77b0b 100644\n> > > --- a/test/camera/statemachine.cpp\n> > > +++ b/test/camera/statemachine.cpp\n> > > @@ -8,13 +8,20 @@\n> > >  #include <iostream>\n> > >\n> > >  #include \"camera_test.h\"\n> > > +#include \"test.h\"\n> > >\n> > >  using namespace std;\n> > >\n> > >  namespace {\n> > >\n> > > -class Statemachine : public CameraTest\n> > > +class Statemachine : public CameraTest, public Test\n> > >  {\n> > > +public:\n> > > +\tStatemachine()\n> > > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > > +\t{\n> > > +\t}\n> > > +\n> > >  protected:\n> > >  \tint testAvailable()\n> > >  \t{\n> > > @@ -233,14 +240,12 @@ protected:\n> > >\n> > >  \tint init() override\n> > >  \t{\n> > > -\t\tint ret = CameraTest::init();\n> > > -\t\tif (ret)\n> > > -\t\t\treturn ret;\n> > > +\t\tif (status_ != TestPass)\n> > > +\t\t\treturn status_;\n> > >\n> > >  \t\tdefconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> > >  \t\tif (!defconf_) {\n> > >  \t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > > -\t\t\tCameraTest::cleanup();\n> > >  \t\t\treturn TestFail;\n> > >  \t\t}\n> > >\n> > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > > index 5af53f64bb6c..4d212abd09e6 100644\n> > > --- a/test/controls/control_list.cpp\n> > > +++ b/test/controls/control_list.cpp\n> > > @@ -13,32 +13,22 @@\n> > >  #include <libcamera/controls.h>\n> > >\n> > >  #include \"camera_controls.h\"\n> > > +\n> > > +#include \"camera_test.h\"\n> > >  #include \"test.h\"\n> > >\n> > >  using namespace std;\n> > >  using namespace libcamera;\n> > >\n> > > -class ControlListTest : public Test\n> > > +class ControlListTest : public CameraTest, public Test\n> > >  {\n> > > -protected:\n> > > -\tint init()\n> > > +public:\n> > > +\tControlListTest()\n> > > +\t\t: CameraTest(\"VIMC Sensor B\")\n> > >  \t{\n> > > -\t\tcm_ = new CameraManager();\n> > > -\n> > > -\t\tif (cm_->start()) {\n> > > -\t\t\tcout << \"Failed to start camera manager\" << endl;\n> > > -\t\t\treturn TestFail;\n> > > -\t\t}\n> > > -\n> > > -\t\tcamera_ = cm_->get(\"VIMC Sensor B\");\n> > > -\t\tif (!camera_) {\n> > > -\t\t\tcout << \"Can not find VIMC camera\" << endl;\n> > > -\t\t\treturn TestSkip;\n> > > -\t\t}\n> > > -\n> > > -\t\treturn TestPass;\n> > >  \t}\n> > >\n> > > +protected:\n> \n> Don't we need to check status_ in init() ?\n> \n> Apart from this:\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> Thanks\n>    j\n> \n> > >  \tint run()\n> > >  \t{\n> > >  \t\tCameraControlValidator validator(camera_.get());\n> > > @@ -156,21 +146,6 @@ protected:\n> > >\n> > >  \t\treturn TestPass;\n> > >  \t}\n> > > -\n> > > -\tvoid cleanup()\n> > > -\t{\n> > > -\t\tif (camera_) {\n> > > -\t\t\tcamera_->release();\n> > > -\t\t\tcamera_.reset();\n> > > -\t\t}\n> > > -\n> > > -\t\tcm_->stop();\n> > > -\t\tdelete cm_;\n> > > -\t}\n> > > -\n> > > -private:\n> > > -\tCameraManager *cm_;\n> > > -\tstd::shared_ptr<Camera> camera_;\n> > >  };\n> > >\n> > >  TEST_REGISTER(ControlListTest)\n> > > diff --git a/test/camera/camera_test.cpp b/test/libtest/camera_test.cpp\n> > > similarity index 55%\n> > > rename from test/camera/camera_test.cpp\n> > > rename to test/libtest/camera_test.cpp\n> > > index 101e31fbce79..2ae4d6776f2e 100644\n> > > --- a/test/camera/camera_test.cpp\n> > > +++ b/test/libtest/camera_test.cpp\n> > > @@ -8,35 +8,39 @@\n> > >  #include <iostream>\n> > >\n> > >  #include \"camera_test.h\"\n> > > +#include \"test.h\"\n> > >\n> > >  using namespace libcamera;\n> > >  using namespace std;\n> > >\n> > > -int CameraTest::init()\n> > > +CameraTest::CameraTest(const char *name)\n> > >  {\n> > >  \tcm_ = new CameraManager();\n> > >\n> > >  \tif (cm_->start()) {\n> > > -\t\tcout << \"Failed to start camera manager\" << endl;\n> > > -\t\treturn TestFail;\n> > > +\t\tcerr << \"Failed to start camera manager\" << endl;\n> > > +\t\tstatus_ = TestFail;\n> > > +\t\treturn;\n> > >  \t}\n> > >\n> > > -\tcamera_ = cm_->get(\"VIMC Sensor B\");\n> > > +\tcamera_ = cm_->get(name);\n> > >  \tif (!camera_) {\n> > > -\t\tcout << \"Can not find VIMC camera\" << endl;\n> > > -\t\treturn TestSkip;\n> > > +\t\tcerr << \"Can not find '\" << name << \"' camera\" << endl;\n> > > +\t\tstatus_ = TestSkip;\n> > > +\t\treturn;\n> > >  \t}\n> > >\n> > >  \t/* Sanity check that the camera has streams. */\n> > >  \tif (camera_->streams().empty()) {\n> > > -\t\tcout << \"Camera has no stream\" << endl;\n> > > -\t\treturn TestFail;\n> > > +\t\tcerr << \"Camera has no stream\" << endl;\n> > > +\t\tstatus_ = TestFail;\n> > > +\t\treturn;\n> > >  \t}\n> > >\n> > > -\treturn TestPass;\n> > > +\tstatus_ = TestPass;\n> > >  }\n> > >\n> > > -void CameraTest::cleanup()\n> > > +CameraTest::~CameraTest()\n> > >  {\n> > >  \tif (camera_) {\n> > >  \t\tcamera_->release();\n> > > diff --git a/test/camera/camera_test.h b/test/libtest/camera_test.h\n> > > similarity index 77%\n> > > rename from test/camera/camera_test.h\n> > > rename to test/libtest/camera_test.h\n> > > index e57b05eb28a9..0b6bad05e37c 100644\n> > > --- a/test/camera/camera_test.h\n> > > +++ b/test/libtest/camera_test.h\n> > > @@ -9,22 +9,18 @@\n> > >\n> > >  #include <libcamera/libcamera.h>\n> > >\n> > > -#include \"test.h\"\n> > > -\n> > >  using namespace libcamera;\n> > >\n> > > -class CameraTest : public Test\n> > > +class CameraTest\n> > >  {\n> > >  public:\n> > > -\tCameraTest()\n> > > -\t\t: cm_(nullptr) {}\n> > > +\tCameraTest(const char *name);\n> > > +\t~CameraTest();\n> > >\n> > >  protected:\n> > > -\tint init();\n> > > -\tvoid cleanup();\n> > > -\n> > >  \tCameraManager *cm_;\n> > >  \tstd::shared_ptr<Camera> camera_;\n> > > +\tint status_;\n> > >  };\n> > >\n> > >  #endif /* __LIBCAMERA_CAMERA_TEST_H__ */\n> > > diff --git a/test/libtest/meson.build b/test/libtest/meson.build\n> > > index ca762b4421c2..3e798ef3810e 100644\n> > > --- a/test/libtest/meson.build\n> > > +++ b/test/libtest/meson.build\n> > > @@ -1,4 +1,5 @@\n> > >  libtest_sources = files([\n> > > +    'camera_test.cpp',\n> > >      'test.cpp',\n> > >  ])\n> > >","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD95B60C33\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Nov 2019 01:36:56 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 389B8A04;\n\tMon, 18 Nov 2019 01:36:56 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1574037416;\n\tbh=Brztm7E8g1diNw5B6rz0c8Zdj7F0V5rtOdUWyu+0dT0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JLNq/JIMJtFwBHxlew4RVVQAbLVEy5co8X/8sxflERMYhNrXA8zH+nOf1t1Z4lHJh\n\tVKilmlFw8g3gNCOiwEgWdxd8+PFIGWatirYTz5W9koZlR/XVUqE2ydfI9unWmMxXi0\n\tD2bCpsu+QdwrNHjvbM8QAohImZOsJC0cW0fN6SKQ=","Date":"Mon, 18 Nov 2019 02:36:52 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191118003652.GH4853@pendragon.ideasonboard.com>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-2-laurent.pinchart@ideasonboard.com>\n\t<20191114080752.GF7353@bigcity.dyn.berto.se>\n\t<20191115155118.wo3gniummxwqlkyy@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191115155118.wo3gniummxwqlkyy@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 01/24] test: Extract CameraTest\n\tclass out of camera tests to libtest","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 18 Nov 2019 00:36:57 -0000"}}]