Date: Tue, 23 Apr 2024 21:17:21 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Kent Gibson <warthog618@gmail.com>,
	Erik Schilling <erik.schilling@linaro.org>,
	Phil Howard <phil@gadgetoid.com>,
	Viresh Kumar <viresh.kumar@linaro.org>, linux-gpio@vger.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [libgpiod][RFC/RFT 02/18] tests: split out the common test code
 for bash scripts
Message-ID: <Zif7McOO-1d2coCO@smile.fi.intel.com>
References: <20240412122804.109323-1-brgl@bgdev.pl>
 <20240412122804.109323-3-brgl@bgdev.pl>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240412122804.109323-3-brgl@bgdev.pl>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Fri, Apr 12, 2024 at 02:27:48PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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.

...

> +++ b/tests/bash/gpiod-bash-test-helper.inc

And why not a simple shell? Do you use bashisms?

...

> +# 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

...

> +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.

> +		[ "$(<$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`.

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?

> +}

...

> +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