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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tuesday, 15 January 2019 16:07:46 EET 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 | 52 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> index e2026c9..b6b0a39 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,64 @@
>  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 un-noticed.
> + */
>  class ListTest : public Test
>  {
>  protected:
>  	int init()
>  	{
>  		cm = CameraManager::instance();
> -		cm->start();
> +		if (!cm) {
> +			cerr << "Failed to get CameraManager instance" << endl;

This can't happen, CameraManager::instance() is guaranteed to succeed.

> +			return TestFail;
> +		}
> 
> -		return 0;
> +		int ret = cm->start();
> +		if (ret) {
> +			cerr << "Failed to start the CameraManager" << endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;

Semantically speaking, this doesn't indicate that the test passed. I won't if 
return 0 wouldn't be better.

>  	}
> 
>  	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;

Will we support multi-line messages when moving to TAP ?

What exactly are we testing here ? If the goal is to test that libcamera 
successfully enumerated cameras, we should instead check that at least one 
supported media device is available in the init() function, and make this case 
a failure.

> +			return TestSkip;
> +		}
> +
> +		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 << "Camera '" << cam->name()
> +			     << "' registered" << endl;

Maybe "Found camera '...'" ? The test doesn't register cameras.

>  		}
> 
> -		return count ? 0 : -ENODEV;
> +		return TestPass;
>  	}
> 
>  	void cleanup()

-- 
Regards,

Laurent Pinchart