Re: [hackers][sbase][PATCH] testing: add first shell-based tests

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Wed, 26 Sep 2018 21:03:46 +0200

Hi Michael

Thanks for having a look!

[2018-09-24 12:41] Michael Forney <mforney_AT_mforney.org>
>
> Hi Silvan,
>
> On 2018-09-09, Silvan Jegen <s.jegen_AT_gmail.com> wrote:
> > We add some shell helper functions to test the expected output of sbase
> > tools. In addition to the helper functions themselves we add some tests
> > for 'dirname'.
> > ---
> > Hi
> >
> > I fixed some of the issues pointed out by Mattias and made the tests
> > runnable from the Makefile.
> >
> > Let me know if there is a chance that we can get some shell-based testing
> > like this into sbase.
>
> I do like the idea of a test suite for sbase. It is too easy to
> accidentally break something when fixing another bug.
>
> Shell-based testing seems fine to me. However, I wonder if it might be
> better to make this a separate repository (similar to musl and
> libc-test). That way, you could test any implementation of the POSIX
> utilities.

I am just worried that devs won't be running any tests if there is an
extra hurdle to jump through. If we put tests into a different repo,
do you think it's enough to mention the testing repo and its tests in
the README?

Users could just "git clone" the test repo into the root of the sbase
repo and run the tests from there, I guess.

Do you prefer tests like these in their own repo or in sbase? If you
prefer to put them somewhere else, do you have a preference or can I
just host them wherever?


> > Makefile | 6 +++++-
> > tests/dirname.test | 20 ++++++++++++++++++++
> > tests/runalltests | 5 +++++
> > tests/test-common.sh | 37 +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 67 insertions(+), 1 deletion(-)
> > create mode 100755 tests/dirname.test
> > create mode 100755 tests/runalltests
> > create mode 100644 tests/test-common.sh
> >
> > diff --git a/Makefile b/Makefile
> > index 0e421e7..9ddd873 100644
> > --- a/Makefile
> > +++ b/Makefile
> > _AT_@ -274,4 +274,8 @@ clean:
> > rm -f $(BIN) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
> > rm -f getconf.h
> >
> > -.PHONY: all install uninstall dist sbase-box sbase-box-install
> > sbase-box-uninstall clean
> > +testing:
>
> If this were to go into the sbase repository, the `testing` target
> should depend on the utilities being tested.

Makes sense.


> > + _AT_cd tests/; \
> > + ./runalltests
> > +
> > +.PHONY: all install uninstall dist sbase-box sbase-box-install
> > sbase-box-uninstall clean testing
> > diff --git a/tests/dirname.test b/tests/dirname.test
> > new file mode 100755
> > index 0000000..3bc45ca
> > --- /dev/null
> > +++ b/tests/dirname.test
> > _AT_@ -0,0 +1,20 @@
> > +#!/bin/sh
> > +
> > +. ./test-common.sh
> > +
> > +# "test name" "command to execute" "expected output"
> > +check_stdout "dirname-noarg" "../dirname" "" && \
> > +check_stderr "dirname-noarg-stderr" "../dirname" "usage: ../dirname path\n"
> > && \
> > +check_stdout "dirname-non-existing" "../dirname a b c" "" && \
> > +check_stdout "dirname-slash" "../dirname /" "/\n" && \
> > +check_stdout "dirname-dashes-slash" "../dirname -- /" "/\n" && \
> > +check_stdout "dirname-dashes-slash-a" "../dirname -- /a" "/\n" && \
> > +check_stdout "dirname-doublequotes" "../dirname \"\"" ".\n" && \
> > +check_stdout "dirname-slashes" "../dirname ///" "/\n" && \
> > +check_stdout "dirname-a/b" "../dirname a/b" "a\n" && \
> > +check_stdout "dirname-a/b/" "../dirname a/b/" "a\n" && \
> > +check_stdout "dirname-a/b//" "../dirname a/b//" "a\n" && \
> > +check_stdout "dirname-a" "../dirname a" ".\n" && \
> > +check_stdout "dirname-a/" "../dirname a/" ".\n" && \
> > +check_stdout "dirname-/a/b/c" "../dirname /a/b/c" "/a/b\n" && \
> > +check_stdout "dirname-//a/b/c" "../dirname //a/b/c" "//a/b\n"
> > diff --git a/tests/runalltests b/tests/runalltests
> > new file mode 100755
> > index 0000000..4cf7933
> > --- /dev/null
> > +++ b/tests/runalltests
> > _AT_@ -0,0 +1,5 @@
> > +#!/bin/sh
> > +
> > +for testfile in $(find -name '*.test' -type f); do
> > + $testfile
> > +done
>
> I believe this will ignore any failures except in the last test.

I was focusing on printing failed tests and didn't take the exit value
of the script into account.

Making the tests return an exit value > 0 and then do something like

ret=0
for testfile in $(find -name '*.test' -type f); do
        $testfile
        if [ $? -gt 0 ]; then
                ret=$(expr $ret + 1)
        fi
done

exit $ret

should work (untested).


> Why not `for testfile in *.test`? Do you anticipate multiple
> subdirectories of tests? If so, something like `find -name '*.test'
> -type f -exec {} ';'` would be better.

No, your $(for testfile in *.test) is better. The 'find' was just some
left-over code I forgot to adjust.


> > diff --git a/tests/test-common.sh b/tests/test-common.sh
> > new file mode 100644
> > index 0000000..e12fc78
> > --- /dev/null
> > +++ b/tests/test-common.sh
> > _AT_@ -0,0 +1,37 @@
> > +check_output() {
> > + testname=$1
> > + cmdtorun=$2
> > + expectedOutput=$3
> > + usestdout=$4
> > + expOutFile=$(mktemp)
> > + actualOutFile=$(mktemp)
>
> I don't really like the mix of camel case, all lowercase, and underscore.

I like underscore in function names but don't care too much about variable
naming styles. I agree that it should be consistent though. Which of
the styles do you prefer?


> > + ret=0
> > +
> > + printf "$expectedOutput" > $expOutFile
> > + if [ $usestdout -eq 1 ]; then
> > + eval $cmdtorun > $actualOutFile 2> /dev/null
> > + else
> > + eval $cmdtorun 2> $actualOutFile 1> /dev/null
> > + fi
> > +
> > + cmp $expOutFile $actualOutFile 2>&1 > /dev/null
> > + if [ $? -eq 1 ]; then
>
> You should probably also consider > 1, in which case there was an
> error with cmp.

True, I will print a different error in that case.


Cheers,

Silvan
Received on Wed Sep 26 2018 - 21:03:46 CEST