Date: Tue, 12 Nov 2013 18:02:59 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Tomi Ollila <tomi.ollila@iki.fi>
Subject: Re: [PATCH 3/3] test: implement and document NOTMUCH_TEST_QUIET
	variable usage
Message-ID: <20131112230259.GD13399@mit.edu>
References: <1384288868-23903-1-git-send-email-tomi.ollila@iki.fi>
	<1384288868-23903-3-git-send-email-tomi.ollila@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1384288868-23903-3-git-send-email-tomi.ollila@iki.fi>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Tomi Ollila on Nov 12 at 10:41 pm:
> When NOTMUCH_TEST_QUIET environment variable is set to non-null value
> messages when new test script starts and when test PASSes are disabled.
> This eases picking the cases when tests FAIL (as those are still printed).
> ---
>  test/README                                      |  8 ++++++++
>  test/basic                                       | 12 ++++++++++--
>  test/test-lib.sh                                 | 11 ++++++++++-
>  test/test.expected-output/test-quiet-verbose-no  | 20 ++++++++++++++++++++
>  test/test.expected-output/test-quiet-verbose-yes | 24 ++++++++++++++++++++++++
>  5 files changed, 72 insertions(+), 3 deletions(-)
>  create mode 100644 test/test.expected-output/test-quiet-verbose-no
>  create mode 100644 test/test.expected-output/test-quiet-verbose-yes
> 
> diff --git a/test/README b/test/README
> index d12cff2..79a9b1b 100644
> --- a/test/README
> +++ b/test/README
> @@ -76,6 +76,14 @@ the tests in one of the following ways.
>  	TEST_EMACS=my-special-emacs TEST_EMACSCLIENT=my-emacsclient ./emacs
>  	make test TEST_EMACS=my-special-emacs TEST_EMACSCLIENT=my-emacsclient
>  
> +Quiet Execution
> +---------------
> +
> +Normally, when new script starts and when test PASSes you get a message
> +printed on screen. This printing can be disabled by setting the
> +NOTMUCH_TEST_QUIET variable to a non-null value. Message on test
> +failures and skips are still printed.
> +
>  Skipping Tests
>  --------------
>  If, for any reason, you need to skip one or more tests, you can do so
> diff --git a/test/basic b/test/basic
> index 64eb7d7..3b7668b 100755
> --- a/test/basic
> +++ b/test/basic
> @@ -73,14 +73,22 @@ suppress_diff_date() {
>  	-e 's/\(.*\+\+\+ test-verbose\.4\.\output\).*/\1/'
>  }
>  
> +if [ -z "$NOTMUCH_TEST_QUIET" ]
> +then
> +	test_verbose_no=$EXPECTED/test-verbose-no
> +	test_verbose_yes=$EXPECTED/test-verbose-yes
> +else
> +	test_verbose_no=$EXPECTED/test-quiet-verbose-no
> +	test_verbose_yes=$EXPECTED/test-quiet-verbose-yes
> +fi
>  test_begin_subtest "Ensure that test output is suppressed unless the test fails"
>  output=$(cd $TEST_DIRECTORY; ./test-verbose 2>&1 | suppress_diff_date)
> -expected=$(cat $EXPECTED/test-verbose-no | suppress_diff_date)
> +expected=$(cat ${test_verbose_no} | suppress_diff_date)
>  test_expect_equal "$output" "$expected"
>  
>  test_begin_subtest "Ensure that -v does not suppress test output"
>  output=$(cd $TEST_DIRECTORY; ./test-verbose -v 2>&1 | suppress_diff_date)
> -expected=$(cat $EXPECTED/test-verbose-yes | suppress_diff_date)
> +expected=$(cat ${test_verbose_yes} | suppress_diff_date)
>  # Do not include the results of test-verbose in totals
>  rm $TEST_DIRECTORY/test-results/test-verbose
>  rm -r $TEST_DIRECTORY/tmp.test-verbose
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index e022e46..4b342ac 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -195,7 +195,10 @@ print_test_description ()
>  	echo
>  	echo $this_test: "Testing ${test_description}"
>  }
> -print_test_description
> +if [ -z "$NOTMUCH_TEST_QUIET" ]
> +then
> +	print_test_description
> +fi
>  
>  exec 5>&1
>  
> @@ -703,6 +706,9 @@ test_ok_ () {
>  		return
>  	fi
>  	test_success=$(($test_success + 1))
> +	if test -n "$NOTMUCH_TEST_QUIET"; then
> +		return 0
> +	fi
>  	say_color pass "%-6s" "PASS"
>  	echo " $test_subtest_name"
>  }
> @@ -713,6 +719,9 @@ test_failure_ () {
>  		return
>  	fi
>  	test_failure=$(($test_failure + 1))
> +	if test -n "$NOTMUCH_TEST_QUIET"; then
> +		print_test_description

This prints the test description for *every* failing test.  Was that
intentional?  I would think that, ideally, it would be only printed
before the first failing subtest in a test (maybe by setting a
variable in print_test_description on the first call and making it
return immediately if this variable is set?  Then you wouldn't even
need the condition here, just the call to print_test_description.)

> +	fi
>  	test_failure_message_ "FAIL" "$test_subtest_name" "$@"
>  	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
>  	return 1

Stylistic nit: The three if's above use two different styles ([ vs
test and hanging 'then').  OTOH, maybe this is consistent with
test-lib's inconsistent style.

> diff --git a/test/test.expected-output/test-quiet-verbose-no b/test/test.expected-output/test-quiet-verbose-no
> new file mode 100644
> index 0000000..74840b9
> --- /dev/null
> +++ b/test/test.expected-output/test-quiet-verbose-no
> @@ -0,0 +1,20 @@
> +
> +test-verbose: Testing the verbosity options of the test framework itself.
> + FAIL   print something in test_expect_success and fail
> +	
> +	  echo "hello stdout" &&
> +	  echo "hello stderr" >&2 &&
> +	  false
> +	
> +hello stdout
> +hello stderr
> +
> +test-verbose: Testing the verbosity options of the test framework itself.
> + FAIL   print something test_begin_subtest and test_expect_equal and fail
> +	--- test-verbose.4.expected
> +	+++ test-verbose.4.output
> +	@@ -1 +1 @@
> +	-b
> +	+a
> +hello stdout
> +hello stderr
> diff --git a/test/test.expected-output/test-quiet-verbose-yes b/test/test.expected-output/test-quiet-verbose-yes
> new file mode 100644
> index 0000000..51e759d
> --- /dev/null
> +++ b/test/test.expected-output/test-quiet-verbose-yes
> @@ -0,0 +1,24 @@
> +hello stdout
> +hello stderr
> +hello stdout
> +hello stderr
> +
> +test-verbose: Testing the verbosity options of the test framework itself.
> + FAIL   print something in test_expect_success and fail
> +	
> +	  echo "hello stdout" &&
> +	  echo "hello stderr" >&2 &&
> +	  false
> +	
> +hello stdout
> +hello stderr
> +hello stdout
> +hello stderr
> +
> +test-verbose: Testing the verbosity options of the test framework itself.
> + FAIL   print something test_begin_subtest and test_expect_equal and fail
> +	--- test-verbose.4.expected
> +	+++ test-verbose.4.output
> +	@@ -1 +1 @@
> +	-b
> +	+a