Re: [libgpiod][RFC/RFT 02/18] tests: split out the common test code for bash scripts

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Apr 23, 2024 at 8:17 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Apr 12, 2024 at 02:27:48PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > In order to allow the upcoming DBus command-line client tests to reuse the
> > existing bash test harness, let's put the common code into an importable
> > file and rename run_tool to run_prog to reflect that it now can run any
> > program.
>
> >  tests/bash/Makefile.am                |   4 +
> >  tests/bash/gpiod-bash-test-helper.inc | 328 +++++++++++++++
>
> Is the 'bash' a new folder name?
>
> If so, it might be problematic for make as it may recognize it as a bash
> command. I would rather name it 'scripts' or 'shell' or alike.
>

Good point, 'scripts' works fine.

For the rest: it's out of scope for this series - you reviewed code
that has existed for a while - and needs to be addressed separately.

> ...
>
> > +++ b/tests/bash/gpiod-bash-test-helper.inc
>
> And why not a simple shell? Do you use bashisms?
>

Yes, quite a lot actually.

> ...
>
> > +# Run the command in $* and fail the test if the command succeeds.
>
> "$*" in most cases is simply wrong. "$@" is better.
>
> https://unix.stackexchange.com/questions/294052/how-to-properly-pass-set-of-parameters-with-spaces-into-shell-function
>

I'll change this.

> ...
>
> > +num_lines_is() {
> > +     [ "$1" -eq "0" ] || [ -z "$output" ] && return 0
>
> -o inside [] will do better than ||
>
> > +     local NUM_LINES=$(echo "$output" | wc -l)
> > +     assertEquals " number of lines:" "$1" "$NUM_LINES"
> > +}
>
> ...
>
> > +     for ARG in $*
>
> This may be wrong.
>
> > +     do
>
> > +     done
>
> ...
>
> > +gpiosim_chip_symlink_cleanup() {
> > +     if [ -n "$GPIOSIM_CHIP_LINK" ]
> > +     then
> > +             rm "$GPIOSIM_CHIP_LINK"
> > +     fi
>
> If you use already || or && you may continue doing that, this will be one liner
> with it.
>
>         [ -n "$GPIOSIM_CHIP_LINK" ] && rm "$GPIOSIM_CHIP_LINK"
>
> > +     unset GPIOSIM_CHIP_LINK
> > +}
>
> ...
>
> > +     for i in {1..30}; do
>
> $(seq 1 30) or IIRC $(seq 30)
>
> Less bashisms are better.
>

Disagree. We know we'll keep on using bash here in tests. No issue
with using it here.

> > +             [ "$(<$PORT)" = "$EXPECTED" ] && return
> > +             sleep 0.01
> > +     done
>
> ...
>
> > +             if [ "$?" = "0" ]
> > +             then
> > +                     for LINE in $(find $BANKPATH/ | grep -E "line[0-9]+$")
>
> What's the point in -E ?
>
> Yet another `Useless use of grep`.
>

Right, we only use grep in one place. I think we used to depend on it
more in the past. I'll drop it.

> IIRC how `find` works. You may filter names there (and with regex IIRC again).
>
> > +                     do
> > +                             test -e $LINE/hog && rmdir $LINE/hog
>
> Why not [ -e ... ] && ?
>
> > +                             rmdir $LINE
> > +                     done
> > +             fi
>
> ...
>
> > +dut_read_redirect() {
> > +     output=$(<$SHUNIT_TMPDIR/$DUT_OUTPUT)
> > +        local ORIG_IFS="$IFS"
> > +        IFS=$'\n' lines=($output)
> > +        IFS="$ORIG_IFS"
>
> TABs vs. spaces indentation issues?
>

Indeed and not only here.

> > +}
>
> ...
>
> > +dut_read() {
> > +     local LINE
> > +     lines=()
> > +     while read -t 0.2 -u ${COPROC[0]} LINE;
>
> What is ; for as you are using three-lines form?
>
> > +     do
> > +             if [ -n "$DUT_FIRST_CHAR" ]
> > +             then
> > +                     LINE=${DUT_FIRST_CHAR}${LINE}
> > +                     unset DUT_FIRST_CHAR
> > +             fi
> > +             lines+=("$LINE")
>
> Don't remember the syntax, but something like this
>
> lines=$((LINE + $lines))
>
> is better than bashisms.
>
> > +     done
> > +     output="${lines[@]}"
> > +}
>
> ...
>
> > +     read -t 0.2 -u ${COPROC[0]} LINE || (echo Timeout && false)
>
> Wouldn't () fork a shell?
> (The () vs. {} discussion, don't remember by heart though.)
>
> ...
>
> > +dut_write() {
> > +     echo $* >&${COPROC[1]}
>
> Oh.
>
> > +}
>
> ...
>
> > +# Must be done after we sources shunit2 as we need SHUNIT_VERSION to be set.
> > +oneTimeSetUp() {
> > +     test "$SHUNIT_VERSION" = "$MIN_SHUNIT_VERSION" && return 0
> > +     local FIRST=$(printf "$SHUNIT_VERSION\n$MIN_SHUNIT_VERSION\n" | sort -Vr | head -1)
>
> I believe you can do it with a simple test rather than involving sort and head
> and printf. And test most likely will be builtin.
>
> > +     test "$FIRST" = "$MIN_SHUNIT_VERSION" && \
> > +             die "minimum shunit version required is $MIN_SHUNIT_VERSION (current version is $SHUNIT_VERSION"
> > +}
>
> ...
>
> > +     SORTED=$(printf "$REQUIRED\n$CURRENT" | sort -V | head -n 1)
>
> Ditto.
>
> > +     if [ "$SORTED" != "$REQUIRED" ]
>
> Dup? Seems to me you can run a single test to get the answer.
>
> > +     then
> > +             die "linux kernel version must be at least: v$REQUIRED - got: v$CURRENT"
>
> Linux
>
> > +     fi
>
> ...
>
> Okay, it seems you moved existing code... That code needs more love towards
> becoming a nicely formatted shell.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

I will send a series addressing some of the issues. Others I choose to
ignore for now as they are a matter of personal taste and Kent is the
author of most of the bash code here.

Bart