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

[Garrett Cooper]

On Jan 22, 2014, at 3:36 PM, Julio Merino <julio at meroh.net> wrote:

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

…

> KYUAFILE=auto is the default; remove here and anywhere else.

Fixed — thanks :)!

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

Yeah, it probably wouldn’t work too well with ${SH_TESTS} being defined the way it is. I overlooked the point when I last updated things by continuing to use that variable...

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

Good point. I’d use the latter method (in this case) because it would be too hairy/convoluted if I tried to use inline shell logic.

...

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

All valuable input. I’ll definitely think about it and apply your comments appropriately :).

Thanks!
-Garrett