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

[Kent Gibson <warthog618@gmail.com>]

On Tue, Jun 20, 2023 at 02:41:30PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> - 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@gmail.com>
>  # SPDX-FileCopyrightText: 2022 Kent Gibson <warthog618@gmail.com>
> +# SPDX-FileCopyrightText: 2023 Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>  

Just extend your previous?
Or is the email address change significant?

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

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

> @@ -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?

> -@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?

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

Cheers,
Kent.