| Message ID | 20210421160319.42251-4-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your patch. On 2021-04-21 18:03:06 +0200, Jacopo Mondi wrote: > Test the newly introduce ControlList::merge() method by creating nit: s/newly introduce// > a new list with a single control and merging it with the existing one. > > Test that the merged list contains all the controls and their values > are not changed. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > test/controls/control_list.cpp | 40 ++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > index 2b321ddd6fa4..91b38993abf9 100644 > --- a/test/controls/control_list.cpp > +++ b/test/controls/control_list.cpp > @@ -150,6 +150,46 @@ protected: > return TestFail; > } > > + /* > + * Create a new list with a new control and merge it with the > + * existing one, verifying that the existing controls > + * values does not get changed. > + */ > + ControlList mergeList(controls::controls, &validator); > + mergeList.set(controls::Saturation, 0.4f); > + > + mergeList.merge(list); > + if (mergeList.size() != 3) { > + cout << "Merged list should contain three elements" << endl; > + return TestFail; > + } > + > + if (!mergeList.contains(controls::Brightness) || > + !mergeList.contains(controls::Contrast) || > + !mergeList.contains(controls::Saturation)) { > + cout << "Merged list does not contain all controls it should" > + << endl; nit: s/ it should// With or without the nits fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > + return TestFail; > + } > + > + if (mergeList.get(controls::Brightness) != 0.5f) { > + cout << "Brightness control value changed after merging lists" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Contrast) != 1.1f) { > + cout << "Contrast control value changed after merging lists" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Saturation) != 0.4f) { > + cout << "Saturation control value changed after merging lists" > + << endl; > + return TestFail; > + } > + > return TestPass; > } > }; > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, thank you for the patch. On Thu, Apr 22, 2021 at 3:14 AM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hi Jacopo, > > Thanks for your patch. > > On 2021-04-21 18:03:06 +0200, Jacopo Mondi wrote: > > Test the newly introduce ControlList::merge() method by creating > > nit: s/newly introduce// > > > a new list with a single control and merging it with the existing one. > > > > Test that the merged list contains all the controls and their values > > are not changed. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > test/controls/control_list.cpp | 40 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > > index 2b321ddd6fa4..91b38993abf9 100644 > > --- a/test/controls/control_list.cpp > > +++ b/test/controls/control_list.cpp > > @@ -150,6 +150,46 @@ protected: > > return TestFail; > > } > > > > + /* > > + * Create a new list with a new control and merge it with the > > + * existing one, verifying that the existing controls > > + * values does not get changed. > > + */ > > + ControlList mergeList(controls::controls, &validator); > > + mergeList.set(controls::Saturation, 0.4f); > > + > > + mergeList.merge(list); > > + if (mergeList.size() != 3) { > > + cout << "Merged list should contain three elements" << endl; > > + return TestFail; > > + } > > + Shall we check if list.size() is one? > > + if (!mergeList.contains(controls::Brightness) || > > + !mergeList.contains(controls::Contrast) || > > + !mergeList.contains(controls::Saturation)) { > > + cout << "Merged list does not contain all controls it should" > > + << endl; > > nit: s/ it should// > > With or without the nits fixed, > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > + return TestFail; > > + } > > + > > + if (mergeList.get(controls::Brightness) != 0.5f) { > > + cout << "Brightness control value changed after merging lists" > > + << endl; > > + return TestFail; > > + } > > + > > + if (mergeList.get(controls::Contrast) != 1.1f) { > > + cout << "Contrast control value changed after merging lists" > > + << endl; > > + return TestFail; > > + } > > + > > + if (mergeList.get(controls::Saturation) != 0.4f) { > > + cout << "Saturation control value changed after merging lists" > > + << endl; > > + return TestFail; > > + } > > + > > return TestPass; > > } > > }; > > -- > > 2.31.1 I would split the function to pieces to test one thing in each piece. But it should not have to be done in this patch. Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Wed, Apr 21, 2021 at 06:03:06PM +0200, Jacopo Mondi wrote: > Test the newly introduce ControlList::merge() method by creating s/introduce/introduced/ or just drop "newly introduce". > a new list with a single control and merging it with the existing one. > > Test that the merged list contains all the controls and their values > are not changed. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > test/controls/control_list.cpp | 40 ++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > index 2b321ddd6fa4..91b38993abf9 100644 > --- a/test/controls/control_list.cpp > +++ b/test/controls/control_list.cpp > @@ -150,6 +150,46 @@ protected: > return TestFail; > } > > + /* > + * Create a new list with a new control and merge it with the > + * existing one, verifying that the existing controls > + * values does not get changed. "existing control values don't get changed" > + */ > + ControlList mergeList(controls::controls, &validator); > + mergeList.set(controls::Saturation, 0.4f); > + > + mergeList.merge(list); > + if (mergeList.size() != 3) { > + cout << "Merged list should contain three elements" << endl; > + return TestFail; > + } > + > + if (!mergeList.contains(controls::Brightness) || > + !mergeList.contains(controls::Contrast) || > + !mergeList.contains(controls::Saturation)) { > + cout << "Merged list does not contain all controls it should" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Brightness) != 0.5f) { > + cout << "Brightness control value changed after merging lists" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Contrast) != 1.1f) { > + cout << "Contrast control value changed after merging lists" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Saturation) != 0.4f) { > + cout << "Saturation control value changed after merging lists" > + << endl; > + return TestFail; > + } Could you please also test that list.size() is zero after the merge ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > return TestPass; > } > };
diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp index 2b321ddd6fa4..91b38993abf9 100644 --- a/test/controls/control_list.cpp +++ b/test/controls/control_list.cpp @@ -150,6 +150,46 @@ protected: return TestFail; } + /* + * Create a new list with a new control and merge it with the + * existing one, verifying that the existing controls + * values does not get changed. + */ + ControlList mergeList(controls::controls, &validator); + mergeList.set(controls::Saturation, 0.4f); + + mergeList.merge(list); + if (mergeList.size() != 3) { + cout << "Merged list should contain three elements" << endl; + return TestFail; + } + + if (!mergeList.contains(controls::Brightness) || + !mergeList.contains(controls::Contrast) || + !mergeList.contains(controls::Saturation)) { + cout << "Merged list does not contain all controls it should" + << endl; + return TestFail; + } + + if (mergeList.get(controls::Brightness) != 0.5f) { + cout << "Brightness control value changed after merging lists" + << endl; + return TestFail; + } + + if (mergeList.get(controls::Contrast) != 1.1f) { + cout << "Contrast control value changed after merging lists" + << endl; + return TestFail; + } + + if (mergeList.get(controls::Saturation) != 0.4f) { + cout << "Saturation control value changed after merging lists" + << endl; + return TestFail; + } + return TestPass; } };
Test the newly introduce ControlList::merge() method by creating a new list with a single control and merging it with the existing one. Test that the merged list contains all the controls and their values are not changed. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- test/controls/control_list.cpp | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)