Re: [libgpiod v2][PATCH v2 4/5] bindings: python: add tests for v2 API

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Jul 07, 2022 at 12:17:17PM +0200, Bartosz Golaszewski wrote:
> On Tue, Jul 5, 2022 at 4:08 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Tue, Jun 28, 2022 at 10:42:25AM +0200, Bartosz Golaszewski wrote:
> > > This adds a python wrapper around libgpiosim and a set of test cases
> > > for the v2 API using python's standard unittest module.
> > >
> > > Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> > > ---
> > >  bindings/python/tests/Makefile.am             |  14 +
> > >  bindings/python/tests/cases/__init__.py       |  12 +
> > >  bindings/python/tests/cases/tests_chip.py     | 157 +++++++
> > >  .../python/tests/cases/tests_chip_info.py     |  59 +++
> > >  .../python/tests/cases/tests_edge_event.py    | 279 +++++++++++
> > >  .../python/tests/cases/tests_info_event.py    | 135 ++++++
> > >  .../python/tests/cases/tests_line_config.py   | 254 ++++++++++
> > >  .../python/tests/cases/tests_line_info.py     |  90 ++++
> > >  .../python/tests/cases/tests_line_request.py  | 345 ++++++++++++++
> > >  bindings/python/tests/cases/tests_misc.py     |  53 +++
> > >  .../tests/cases/tests_request_config.py       |  77 ++++
> > >  bindings/python/tests/gpiod_py_test.py        |  25 +
> > >  bindings/python/tests/gpiosimmodule.c         | 434 ++++++++++++++++++
> > >  13 files changed, 1934 insertions(+)
> > >  create mode 100644 bindings/python/tests/Makefile.am
> > >  create mode 100644 bindings/python/tests/cases/__init__.py
> > >  create mode 100644 bindings/python/tests/cases/tests_chip.py
> > >  create mode 100644 bindings/python/tests/cases/tests_chip_info.py
> > >  create mode 100644 bindings/python/tests/cases/tests_edge_event.py
> > >  create mode 100644 bindings/python/tests/cases/tests_info_event.py
> > >  create mode 100644 bindings/python/tests/cases/tests_line_config.py
> > >  create mode 100644 bindings/python/tests/cases/tests_line_info.py
> > >  create mode 100644 bindings/python/tests/cases/tests_line_request.py
> > >  create mode 100644 bindings/python/tests/cases/tests_misc.py
> > >  create mode 100644 bindings/python/tests/cases/tests_request_config.py
> > >  create mode 100755 bindings/python/tests/gpiod_py_test.py
> > >  create mode 100644 bindings/python/tests/gpiosimmodule.c
> > >
> > > diff --git a/bindings/python/tests/Makefile.am b/bindings/python/tests/Makefile.am
> > > new file mode 100644
> > > index 0000000..099574f
> > > --- /dev/null
> > > +++ b/bindings/python/tests/Makefile.am
> > > @@ -0,0 +1,14 @@
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@xxxxxxxxx>
> > > +
> >
> > It is 2022?
> >
> > Which email address are you going with?  gmail here and bgdev below.
> >
> 
> These patches will be squashed together anyway. When I wrote this part
> I used this email and then switched to brgl@xxxxxxxx. It's just
> copyright anyway. I can fix it up later.
> 
> [snip!]
> 
> > > +
> > > +    def test_falling_edge_event(self):
> > > +        with gpiod.request_lines(
> > > +            self.sim.dev_path,
> > > +            gpiod.RequestConfig(offsets=[6]),
> > > +            gpiod.LineConfig(edge_detection=Edge.FALLING),
> > > +        ) as req:
> > > +            buf = gpiod.EdgeEventBuffer()
> > > +            self.thread = threading.Thread(
> > > +                target=partial(self.trigger_falling_and_rising_edge, 6)
> > > +            )
> > > +            self.thread.start()
> > > +
> >
> > Benefit of the thread? (and elsewhere a background thread is used)
> > The sleeps therein are only necessary because it is run in the
> > background.
> >
> 
> Just to make it similar to real-life applications. I did the same for
> C++ and C. And no: if I triggered multiple events without any sleeps
> in between, then some of them would risk not being registered. You can
> try it for yourself with gpiosim. It happens because when the kernel
> irq_work is busy adding an interrupt, new ones get ignored.
> 

I know, and I still don't think that this is the place for that.
I'd rather see some example code do that.
If you want to add some threaded tests in then sure, do that, but the
tests do not really need it - it just makes them more complicated than
you require.

Sure, you can't issue multiple events on a single gpio-sim line without
waiting for the result, but you never need to.  You toggle a line
and check the result.  Toggle a line, check a result.
All from the main thread.

And yeah, it is a bit disconcerting that userspace can toggle the
gpio-sim line faster than the interrupt handling in the kernel can
manage.  But I can live with that.

> [nsip]
> 
> >
> > These tests should be in tests_chip.py, as they are testing the
> > Chip.request_lines() method.
> >
> 
> I would argue that there's some overlap in where the test cases should
> live. For instance - if we moved the line watching out of
> tests_info_event into tests_chip then not much would be left. I would
> leave these here as they test the general idea of requesting lines
> rather than the functionality of class LineRequest. Same for the
> module level line requests.
> 

And I would argue the reverse - that overlap is imaginary.
This is just basic discoverability.
I looked in the tests_chip.py for the tests for Chip.request_lines(), so a
method on a Chip and implemented in chip.c, and found nothing.
Putting them in tests_line_request.py because that is what they
construct is a wee bit unintuitive.

The tests in test_line_request.py will certainly need to call
Chip.request_lines(), as that is effectively the constructor, but I
would only epxect to see successful Chip.request_lines() there as part
of the test setup, not the test proper.  All the failure cases should be
in tests_chip.py, and of course some success cases as well.
But that isn't overlap.

In general the tests in tests_<blah>.py should be for the methods
implemented in <blah>.c.  In the case of InfoEvent, that might not be
much, but you get that - it is a tiny module.  Those tests being
lonely is not a good reason to move tests in from tests_chip.c.

Cheers,
Kent.