Re: [libgpiod][PATCH] tools: tests: port tests to shunit2

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Jun 20, 2023 at 5:36 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Tue, Jun 20, 2023 at 05:19:27PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Jun 20, 2023 at 3:43 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > >
> > > >  gpiosim_chip_symlink() {
> > > > -     GPIOSIM_CHIP_LINK="$2/${GPIOSIM_APP_NAME}-$$-lnk"
> > > > +     GPIOSIM_CHIP_LINK="/tmp/${GPIOSIM_APP_NAME}-$$-lnk"
> > > >       ln -s ${GPIOSIM_CHIP_PATH[$1]} "$GPIOSIM_CHIP_LINK"
> > > >  }
> > >
> > > The $2 dicates where the symlink was placed.
> > > Now it is ignored and placed in /tmp regardless, which is wrong.
> > > Refer to where it is called.
> > >
> >
> > I understand the $2 was ignored here but why is it wrong to use /tmp?
> > Why would we want to create the link in .? Also: shunit defines
> > SHUNIT_TMPDIR which seems to be exposed for temporary files generated
> > by tests, I'm more inclined towards using this one.
> >
>
> The $2 is there for a reason - that is where you want the symlink
> located.
>
> "gpiodetect: all chips" puts a symlink in /dev to check that it is
> ignored.
>
> ""gpiodetect: a chip" puts one in the PWD to check that gpiodetect will
> find it.
>
> If you want to remove that parameter then review and revise  all the
> places it is used.
>

Indeed, thanks. I'll keep it as is then, as these are valid use-cases.

> > > > @@ -2072,9 +2063,13 @@ request_release_line() {
> > > >       dut_run_redirect gpiomon --num-events=4 --chip $sim0 4
> > > >
> > > >       gpiosim_set_pull sim0 4 pull-up
> > > > +     sleep 0.01
> > > >       gpiosim_set_pull sim0 4 pull-down
> > > > +     sleep 0.01
> > > >       gpiosim_set_pull sim0 4 pull-up
> > > > +     sleep 0.01
> > > >       gpiosim_set_pull sim0 4 pull-down
> > > > +     sleep 0.01
> > > >
> > >
> > > Why are delays now required between sim pulls?
> > > Might toggle the pull before it gets propagated to the cdev?
> > > Add a function that describes that rather than a raw sleep?
> > > gpiosim_wait_pull?
> > >
> > > Split that out from the shunit2 change if if is a general problem?
> >
> > Porting to shunit revealed a problem similar to the one I saw in C
> > tests - toggling the sim pull too fast would lead to losing events.
> > Turns out BATS was slow enough to hide the problem. If I run shunit
> > over strace, the problem is gone too because it slows down the
> > execution.
> >
>
> I see the same in some of my compiled unit tests, and add propagation
> delays to allow for it.  For me, the tool tests have always been too slow
> to trigger it - and still are, but it wouldn't surprise me to see it
> on a faster machine.  So no problem with adding some allowance there.
>

I'll put it into a separate patch.

Bart

> Cheers,
> Kent.