Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 27 Aug 2021 23:26:18 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Rae Moar <rmoar@google.com>
Cc:     brendanhiggins@google.com, davidgow@google.com,
        dlatypov@google.com, shuah@kernel.org, kunit-dev@googlegroups.com,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] selftests: tool: Add subtest header line and change
 indentation format in TAP results
Message-ID: <202108272314.403830F980@keescook>
References: <20210827225812.3247919-1-rmoar@google.com>
 <20210827225812.3247919-2-rmoar@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210827225812.3247919-2-rmoar@google.com>
Precedence: bulk

On Fri, Aug 27, 2021 at 10:58:11PM +0000, Rae Moar wrote:
> This patch is part of a series to alter the format of kselftest TAP
> results to improve compatibility with proposed KTAP specification
> (https://lore.kernel.org/linux-kselftest/CA+GJov6tdjvY9x12JsJT14qn6c7NViJxqaJk+r-K1YJzPggFDQ@mail.gmail.com/).
> 
> Two changes:
> - Change from "# " to "  " for indentation of nested tests
> - Add subtest header line at start of tests with subtests. Line format
> is "# Subtest: [name of test]".
> 
> An example of the new format:
> 
> Old format:
> 
>  TAP version 13
>  1..1
>  # TAP version 13
>  # 1..1
>  # # Starting 1 tests from 1 test cases.
>  # #  RUN           global.get_syscall_info ...
>  # #            OK  global.get_syscall_info
>  # ok 1 global.get_syscall_info
>  # # PASSED: 1 / 1 tests passed.
>  # # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
>  ok 1 selftests: ptrace: get_syscall_info
> 
> New format:
> 
> TAP version 13
> 1..1
>   # Subtest: selftests: ptrace: get_syscall_info
>   TAP version 13
>   1..1
>   # Starting 1 tests from 1 test cases.
>   #  RUN           global.get_syscall_info ...
>   #            OK  global.get_syscall_info
>   ok 1 global.get_syscall_info
>   # PASSED: 1 / 1 tests passed.
>   # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
> ok 1 selftests: ptrace: get_syscall_info
> 
> Signed-off-by: Rae Moar <rmoar@google.com>
> Change-Id: I139774745310ad2cd6dc5d7740254e48d8226241
> ---
>  tools/testing/selftests/kselftest/prefix.pl | 2 +-
>  tools/testing/selftests/kselftest/runner.sh | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest/prefix.pl b/tools/testing/selftests/kselftest/prefix.pl
> index 12a7f4ca2684..e59374b62603 100755
> --- a/tools/testing/selftests/kselftest/prefix.pl
> +++ b/tools/testing/selftests/kselftest/prefix.pl
> @@ -16,7 +16,7 @@ while (1) {
>  	my $bytes = sysread(STDIN, $char, 1);
>  	exit 0 if ($bytes == 0);
>  	if ($needed) {
> -		print "# ";
> +		print "  ";
>  		$needed = 0;
>  	}
>  	print $char;
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index cc9c846585f0..9b04aeb26d3a 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -23,7 +23,7 @@ fi
>  tap_prefix()
>  {
>  	if [ ! -x /usr/bin/perl ]; then
> -		sed -e 's/^/# /'
> +		sed -e 's/^/  /'
>  	else
>  		"$BASE_DIR"/kselftest/prefix.pl
>  	fi

Some of the existing TAP parsers expect leading spaces to be parsed as
YAML (from the _preceeding_ test result). "#" is understood to be
diagnostics about the currently running test.

Since there's no "---" nor "..." here, and the other parsers were pretty
busted to try to parse YAML without those, I can be convinced that
diagnostics can be marked with a leading " ", but if that's true, why
not just drop all "#" usage? Then we'd have:

TAP version 13
1..1
  # Subtest: selftests: ptrace: get_syscall_info
  TAP version 13
  1..1
    Starting 1 tests from 1 test cases.
     RUN           global.get_syscall_info ...
               OK  global.get_syscall_info
  ok 1 global.get_syscall_info
    PASSED: 1 / 1 tests passed.
    Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
ok 1 selftests: ptrace: get_syscall_info

> @@ -75,7 +75,8 @@ run_one()
>  		echo "not ok $test_num $TEST_HDR_MSG"
>  	else
>  		cd `dirname $TEST` > /dev/null
> -		((((( tap_timeout ./$BASENAME_TEST 2>&1; echo $? >&3) |
> +		(echo "  # Subtest: selftests: $DIR: $BASENAME_TEST" &&
> +		(((( tap_timeout ./$BASENAME_TEST 2>&1; echo $? >&3) |

What's the benefit here? All I see is that now the test and the runner
are once again mixing the responsibility of generating the test output.
The subtest should be able to run strictly independently.

>  			tap_prefix >&4) 3>&1) |
>  			(read xs; exit $xs)) 4>>"$logfile" &&
>  		echo "ok $test_num $TEST_HDR_MSG") ||
> @@ -83,7 +84,6 @@ run_one()
>  		if [ $rc -eq $skip_rc ]; then	\
>  			echo "ok $test_num $TEST_HDR_MSG # SKIP"
>  		elif [ $rc -eq $timeout_rc ]; then \
> -			echo "#"

NAK: this is ensuring a newline before the "not ok" line on a timeout,
where the test output may have been interrupted before printing a
newline. (i.e. unflushed progress report style output)

>  			echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT $kselftest_timeout seconds"
>  		else
>  			echo "not ok $test_num $TEST_HDR_MSG # exit=$rc"

-Kees

-- 
Kees Cook