| Message ID | 20190621161401.28337-8-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for your work. 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@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@ragnatech.se> > + return TestFail; > + } > + > + 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; > } > }; > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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; > > } > > };
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@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@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; >>> } >>> }; >
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@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@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; > >>> } > >>> };
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@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@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; >>>>> } >>>>> }; >
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; + return TestFail; + } + + 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; } };
Extend the Controls tests with specific testing of the ControlList infrastructure and public APIs. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- test/controls.cpp | 99 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+)