tools: tests: port tests to shunit2

PATCH Superseded Bartosz Golaszewski 3 participants 8 messages Jun 20, 2023 – Jun 22, 2023 GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2023-06-20 12:41 UTC #0 lore raw

PATCH 1/1 tools: tests: port tests to shunit2

From: Bartosz Golaszewski <[email protected]>

BATS has been confirmed to run much more slowly that shunit2. This is most likely caused by the way BATS evaluates each test file n+1 times where n is the number of tests[1].

Port tests to using shunit2 which executes as a regular shell script which, in addition to higher speed, allows for easier debugging as standard shell flags like -x now work.

[1] https://bats-core.readthedocs.io/en/stable/writing-tests.html

Signed-off-by Bartosz Golaszewski <[email protected]>

---
Changes since RFC:
- remove all references to bats
- check for shunit2 in configure
- update docs
- remove all bats-specific workarounds

 README                                        |   6 +-
 TODO                                          |   2 +-
 configure.ac                                  |   6 +-
 tools/Makefile.am                             |   4 +-
 tools/gpio-tools-test                         |  57 ---
 ...o-tools-test.bats => gpio-tools-test.bash} | 451 ++++++++++--------
 6 files changed, 250 insertions(+), 276 deletions(-)
 delete mode 100755 tools/gpio-tools-test
 rename tools/{gpio-tools-test.bats => gpio-tools-test.bash} (88%)

README +3 -3

index 5764eee..962dc06 100644
@@ -247,8 +247,8 @@ superuser privileges.
 The testing framework uses the GLib unit testing library so development package
 for GLib must be installed.
 
-The gpio-tools programs can be tested separately using the gpio-tools-test.bats
-script. It requires bats[1] to run and assumes that the tested executables are
+The gpio-tools programs can be tested separately using the gpio-tools-test.bash
+script. It requires shunit2[1] to run and assumes that the tested executables are
 in the same directory as the script.
 
 C++, Rust and Python bindings also include their own test-suites. All three

@@ -294,7 +294,7 @@ Those also provide examples of the expected formatting.
 Allow some time for your e-mail to propagate to the list before retrying,
 particularly if there are no e-mails in the list more recent than yours.
 
-[1] https://github.com/bats-core/bats-core
+[1] https://github.com/kward/shunit2
 [2] http://vger.kernel.org/vger-lists.html#linux-gpio
 [3] https://docs.kernel.org/process/email-clients.html
 [4] https://docs.kernel.org/process/coding-style.html

TODO +1 -1

index cbd6dbd..79a6246 100644
@@ -47,7 +47,7 @@ standardized protocol, it will take much more effort to implement it correctly.
 The existing tab completion uses libedit's readline emulation layer which
 has a few limitations, including not being able to correctly handle quoted
 line names and being disabled when stdin/stdout are not a tty (which makes
-testing with gpio-tools-test.bats using coproc problematic).
+testing with gpio-tools-test.bash using coproc problematic).
 
 One approach that could address both these problems is to bypass the readline
 emulation and use the libedit API (histedit.h) directly.

configure.ac +3 -3

index dde2fa5..4f120e5 100644
@@ -176,10 +176,10 @@ then
 
 	if test "x$with_tools" = xtrue
 	then
-		AC_CHECK_PROG([has_bats], [bats], [true], [false])
-		if test "x$has_bats" = "xfalse"
+		AC_CHECK_PROG([has_shunit2], [shunit2], [true], [false])
+		if test "x$has_shunit2" = "xfalse"
 		then
-			AC_MSG_NOTICE([bats not found - gpio-tools tests cannot be run])
+			AC_MSG_NOTICE([shunit2 not found - gpio-tools tests cannot be run])
 		fi
 	fi
 fi

tools/Makefile.am +1 -3

index bdad8f7..92a819f 100644
@@ -30,10 +30,8 @@ gpiomon_SOURCES = gpiomon.c
 
 gpionotify_SOURCES = gpionotify.c
 
-EXTRA_DIST = gpio-tools-test gpio-tools-test.bats
-
 if WITH_TESTS
 
-noinst_SCRIPTS = gpio-tools-test gpio-tools-test.bats
+noinst_SCRIPTS = gpio-tools-test.bash
 
 endif

tools/gpio-tools-test deleted +0 ~~-57~~

57 lines not on this page · raw message

tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +242 ~~-209~~

1,554 lines not on this page · raw message

Signature · 1 line

2.39.2

Kent Gibson

2023-06-20 13:43 UTC #1 lore raw

Quoted 4 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
- update docs
- remove all bats-specific workarounds

Not sure you got them all - more below.

Quoted 5 lines

# SPDX-License-Identifier: GPL-2.0-or-later
# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> # SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
+# SPDX-FileCopyrightText: 2023 Bartosz Golaszewski <[email protected]>

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

tools/gpio-tools-test.bash line 21

Quoted 6 lines

 # mappings from local name to system chip name, path, dev name

tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +1 -1

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

tools/gpio-tools-test.bash gpiosim_chip_number() { · line 106

Quoted 9 lines

gpiosim_chip() {

tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +1 -1

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

tools/gpio-tools-test.bash request_release_line() { · line 2073

Quoted 12 lines

tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +4 -0

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

tools/gpio-tools-test.bash request_release_line() { · line 2117

Quoted 10 lines

tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +1 -1
-@test "gpiomon: with idle-timeout" {
+test_gpiomon_with_idle_timeout() {
 	gpiosim_chip sim0 num_lines=8
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +0 -1
 	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?

tools/gpio-tools-test.bash request_release_line() { · line 2675

Quoted 6 lines

tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +0 -2
@@ -2671,11 +2664,9 @@ request_release_line() {
 	dut_wait
 	status_is 0
 	dut_read_redirect
-
-	num_lines_is 0

And again.

tools/gpio-tools-test.bash request_release_line() { · line 3064

Quoted 2 lines

tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +2 -0
+# 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.

Bartosz Golaszewski

author

2023-06-20 15:19 UTC #2 lore raw

Quoted 9 lines · Kent Gibson

Quoted 4 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
- update docs
- remove all bats-specific workarounds
Not sure you got them all - more below.

Quoted 9 lines

Quoted 5 lines
# SPDX-License-Identifier: GPL-2.0-or-later
# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> # SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
+# SPDX-FileCopyrightText: 2023 Bartosz Golaszewski <[email protected]>
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. :)

Quoted 10 lines

tools/gpio-tools-test.bash line 21
Quoted 6 lines
 # mappings from local name to system chip name, path, dev name
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +1 -1
-# -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.

Quoted 4 lines

Quoted 4 lines
gpiosim_chip() {
(quoted diff) +0 -0
@@ -110,7 +101,7 @@ gpiosim_chip_number() {
 }
 

Quoted 10 lines

tools/gpio-tools-test.bash gpiosim_chip_number() { · line 106
Quoted 5 lines
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +1 -1
 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.

Quoted 18 lines

tools/gpio-tools-test.bash request_release_line() { · line 2073
Quoted 12 lines
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +4 -0
@@ -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?

Quoted 1 line

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.

Quoted 17 lines

tools/gpio-tools-test.bash request_release_line() { · line 2117
Quoted 11 lines
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +1 -1
-@test "gpiomon: with idle-timeout" {
+test_gpiomon_with_idle_timeout() {
      gpiosim_chip sim0 num_lines=8
      local sim0=${GPIOSIM_CHIP_NAME[sim0]}
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +0 -1
@@ -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.

Quoted 2 lines

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.

Quoted 9 lines

tools/gpio-tools-test.bash request_release_line() { · line 2675
Quoted 6 lines
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +0 -2
@@ -2671,11 +2664,9 @@ request_release_line() {
      dut_wait
      status_is 0
      dut_read_redirect
-
-     num_lines_is 0
And again.

Quoted 9 lines

tools/gpio-tools-test.bash request_release_line() { · line 3064
Quoted 2 lines
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +2 -0
+# 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

Quoted 2 lines

Cheers,
Kent.

Kent Gibson

2023-06-20 15:36 UTC #3 lore raw

Quoted 13 lines · Bartosz Golaszewski

Quoted 11 lines · Kent Gibson
tools/gpio-tools-test.bash gpiosim_chip_number() { · line 106
Quoted 6 lines
gpiosim_chip_symlink() {
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +1 -1
-     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.

Quoted 5 lines

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.

Quoted 18 lines

Quoted 18 lines
tools/gpio-tools-test.bash request_release_line() { · line 2073
Quoted 12 lines
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +4 -0
@@ -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?

Quoted 8 lines

Quoted 1 line
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.

Cheers,
Kent.

Bartosz Golaszewski

author

2023-06-20 17:28 UTC #4 lore raw

Quoted 24 lines · Kent Gibson

Quoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Kent Gibson
tools/gpio-tools-test.bash gpiosim_chip_number() { · line 106
Quoted 6 lines
gpiosim_chip_symlink() {
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +1 -1
-     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.
Quoted 5 lines
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.

Quoted 9 lines

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

Quoted 27 lines

Quoted 18 lines
Quoted 18 lines
tools/gpio-tools-test.bash request_release_line() { · line 2073
Quoted 12 lines
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +4 -0
@@ -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?
Quoted 8 lines
Quoted 1 line
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.

Quoted 5 lines

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

Quoted 2 lines

Cheers,
Kent.

Erik Schilling

2023-06-22 14:07 UTC #5 lore raw

In reply to Bartosz Golaszewski

Just took a look at this since I am looking for ways to invoke the Rust binding tests...

tools/gpio-tools-test.bash request_release_line() { · line 3076

Quoted 4 lines · Bartosz Golaszewski

tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +3 -0
+# Check all required non-coreutils tools
+check_prog shunit2
[...]
+. shunit2

Hm... How does this work for you? Under Fedora shunit2 seems to be packaged to install into /usr/share/shunit2. The installed examples of the package are sourcing that. So the check_prog check fails since there is no binary in $PATH and the . shunit2 fails since there is no shunit2 in $PWD.

Am I missing some necessary setup step?

- Erik

Bartosz Golaszewski

author

2023-06-22 14:10 UTC #6 lore raw

Quoted 16 lines · Erik Schilling

Just took a look at this since I am looking for ways to invoke the Rust binding tests...
tools/gpio-tools-test.bash request_release_line() { · line 3076
Quoted 4 lines · Bartosz Golaszewski
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +3 -0
+# Check all required non-coreutils tools
+check_prog shunit2
[...]
+. shunit2
Hm... How does this work for you? Under Fedora shunit2 seems to be packaged to install into /usr/share/shunit2. The installed examples of the package are sourcing that. So the check_prog check fails since there is no binary in $PATH and the . shunit2 fails since there is no shunit2 in $PWD.

Quoted 4 lines

Am I missing some necessary setup step?
- Erik

On ubuntu and debian it's shipped in /usr/bin/shunit2 as it should. Upstream docs suggest using it as an executable as one of two supported ways (next to sourcing) so it should definitely go into /usr/bin/. I think it's just a fedora quirk. Maybe suggest a change?

Bart

Erik Schilling

2023-06-22 14:35 UTC #7 lore raw

Quoted 20 lines · Bartosz Golaszewski

Quoted 18 lines · Erik Schilling
Just took a look at this since I am looking for ways to invoke the Rust binding tests...
tools/gpio-tools-test.bash request_release_line() { · line 3076
Quoted 4 lines · Bartosz Golaszewski
tools/gpio-tools-test.bash renamed from tools/gpio-tools-test.bats +3 -0
+# Check all required non-coreutils tools
+check_prog shunit2
[...]
+. shunit2
Hm... How does this work for you? Under Fedora shunit2 seems to be packaged to install into /usr/share/shunit2. The installed examples of the package are sourcing that. So the check_prog check fails since there is no binary in $PATH and the . shunit2 fails since there is no shunit2 in $PWD.
Am I missing some necessary setup step?

Quoted 7 lines

Quoted 2 lines
- Erik
On ubuntu and debian it's shipped in /usr/bin/shunit2 as it should. Upstream docs suggest using it as an executable as one of two supported ways (next to sourcing) so it should definitely go into /usr/bin/. I think it's just a fedora quirk. Maybe suggest a change?

Done: https://bugzilla.redhat.com/show_bug.cgi?id=2216783

- Erik