[PATCH] convert bin/date over to ATF

[Garrett Cooper]

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

> On Jan 20, 2014, at 13:40, Garrett Cooper <yaneurabeya at gmail.com> wrote:
> 
>> 	This is based on work done by Giorgos a couple years ago.
>> Thanks!
>> -Garrett
> 
> Pasting patch contents and commenting inline:
> 
>> diff --git a/bin/date/tests/Makefile b/bin/date/tests/Makefile
>> index 540008b..459d019 100644
>> --- a/bin/date/tests/Makefile
>> +++ b/bin/date/tests/Makefile
>> @@ -4,6 +4,6 @@
>> 
>> TESTSDIR=	${TESTSBASE}/bin/date
>> 
>> -TAP_TESTS_SH=	legacy_test
>> +ATF_TESTS_SH=	regress
> 
> Tests ought to end with _test per the description in https://wiki.freebsd.org/TestSuite/Structure

Ok! Is `_tests` ok?

> Also, "regress_test" is not a very indicative name.  Will this only contain test cases for bugs to prevent regressions?

It should be “date_functional_tests”, because that’s what they’re doing.

> date_test or integration_test (due to the lack of unit tests for the code) may be better choices.

...

>> --- /dev/null
>> +++ b/bin/date/tests/regress.sh
>> @@ -0,0 +1,557 @@
>> +#!/bin/sh
> 
> Remove.  This comes from atf.test.mk automatically.
> 
>> +
>> +#
>> +# Regression tests for date(1)
>> +#
>> +# Submitted by Edwin Groothuis <edwin at FreeBSD.org>
>> +#
>> +# $FreeBSD: src/tools/regression/bin/date/regress.sh,v 1.4 2011/01/09 22:05:09 keramida Exp $
>> +#
>> +
>> +#
>> +# These two date/times have been chosen carefully, they
>> +# create both the single digit and double/multidigit version of
>> +# the values.
>> +#
>> +# To create a new one, make sure you are using the UTC timezone!
>> +#
>> +
>> +TEST1=3222243		# 1970-02-07 07:04:03
>> +TEST2=1005600000	# 2001-11-12 21:11:12
>> +
>> +export LC_ALL=C
>> +export TZ=UTC
> 
> Kyua does this already as part of the contract between atf and the runtime engine.  You should not be resetting these.

Ok. This was code originally written with ATF in mind, but I totally agree.

>> +
>> +check()
>> +{
>> +	S=$1
>> +	A1=$2
>> +	A2=$3
> 
> S, A1, A2?  What do these mean?

Not sure… giorgos wrote these (but I assume it’s format_string, string, string_post_manipulation based on the comments…).

> Also, make local.

Totally agree (will do)!

>> +
>> +	# If the second sample text for formatted output has not been
>> +	# passed, assume it should match exactly the first one.
>> +	if [ -z "${A2}" ]; then
>> +		A2=${A1}
>> +	fi
>> +
>> +	R=`date -r ${TEST1} +%${S}`
> 
> Prefer $() over ``.

Me too (will change).

>> +	atf_check test "${R}" = "${A1}"
>> +
>> +	R=`date -r ${TEST2} +%${S}`
>> +	atf_check test "${R}" = "${A2}"
>> +}
>> +
>> +# ----------------------------------------------------------------------
>> +
>> +atf_test_case A
>> +A_head()
>> +{
>> +	atf_set "descr" "Verifies that 'A' formatting spec works"
> 
> These test case names are truly non-descriptive.  I'd recommend renaming them to something like formatting_spec__A, etc. and omitting the definition of head() altogether.  Will result on a much shorter test program, and being concise here for readability matters a lot.
> 
> ... and then you can just create a "macro" to define test cases.  Like this, but untested:
> 
> formatting_spec_test_case() {
>    local subname="${1}"; shift
>    local name="formatting_spec__${subname}"
> 
>    atf_test_case "${name}"
>    eval "${name}_body() { check ${*} }"
> }
> 
> formatting_spec_test_case a a Sat Mon
> formatting_spec_test_case B B February November
> ...
> formatting_spec_test_case plus + "Sat Feb  7 07:04:03 UTC 1970" "Mon Nov 12 21:20:00 UTC 2001”

Yeah. I’ll think about that further and get back to you after I figure out how the code works a bit more (this was just a straight carryover from giorgos).

…

Thanks!
-Garrett