[libcamera-devel] [RFC PATCH v2 7/9] libcamera: test: Add ControlList tests

[Laurent Pinchart]

Hi Kieran,

Thanks for your work.

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

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

In other words, let's not go too trivial.

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 :-)

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