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

[Kieran Bingham]

Hi Laurent,

On 23/06/2019 23:44, Laurent Pinchart wrote:
> 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

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)


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


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


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


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

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.



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