[libgpiod][PATCH 0/2] tests: improve wait_multiple coverage

[libgpiod][PATCH 0/2] tests: improve wait_multiple coverage

[Kent Gibson]

A couple of patches for the v2 branch that improve the coverage of the
wait_multiple test case.

The first creates a mismatch between the chip offsets and bulk offsets
to highlight the problem with my initial implementation of
gpiod_line_event_wait_bulk() for uAPI v2.

The second adds a check on the event.offset field added for uAPI v2.

Kent Gibson (2):
  tests: create mismatch between chip and bulk offsets in wait_multiple
  tests: add check of event offset to wait_multiple

 tests/tests-event.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.28.0

[libgpiod][PATCH 1/2] tests: create mismatch between chip and bulk offsets in wait_multiple

[Kent Gibson]

Create a mismatch between the chip offsets and the bulk offsets to ensure
the implementation is not dependent on them matching.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tests/tests-event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/tests-event.c b/tests/tests-event.c
index 6066e45..a43f865 100644
--- a/tests/tests-event.c
+++ b/tests/tests-event.c
@@ -546,7 +546,7 @@ GPIOD_TEST_CASE(wait_multiple, 0, { 8 })
 	gpiod_test_return_if_failed();
 
 	gpiod_line_bulk_init(&bulk);
-	for (i = 0; i < 8; i++) {
+	for (i = 1; i < 8; i++) {
 		line = gpiod_chip_get_line(chip, i);
 		g_assert_nonnull(line);
 		gpiod_test_return_if_failed();
-- 
2.28.0

[libgpiod][PATCH 2/2] tests: add check of event offset to wait_multiple

[Kent Gibson]

The offset field is added for uAPI v2, so extend the test to check that
the value returned is correct.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tests/tests-event.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/tests-event.c b/tests/tests-event.c
index a43f865..0e5effe 100644
--- a/tests/tests-event.c
+++ b/tests/tests-event.c
@@ -570,6 +570,7 @@ GPIOD_TEST_CASE(wait_multiple, 0, { 8 })
 	ret = gpiod_line_event_read(line, &ev);
 	g_assert_cmpint(ret, ==, 0);
 	g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_RISING_EDGE);
+	g_assert_cmpint(ev.offset, ==, 4);
 }
 
 GPIOD_TEST_CASE(get_fd_when_values_requested, 0, { 8 })
-- 
2.28.0

Re: [libgpiod][PATCH 0/2] tests: improve wait_multiple coverage

[Bartosz Golaszewski]

On Wed, Oct 14, 2020 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> A couple of patches for the v2 branch that improve the coverage of the
> wait_multiple test case.
>
> The first creates a mismatch between the chip offsets and bulk offsets
> to highlight the problem with my initial implementation of
> gpiod_line_event_wait_bulk() for uAPI v2.
>
> The second adds a check on the event.offset field added for uAPI v2.
>
> Kent Gibson (2):
>   tests: create mismatch between chip and bulk offsets in wait_multiple
>   tests: add check of event offset to wait_multiple
>
>  tests/tests-event.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> --
> 2.28.0
>

I suppose that, if we don't want to do the backward compatible port
for now, these can be ignored?

Bartosz

Re: [libgpiod][PATCH 0/2] tests: improve wait_multiple coverage

[Kent Gibson]

On Wed, Oct 14, 2020 at 09:50:08AM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 14, 2020 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > A couple of patches for the v2 branch that improve the coverage of the
> > wait_multiple test case.
> >
> > The first creates a mismatch between the chip offsets and bulk offsets
> > to highlight the problem with my initial implementation of
> > gpiod_line_event_wait_bulk() for uAPI v2.
> >
> > The second adds a check on the event.offset field added for uAPI v2.
> >
> > Kent Gibson (2):
> >   tests: create mismatch between chip and bulk offsets in wait_multiple
> >   tests: add check of event offset to wait_multiple
> >
> >  tests/tests-event.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > --
> > 2.28.0
> >
> 
> I suppose that, if we don't want to do the backward compatible port
> for now, these can be ignored?
> 

It wouldn't hurt to have them either way - in my book it never hurts
to increase test coverage.

Cheers,
Kent.

Re: [libgpiod][PATCH 0/2] tests: improve wait_multiple coverage

[Bartosz Golaszewski]

On Wed, Oct 14, 2020 at 10:37 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Oct 14, 2020 at 09:50:08AM +0200, Bartosz Golaszewski wrote:
> > On Wed, Oct 14, 2020 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > A couple of patches for the v2 branch that improve the coverage of the
> > > wait_multiple test case.
> > >
> > > The first creates a mismatch between the chip offsets and bulk offsets
> > > to highlight the problem with my initial implementation of
> > > gpiod_line_event_wait_bulk() for uAPI v2.
> > >
> > > The second adds a check on the event.offset field added for uAPI v2.
> > >
> > > Kent Gibson (2):
> > >   tests: create mismatch between chip and bulk offsets in wait_multiple
> > >   tests: add check of event offset to wait_multiple
> > >
> > >  tests/tests-event.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > --
> > > 2.28.0
> > >
> >
> > I suppose that, if we don't want to do the backward compatible port
> > for now, these can be ignored?
> >
>
> It wouldn't hurt to have them either way - in my book it never hurts
> to increase test coverage.
>
> Cheers,
> Kent.

Ok I thought they only apply to your v2 port but I see they'll work in
master too.

Bartosz

Re: [libgpiod][PATCH 1/2] tests: create mismatch between chip and bulk offsets in wait_multiple

[Bartosz Golaszewski]

On Wed, Oct 14, 2020 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Create a mismatch between the chip offsets and the bulk offsets to ensure
> the implementation is not dependent on them matching.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  tests/tests-event.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/tests-event.c b/tests/tests-event.c
> index 6066e45..a43f865 100644
> --- a/tests/tests-event.c
> +++ b/tests/tests-event.c
> @@ -546,7 +546,7 @@ GPIOD_TEST_CASE(wait_multiple, 0, { 8 })
>         gpiod_test_return_if_failed();
>
>         gpiod_line_bulk_init(&bulk);
> -       for (i = 0; i < 8; i++) {
> +       for (i = 1; i < 8; i++) {
>                 line = gpiod_chip_get_line(chip, i);
>                 g_assert_nonnull(line);
>                 gpiod_test_return_if_failed();
> --
> 2.28.0
>

Applied, thanks!

Bartosz

Re: [libgpiod][PATCH 0/2] tests: improve wait_multiple coverage

[Kent Gibson]

On Wed, Oct 14, 2020 at 10:39:49AM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 14, 2020 at 10:37 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 09:50:08AM +0200, Bartosz Golaszewski wrote:
> > > On Wed, Oct 14, 2020 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > A couple of patches for the v2 branch that improve the coverage of the
> > > > wait_multiple test case.
> > > >
> > > > The first creates a mismatch between the chip offsets and bulk offsets
> > > > to highlight the problem with my initial implementation of
> > > > gpiod_line_event_wait_bulk() for uAPI v2.
> > > >
> > > > The second adds a check on the event.offset field added for uAPI v2.
> > > >
> > > > Kent Gibson (2):
> > > >   tests: create mismatch between chip and bulk offsets in wait_multiple
> > > >   tests: add check of event offset to wait_multiple
> > > >
> > > >  tests/tests-event.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > --
> > > > 2.28.0
> > > >
> > >
> > > I suppose that, if we don't want to do the backward compatible port
> > > for now, these can be ignored?
> > >
> >
> > It wouldn't hurt to have them either way - in my book it never hurts
> > to increase test coverage.
> >
> > Cheers,
> > Kent.
> 
> Ok I thought they only apply to your v2 port but I see they'll work in
> master too.
> 

Yes and No, i.e. the first is general, the second is requires the offset
field in the event, and so only works for a uAPI v2 port.

Kent.

Re: [libgpiod][PATCH 2/2] tests: add check of event offset to wait_multiple

[Bartosz Golaszewski]

On Wed, Oct 14, 2020 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> The offset field is added for uAPI v2, so extend the test to check that
> the value returned is correct.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  tests/tests-event.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/tests-event.c b/tests/tests-event.c
> index a43f865..0e5effe 100644
> --- a/tests/tests-event.c
> +++ b/tests/tests-event.c
> @@ -570,6 +570,7 @@ GPIOD_TEST_CASE(wait_multiple, 0, { 8 })
>         ret = gpiod_line_event_read(line, &ev);
>         g_assert_cmpint(ret, ==, 0);
>         g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_RISING_EDGE);
> +       g_assert_cmpint(ev.offset, ==, 4);
>  }
>
>  GPIOD_TEST_CASE(get_fd_when_values_requested, 0, { 8 })
> --
> 2.28.0
>

This one relies on your v2 port but since it breaks tests, I can't
apply it to master and same for this patch. We'll have to revisit this
once we figure out the new API for the library.

Bartosz

Re: [libgpiod][PATCH 2/2] tests: add check of event offset to wait_multiple

[Bartosz Golaszewski]

On Wed, Oct 14, 2020 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> The offset field is added for uAPI v2, so extend the test to check that
> the value returned is correct.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  tests/tests-event.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/tests-event.c b/tests/tests-event.c
> index a43f865..0e5effe 100644
> --- a/tests/tests-event.c
> +++ b/tests/tests-event.c
> @@ -570,6 +570,7 @@ GPIOD_TEST_CASE(wait_multiple, 0, { 8 })
>         ret = gpiod_line_event_read(line, &ev);
>         g_assert_cmpint(ret, ==, 0);
>         g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_RISING_EDGE);
> +       g_assert_cmpint(ev.offset, ==, 4);
>  }
>
>  GPIOD_TEST_CASE(get_fd_when_values_requested, 0, { 8 })
> --
> 2.28.0
>

Now applied to branch for-linux-v5.10-rc1. Thanks!

Bartosz