| Message ID | 20190115140749.8297-2-jacopo@jmondi.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
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@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() >
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@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()
Hi Laurent, On Tue, Jan 15, 2019 at 06:08:21PM +0200, Laurent Pinchart wrote: > 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@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. > Correct, I'll drop. > > + 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. > Ok... > > } > > > > 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 ? > Might there be additional messages printed out along with the TAP-compatible string? What I mean is, can I "cerr << " and then create a TestResult object with the TAP messages. > 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. > Well, so, list-cameras test has an issue: we don't know if something went wrong in the device enumeration, pipeline handler matching and camera creation, or it is actually the case no pipeline handler is registered for the current test platform. Previously, the test failed if no pipeline handler got matched, even if there was actually no pipeline handler for the platform, or the VIMC driver was not loaded. My decision here was to skip the test, but to point out that this might hide some issues. If we manually query the media devices in the system, how would you decide that one media device has a valid pipeline manager associated? We can do that if we restrict the test to a known platform, like I've done for VIMC in the link handling test for media-device, but in the general case, how could we do that? The media device "xyz" might not be supported now, so no camera should be enumerated, but when an XYZPipelineHanlder is added, how should we determine that "xyz" is now supposed to be picked? How many cameras would we expect on "xyz" ? I had a thought on that, and the best option is to skip the test if no cameras gets created. I expect platform specific test to tell if everything is fine for the current platform or not.. We could have an IPU3-list-camera test soon, that verifies everything is fine when run on IPU3 and skip otherwise, but for this simple 'bootstrap' test, I think that's the best we could do now. At least it does not fail as it used to do if you don't have VIMC loaded. > > + 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. > Ok... Thanks j > > } > > > > - return count ? 0 : -ENODEV; > > + return TestPass; > > } > > > > void cleanup() > > -- > Regards, > > Laurent Pinchart > > >
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; + 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 + << "This might be expected if no pipeline handler" + << " supports the testing platform" + << endl; + 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; } - return count ? 0 : -ENODEV; + return TestPass; } void cleanup()
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@jmondi.org> --- test/list-cameras.cpp | 52 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 7 deletions(-)