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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Jun 20, 2023 at 3:43 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Tue, Jun 20, 2023 at 02:41:30PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > - update docs
> > - remove all bats-specific workarounds
>
> Not sure you got them all - more below.
>
> >  # SPDX-License-Identifier: GPL-2.0-or-later
> >  # SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@xxxxxxxxx>
> >  # SPDX-FileCopyrightText: 2022 Kent Gibson <warthog618@xxxxxxxxx>
> > +# SPDX-FileCopyrightText: 2023 Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
>
> Just extend your previous?
> Or is the email address change significant?
>

I'm doing it on linaro payroll in 2023 so let's give them credit. :)

> >  # mappings from local name to system chip name, path, dev name
> > -# -g required for the associative arrays, cos BATS...
> > +# -g required for the associative arrays.
> >  declare -g -A GPIOSIM_CHIP_NAME
> >  declare -g -A GPIOSIM_CHIP_PATH
> >  declare -g -A GPIOSIM_DEV_NAME
>
> As per the comment, the -g was there to satisfy bats, so can be removed
> nowi - both the comment and the -g option.
>

Got it.

> >  gpiosim_chip() {
> > @@ -110,7 +101,7 @@ gpiosim_chip_number() {
> >  }
> >
> >  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.

> > @@ -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 can send it in a separate patch alright.

>
> > -@test "gpiomon: with idle-timeout" {
> > +test_gpiomon_with_idle_timeout() {
> >       gpiosim_chip sim0 num_lines=8
> >
> >       local sim0=${GPIOSIM_CHIP_NAME[sim0]}
> > @@ -2114,10 +2109,9 @@ request_release_line() {
> >       dut_wait
> >       status_is 0
> >       dut_read_redirect
> > -     num_lines_is 0
>
>
> You have something against testing that there is no incidental output?
> That being the point of that check.
> Without it the dut_read_redirect is redundant too.
>
> Or does the new form of num_lines_is not handle the 0 case?
>

Ah I removed it because it was causing problems and forgot to
investigate it. I'll restore it and fix the issue in v2.

> > @@ -2671,11 +2664,9 @@ request_release_line() {
> >       dut_wait
> >       status_is 0
> >       dut_read_redirect
> > -
> > -     num_lines_is 0
>
> And again.
>
> > +# Check all required non-coreutils tools
> > +check_prog shunit2
>
> As noted previously, assertContains requires 2.1.8, so would be good to
> either check for that if possible, or at least document it.
> As it stands the tests will not run correctly on Debian Bullseye, which
> is still on 2.1.6 - though IIRC they do report success regardless -
> which is either nice or unfortunate, depending how you look at it.
>

Sure, I'll add a test for that.

Bart

> Cheers,
> Kent.