[libcamera-devel] [RFC PATCH v2 7/9] libcamera: test: Add ControlList tests
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jun 24 18:57:50 CEST 2019
Hi Laurent,
On 24/06/2019 13:36, Laurent Pinchart wrote:
> 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.
Like by changing the class to a simple using-statement. ... I wonder if
these tests will all still work when I do :-)
>>
>> 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 :-)
Yes, and I (in the non-pseudo code) removed the basic size != 1 test,
and replaced it with size == 2 after both controls are now added.
>
> Note that list.update(gainInfo, 200) should add a control to the list,
> so the size will change.
>
Yes, that was psuedo code to describe. Sorry it was incorrect, the
implementation is correct. (for what it actually does).
Alternatively I could put:
>> list.update(gainInfo, 200);
>> assert(list[ManualGain] == 200);
>> listSize = list.size();
>> list.update(secondGainInfo, 201);
>> assert(list[ManualGain] == 201);
>> assert(list.size()==listSize);
And actually test that it doesn't change, but I don't want to rely on
the return value of list.size() being the same twice - I have hardcoded
the return value I expect (2) to ensure that a faulty size() always
returning 0 doesn't cause an unexpected test pass.
>>> 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.
I also already test the size() returns 2 at the end of this test procedure.
>
> 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.
I don't want to test the *underlying* container (I don't care if it
changes underneath). I want to guarantee that the ControlList API
contracts are held to account.
Right - except now I've finally got here, I have now seen your
suggestion of changing the ControlList to just be a 'using' statement to
directly bring in the unordered_map.
That would then remove the wrapper, and my desire to test the wrapper.
I believe I added the wrapper class with an intention to add extra
checks etc, so I'm not yet sure if it can be removed - but if it does -
it removes a lot of these tests. So win-win right...
>>> 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
--
Kieran
More information about the libcamera-devel
mailing list