[libcamera-devel] [RFC PATCH v2 7/9] libcamera: test: Add ControlList tests
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 24 14:36:26 CEST 2019
Hi Kieran,
On Mon, Jun 24, 2019 at 01:16:04PM +0100, Kieran Bingham wrote:
> On 23/06/2019 23:44, Laurent Pinchart wrote:
> > On Sun, Jun 23, 2019 at 05:39:57PM +0200, Niklas Söderlund wrote:
> >> On 2019-06-21 17:13:59 +0100, Kieran Bingham wrote:
> >>> Extend the Controls tests with specific testing of the ControlList
> >>> infrastructure and public APIs.
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>> ---
> >>> test/controls.cpp | 99 +++++++++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 99 insertions(+)
> >>>
> >>> diff --git a/test/controls.cpp b/test/controls.cpp
> >>> index 94e89f270108..c4145b7a0543 100644
> >>> --- a/test/controls.cpp
> >>> +++ b/test/controls.cpp
> >>> @@ -25,6 +25,101 @@ protected:
> >>> return TestPass;
> >>> }
> >>>
> >>> + int testControlList()
> >>> + {
> >>> + ControlList list;
> >>> +
> >>> + if (list.contains(ManualGain)) {
> >>> + cout << "Unexpected item in the bagging area" << endl;
> >>
> >> If this error was returned I would not know what went wrong without
> >> looking at the source ;-)
> >>
> >> This is a test case so I think it's fine,
> >>
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>
> >>> + return TestFail;
> >>> + }
> >
> > Isn't this a bit too random ? :-) I don't really see how this could be
> > the case, and I don't really see what value this test brings. Same for
> > most of the ones below. I think we should try to write test that focus
>
> I put in the tongue in cheek "Unexpected item in the bagging area"
> because I have no expectation of this firing - unless someone starts
> screwing about with the internals of the ControlList.
>
> But that doesn't mean I don't think it has a value in the test-suite. At
> that point - if they are doing that - they are already looking at the
> appropriate code - This message just states that someone has broken the
> internals. It should be noticed immediately by that person (or they
> aren't running the tests)
I'd say that if it gets that far it should even be noticed without the
test suite. We don't have a test checking if something has broken the
fundamental laws of physics, it will be noticeable when planets start
crashing into each other :-)
> > on use cases, on expected problems, and on API contracts. Sure, a
> > ControlList being empty after construction is an API contract, but if
> > you go there you can as well test that it contains none of the defined
> > controls. Or that calling size() twice in a row will return the same
> > value.
>
> I'm trying to exercise each exposed call from the *application public*
> API of the ControlList.
>
> Should we leave public API functions untested just because they are
> 'trivial' (i.e. size, should return the size....)
>
> If this was a libcamera internal object I would be more likely to agree
> that the tests are overkill. But this is an interface which applications
> will be able to use.
We shouldn't leave them untested, we should implement meaningful tests
:-)
> > An example of a potentially useful test, in the current implementation,
> > would be that the has function operates correctly on the id() contained
> > in the ControlInfo. It would insert two items with different
> > ControlInfo that have the same ID, and verify that the second one
> > replaces the first one (both for the key and the value).
>
> so I've now added this (psuedocode):
>
> ControlInfo gainInfo(ManualGain);
> ControlInfo secondGainInfo(ManualGain);
>
> listSize = list.size();
> list.update(gainInfo, 200);
> assert(list[ManualGain] == 200);
> list.update(secondGainInfo, 201);
> assert(list[ManualGain] == 201);
> assert(list.size()==listSize);
And you've also tested the size() method :-)
Note that list.update(gainInfo, 200) should add a control to the list,
so the size will change.
> > In other words, let's not go too trivial.
>
> ControlList is a container. And more (*much more*) importantly, this is
> a container exposed to libcamera applications.
>
> This is testing/enforcing/guaranteeing that container API.
>
> Should someone shift the underlying storage mechanism, I would expect
> these calls to still function. That's the simple point of the unit tests
> right?
>
> How else should we enforce the API contract designs if not through
> adding tests?
Again, this isn't about not adding tests, it's about adding meaningful
tests :-)
You test the size() method by making sure that it returns 1 after adding
one element. What if someone modifies the storage mechanism and
incorrectly returns !empty() for the size() implementation ? Your test
will succeed, but size() won't behave correctly with two elements.
There's always a knowledge of the implementation hardcoded in tests, as
even if you tested for size() in the 0, 1 and 2 cases, it could still
fail for other values.
My point is that the test shouldn't be random, but should meaningfully
test for problems we foresee (or problems we have experienced). In this
case I would write a test suite that essentially tests the underlying
containers, I would concentrate on the added value of the ControlList
class.
If you want to test the underlying container then I think you'll have to
test it function by function, including the iterators.
> > And another very useful test would operate at the request level with the
> > VIMC pipeline handler, and verify that controls are correctly applied.
> > That's more work though :-)
>
> Sure - but that's not testing the ControlList, that's testing Controls
> and V4L2Controls.
It would still indirectly test the control list :-) But I agree that
indirect tests don't necessarily provide the coverage we need.
> Useful to add - but these were to validate and verify the function of
> the container object.
>
> This test-set is "testControlList".
>
> Testing the controls themselves is a different layer and while they
> might be in this 'controls.cpp file', they would be in a function called
> testControls().
>
> Testing Controls require the controls to be clearly defined, and have an
> implementation for managing the controls in the VIMC (or required)
> pipeline handler.
>
> IMO Those should be added, but once the pipeline handlers are updated to
> provide that required functionality.
Yes, that will be done in a separate step.
> >>> +
> >>> + if (list.size() != 0) {
> >>> + cout << "List should contain zero elements" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (!list.empty()) {
> >>> + cout << "List expected to be empty" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + /* Set a control */
> >>> + list[ManualBrightness] = 255;
> >>> +
> >>> + /* Contains should still not find a non set control */
> >>> + if (list.contains(ManualGain)) {
> >>> + cout << "Unexpected item in the bagging area" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (list.size() != 1) {
> >>> + cout << "List should contain one element" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (list.empty()) {
> >>> + cout << "List not expected to be empty" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + /* Finally lets find that Gain */
> >>> + list[ManualGain] = 128;
> >>> + if (!list.contains(ManualGain)) {
> >>> + cout << "Couldn't identify an in-list control" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + /* Validate value retrieval */
> >>> + if (list[ManualGain].getInt() != 128 ||
> >>> + list[ManualBrightness].getInt() != 255) {
> >>> + cout << "Failed to retrieve control value" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + /* Update using ControlInfo */
> >>> + ControlInfo gainInfo(ManualGain);
> >>> + list.update(gainInfo, 200);
> >>> + if (list[ManualGain].getInt() != 200) {
> >>> + cout << "Failed to update control value" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + /* Create a new list from an existing one */
> >>> + ControlList newList;
> >>> +
> >>> + newList.update(list);
> >>> +
> >>> + /*
> >>> + * Clear down the original list and assert items are still in
> >>> + * the new list
> >>> + */
> >>> + list.reset();
> >>> +
> >>> + if (list.size() != 0) {
> >>> + cout << "Old List should contain zero elements" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (!list.empty()) {
> >>> + cout << "Old List expected to be empty" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (newList.size() != 2) {
> >>> + cout << "New list with incorrect size" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (!(newList.contains(ManualGain) &&
> >>> + newList.contains(ManualBrightness))) {
> >>> + cout << "New list with incorrect items" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + return TestPass;
> >>> + }
> >>> +
> >>> int run()
> >>> {
> >>> int ret;
> >>> @@ -33,6 +128,10 @@ protected:
> >>> if (ret)
> >>> return ret;
> >>>
> >>> + ret = testControlList();
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> return TestPass;
> >>> }
> >>> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list