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

[Kieran Bingham]

Hi Jacopo,

On 15/01/2019 14:07, 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.

s/registred/registered/

> 
> 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.

s/un-noticed/unnoticed/

> + */
>  class ListTest : public Test
>  {
>  protected:
>  	int init()
>  	{
>  		cm = CameraManager::instance();
> -		cm->start();
> +		if (!cm) {
> +			cerr << "Failed to get CameraManager instance" << endl;
> +			return TestFail;
> +		}
>  
> -		return 0;
> +		int ret = cm->start();
> +		if (ret) {
> +			cerr << "Failed to start the CameraManager" << endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
>  	}
>  
>  	int run()
>  	{
> -		unsigned int count = 0;
> +		vector<string> cameraList = cm->list();
> +		if (cameraList.empty()) {
> +			cerr << "No cameras registered in the system: test skip"
> +			     << endl
We allow ourselves up to 120 chars per line, I certainly think this endl
could go at the end of the previous line,

> +			     << "This might be expected if no pipeline handler"
> +			     << " supports the testing platform"
> +			     << endl;

And these three lines can be joined to reach 119 chars. You can even add
a full stop to the end of the sentence :).


> +			return TestSkip;
> +		}
> +
> +		for (auto name : cameraList) {
> +			Camera *cam = cm->get(name);
> +			if (!cam) {
> +				cerr << "Failed to get camera '" << name
> +				     << "' by name" << endl;

I think single quotes saves a lot of escaping ... it seems to make sense
I think.

> +				return TestFail;
> +			}
>  
> -		for (auto name : cm->list()) {
> -			cout << "- " << name << endl;
> -			count++;
> +			cout << "Camera '" << cam->name()
> +			     << "' registered" << endl;

I would also have made that a single line for the output. But it looks
like it's your preference to break at 80, so as you wish.

I would think string outputs is one place that makes sense to me to take
advantage of longer lines.


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

-- 
Regards
--
Kieran