[libcamera-devel] [PATCH v2 1/5] test: list-cameras: Make test output more verbose

[Jacopo Mondi]

Hi Laurent,

On Mon, Jan 21, 2019 at 11:09:56AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jan 16, 2019 at 02:59:45PM +0100, Jacopo Mondi wrote:
> > Make the list-cameras test a little more verbose to better describe
> > failures. While at there use the Test class defined TestStatus value as
> > test exit codes, and skip the test if no camera gets registred.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  test/list-cameras.cpp | 41 +++++++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> > index e2026c9..c9dc199 100644
> > --- a/test/list-cameras.cpp
> > +++ b/test/list-cameras.cpp
> > @@ -7,6 +7,7 @@
> >
> >  #include <iostream>
> >
> > +#include <libcamera/camera.h>
> >  #include <libcamera/camera_manager.h>
> >
> >  #include "test.h"
> > @@ -14,27 +15,55 @@
> >  using namespace std;
> >  using namespace libcamera;
> >
> > +/*
> > + * List all cameras registered in the system, using the CameraManager.
> > + *
> > + * In order for the test to run successfully, a pipeline handler supporting
> > + * the current test platform should be available in the library.
> > + * Libcamera provides a platform-agnostic pipeline handler for the 'vimc'
> > + * virtual media device, which can be used for testing purposes.
> > + *
> > + * The test tries to list all cameras registered in the system, if no
> > + * camera is found the test is skipped. If the test gets skipped on a
> > + * platform where a pipeline handler is known to be available, an error
> > + * in camera enumeration might get unnoticed.
> > + */
> >  class ListTest : public Test
> >  {
> >  protected:
> >  	int init()
> >  	{
> >  		cm = CameraManager::instance();
> > -		cm->start();
> > +
> > +		int ret = cm->start();
> > +		if (ret) {
> > +			cerr << "Failed to start the CameraManager" << endl;
> > +			return TestFail;
> > +		}
> >
> >  		return 0;
> >  	}
> >
> >  	int run()
> >  	{
> > -		unsigned int count = 0;
> > +		vector<string> cameraList = cm->list();
> > +		if (cameraList.empty()) {
> > +			cerr << "No cameras registered in the system: test skip" << endl
> > +			     << "This might be expected if no pipeline handler supports the testing platform" << endl;
> > +			return TestSkip;
>
> As commented before, I have some doubts on the value of this test,
> especially aftere rebasing it on the lifetime management series, as it
> will then only return TestSkip or TestPass. A test that can't fail has
> limited use :-) I think listing cameras should be part of a libcamera
> command line tool, not a test case.
>
> I wont block this patch, but please be aware that I plan to remove the
> list-cameras test.
>

This was my understanding as well, that's why I have sent an IPU3
pipeline specific test. I'm fine with this being turned into a
utility, and encourage pipeline specific tests to verify enumeration
and matching works as expected.

Thanks
  j

> > +		}
> > +
> > +		for (auto name : cameraList) {
> > +			Camera *cam = cm->get(name);
> > +			if (!cam) {
> > +				cerr << "Failed to get camera '" << name << "' by name" << endl;
> > +				return TestFail;
> > +			}
> >
> > -		for (auto name : cm->list()) {
> > -			cout << "- " << name << endl;
> > -			count++;
> > +			cout << "Found camera '" << cam->name() << "'" << endl;
> >  		}
> >
> > -		return count ? 0 : -ENODEV;
> > +		return TestPass;
> >  	}
> >
> >  	void cleanup()
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190121/fc6c6738/attachment.sig>