[PATCH] convert /bin/sh tests over to ATF

[Julio Merino]

On Jan 20, 2014, at 11:44, Garrett Cooper <yaneurabeya at gmail.com> wrote:

> 	The attached patch converts the /bin/sh tests using the existing framework bin/sh/tests.
> 	All of the tests pass except locale.0, and that’s due to some [currently] unknown issue with my environment — see misc/181151). I could make some of the entries more permanent, but I was trying to make the tests easier to backport by using shell globs.
> 	Also, I added some functionality so the tests expect failures on certain versions of FreeBSD due to missing functionality or conformance modifications (see the .valid_osreldate files).
> 	Thoughts? Comments?
> Thanks!
> -Garrett<convert-bin-sh-over-to-atf.patch>

Pasted your diff inline for easier commenting but trimmed stuff.

> diff --git a/bin/sh/tests/Makefile b/bin/sh/tests/Makefile
> index f6ddb8a..e1424f5 100644
> --- a/bin/sh/tests/Makefile
> +++ b/bin/sh/tests/Makefile
> @@ -4,15 +4,8 @@
>  
>  TESTSDIR=	${TESTSBASE}/bin/sh
>  
> -TAP_TESTS_SH=	legacy_test
> -TAP_TESTS_SH_SED_legacy_test=	-e 's,__SH__,/bin/sh,g'
> -# Some tests in here are silently not run when the tests are executed as
> -# root.  Explicitly tell Kyua to drop privileges.
> -#
> -# TODO(jmmv): Kyua needs to do this by default, not only when explicitly
> -# requested.  See https://code.google.com/p/kyua/issues/detail?id=6
> -TEST_METADATA.legacy_test+= required_user="unprivileged"
> +KYUAFILE=	auto

KYUAFILE=auto is the default; remove here and anywhere else.
 
> -SUBDIR+=	builtins errors execution expansion parameters parser set-e
> +ATF_TESTS_SUBDIRS+=	builtins errors execution expansion parameters parser set-e
>  
> -.include <tap.test.mk>
> +.include <atf.test.mk>
> diff --git a/bin/sh/tests/Makefile.inc b/bin/sh/tests/Makefile.inc
> new file mode 100644
> index 0000000..0a79fd8
> --- /dev/null
> +++ b/bin/sh/tests/Makefile.inc
> @@ -0,0 +1,51 @@
> +# $FreeBSD$
> +
> +CLEANFILES=	regress.sh
> +
> +KYUAFILE=	auto
> +
> +SH_TESTS!=	cd ${.CURDIR} && find -Es . -regex '.*\.[0-9]+' | sed -e 's,^\./,,g'
> +
> +SH_TESTS_FILES!=	cd ${.CURDIR} && find . \( -type f -and \! -name Makefile\* \) | \
> +		sed -e 's,^\./,,g'

This won't work well if you don't use an OBJDIR, will it?

> +
> +TESTSDIR=	${TESTSBASE}/bin/sh/${.CURDIR:T}
> +
> +FILES+=		${SH_TESTS_FILES}
> +FILESDIR=	${TESTSDIR}
> +
> +regress.sh: ../Makefile.inc ${SH_TESTS_FILES}
> +	@echo '' > ${.TARGET}

Don't write directly to ${.TARGET} if you have more than one command doing so.  Otherwise, if the target fails half-way through (or is interrupted), a second invocation of make won't properly regenerate the file.

You can either do:

@{ \
   echo ... \
   echo ... \
   ... \
} >${.TARGET}

or create a .tmp file and move "atomically" to the target at the end.

> +	@echo 'OSRELDATE=$$(sysctl -n kern.osreldate)' >> ${.TARGET}
> +	@echo ': $${SH=/bin/sh}' >> ${.TARGET}
> +	@echo 'export SH' >> ${.TARGET}
> +	@echo '' >> ${.TARGET}
> +.for test in ${SH_TESTS}
> +	@echo 'atf_test_case ${test:S/\//_/g:S/./__/g:S/-/____/g}' >> ${.TARGET}
> +	@echo '${test:S/\//_/g:S/./__/g:S/-/____/g}_body() {' >> ${.TARGET}
> +	@echo '	cd $$(atf_get_srcdir)' >> ${.TARGET}
> +.if exists(${test}.valid_osreldate)
> +	@echo -n '	[ '`cat ${.CURDIR}/${test}.valid_osreldate`' ] && ' \
> +	    >> ${.TARGET}
> +	@echo 'atf_expect_fail "expecting failure based on OS version"' \
> +	    >> ${.TARGET}
> +.endif
> +	@echo -n '	atf_check -s exit:${test:E} ' >> ${.TARGET}
> +.if exists(${test}.stderr)
> +	@echo -n '-e "file:$$(atf_get_srcdir)/${test}.stderr" ' >> ${.TARGET}
> +.endif
> +.if exists(${test}.stdout)
> +	@echo -n '-o "file:$$(atf_get_srcdir)/${test}.stdout" ' >> ${.TARGET}
> +.endif
> +	@echo '$${SH} "./${test}"' >> ${.TARGET}
> +	@echo '}' >> ${.TARGET}
> +	@echo '' >> ${.TARGET}
> +.endfor
> +	@echo '' >> ${.TARGET}
> +	@echo 'atf_init_test_cases() {' >> ${.TARGET}
> +.for test in ${SH_TESTS}
> +	@echo '	atf_add_test_case ${test:S/\//_/g:S/./__/g:S/-/____/g}' >> ${.TARGET}
> +.endfor
> +	@echo '}' >> ${.TARGET}

All this logic in a Makefile is too complex and should really not be there.

Just create a simple shell script that defines test cases dynamically based on the available files and install that instead.  It's shell after all, so you can define functions at run time with ease!  The results will be much easier to digest.

----

But now, on a more abstract level...

This approach does not fit the "model" of atf-sh -- nor the model of most unit testing libraries for that matter.  An individual test program for every test case is overkill and keeping all these tiny files around is suboptimal.  The following page has more details on this and the corresponding rationales:

    https://wiki.freebsd.org/TestSuite/Structure

I recommend to not do this in this manner.  Instead, what I'd do is the following:

First, define one test program for every directory that currently exists.  This would leave you with builtins_test errors_test execution_test expansion_test parameters_test parser_test set-e_test, all in /usr/tests/bin/sh.  There is no reason to put them in separate subdirectories.

Second, make each of these top-level test programs "auto-discover" the various data files in their corresponding subdirectories, define an individual test case for each, and run them "as is".  For example, builtins_test would inspect srcdir/builtins/*.0, dynamically define an atf_test_case, and run the test code from within it.

These two steps change the "glue code" to be atf and expose the individual test cases to kyua with minimal changes to the existing code.

And third, separately once the above works, consider merging the contents of the individual files into the main test program.  I'm not sure keeping tons of small files is worth it, but that's a separate discussion once you have got the basic atf structure in place.

This is all very similar to what was done with the ipf tests in NetBSD.  You can take a look at them here:

    http://cvsweb.netbsd.org/bsdweb.cgi/src/tests/ipf/?only_with_tag=MAIN

(Before this was done, something similar to what you did above was attempted by using awk to generate test programs... it was a nightmare to maintain.)

Hope this helps and thanks for looking into this!