[libcamera-devel] [PATCH v3 03/16] test: control_list: Test ControlList::merge()

[Hirokazu Honda]

Hi Jacopo, thank you for the patch.

On Thu, Apr 22, 2021 at 3:14 AM Niklas Söderlund
<niklas.soderlund at 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 at 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 at 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 at chromium.org>

> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel