linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] kselftest: runner: fix TAP output for skipped tests
@ 2020-06-10 15:44 Paolo Bonzini
  2020-06-10 16:19 ` Shuah Khan
  0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2020-06-10 15:44 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: shuah, linux-kselftest

According to the TAP specification, a skipped test must be marked as "ok"
and annotated with the SKIP directive, for example

   ok 23 # skip Insufficient flogiston pressure.
   (https://testanything.org/tap-specification.html)

Fix the runner script to match this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tools/testing/selftests/kselftest/runner.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index 676b3a8b114d..f4815cbcd60f 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -77,7 +77,7 @@ run_one()
 		echo "ok $test_num $TEST_HDR_MSG") ||
 		(rc=$?;	\
 		if [ $rc -eq $skip_rc ]; then	\
-			echo "not ok $test_num $TEST_HDR_MSG # SKIP"
+			echo "ok $test_num $TEST_HDR_MSG # SKIP"
 		elif [ $rc -eq $timeout_rc ]; then \
 			echo "#"
 			echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT"
-- 
2.26.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kselftest: runner: fix TAP output for skipped tests
  2020-06-10 15:44 [PATCH] kselftest: runner: fix TAP output for skipped tests Paolo Bonzini
@ 2020-06-10 16:19 ` Shuah Khan
  2020-06-10 17:43   ` Bird, Tim
  0 siblings, 1 reply; 6+ messages in thread
From: Shuah Khan @ 2020-06-10 16:19 UTC (permalink / raw)
  To: Paolo Bonzini, linux-kernel, kvm; +Cc: shuah, linux-kselftest, Shuah Khan

On 6/10/20 9:44 AM, Paolo Bonzini wrote:
> According to the TAP specification, a skipped test must be marked as "ok"
> and annotated with the SKIP directive, for example
> 
>     ok 23 # skip Insufficient flogiston pressure.
>     (https://testanything.org/tap-specification.html)
> 
> Fix the runner script to match this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tools/testing/selftests/kselftest/runner.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index 676b3a8b114d..f4815cbcd60f 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -77,7 +77,7 @@ run_one()
>   		echo "ok $test_num $TEST_HDR_MSG") ||
>   		(rc=$?;	\
>   		if [ $rc -eq $skip_rc ]; then	\
> -			echo "not ok $test_num $TEST_HDR_MSG # SKIP"
> +			echo "ok $test_num $TEST_HDR_MSG # SKIP"
>   		elif [ $rc -eq $timeout_rc ]; then \
>   			echo "#"
>   			echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT"
> 

Thanks. I will pull this in for Linux 5.8-rc2

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] kselftest: runner: fix TAP output for skipped tests
  2020-06-10 16:19 ` Shuah Khan
@ 2020-06-10 17:43   ` Bird, Tim
  2020-06-10 18:22     ` Paolo Bonzini
  2020-06-10 20:02     ` Shuah Khan
  0 siblings, 2 replies; 6+ messages in thread
From: Bird, Tim @ 2020-06-10 17:43 UTC (permalink / raw)
  To: Shuah Khan, Paolo Bonzini, linux-kernel, kvm; +Cc: shuah, linux-kselftest



> -----Original Message-----
> From: linux-kselftest-owner@vger.kernel.org <linux-kselftest-owner@vger.kernel.org> On Behalf Of Shuah Khan
> 
> On 6/10/20 9:44 AM, Paolo Bonzini wrote:
> > According to the TAP specification, a skipped test must be marked as "ok"
> > and annotated with the SKIP directive, for example
> >
> >     ok 23 # skip Insufficient flogiston pressure.
> >     (https://testanything.org/tap-specification.html)
> >
> > Fix the runner script to match this.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   tools/testing/selftests/kselftest/runner.sh | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> > index 676b3a8b114d..f4815cbcd60f 100644
> > --- a/tools/testing/selftests/kselftest/runner.sh
> > +++ b/tools/testing/selftests/kselftest/runner.sh
> > @@ -77,7 +77,7 @@ run_one()
> >   		echo "ok $test_num $TEST_HDR_MSG") ||
> >   		(rc=$?;	\
> >   		if [ $rc -eq $skip_rc ]; then	\
> > -			echo "not ok $test_num $TEST_HDR_MSG # SKIP"
> > +			echo "ok $test_num $TEST_HDR_MSG # SKIP"

This is a pretty big change, and might break upstream CIs that have come to
rely on kselftest's existing behavior.  I know it's going to break Fuego's parsing
of results.

kselftest has a few conventions that are different from the TAP spec, 
and a few items it does that are extensions to the TAP spec.
IMHO, the TAP spec got this one wrong, but I could be convinced
otherwise.  But I think we should discuss this among CI users of
kselftest before making the change.

I started work quite a while ago on an effort to document the
conventions used by kselftest (particularly where it deviates
from the TAP spec),  but never submitted it.

I'm going to submit what I've got as an RFC now, for discussion,
even though it's not finished.  I'll do that in a separate thread.


> >   		elif [ $rc -eq $timeout_rc ]; then \
> >   			echo "#"
> >   			echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT"
> >
> 
> Thanks. I will pull this in for Linux 5.8-rc2
Shuah - can you hold off on this until we discuss it?

Thanks,
 -- Tim


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kselftest: runner: fix TAP output for skipped tests
  2020-06-10 17:43   ` Bird, Tim
@ 2020-06-10 18:22     ` Paolo Bonzini
  2020-06-15 19:28       ` Bird, Tim
  2020-06-10 20:02     ` Shuah Khan
  1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2020-06-10 18:22 UTC (permalink / raw)
  To: Bird, Tim, Shuah Khan, linux-kernel, kvm; +Cc: shuah, linux-kselftest

On 10/06/20 19:43, Bird, Tim wrote:
>>>   		(rc=$?;	\
>>>   		if [ $rc -eq $skip_rc ]; then	\
>>> -			echo "not ok $test_num $TEST_HDR_MSG # SKIP"
>>> +			echo "ok $test_num $TEST_HDR_MSG # SKIP"
> 
> This is a pretty big change, and might break upstream CIs that have come to
> rely on kselftest's existing behavior.  I know it's going to break Fuego's parsing
> of results.

Do you have a pointer to this code?

> kselftest has a few conventions that are different from the TAP spec, 
> and a few items it does that are extensions to the TAP spec.

Yes, there are extensions to directives are not a problem and parsers
might raise an error on them.  That can be an issue, but it's a separate
one (and it's easier to ignore it as long as test pass...).

> IMHO, the TAP spec got this one wrong, but I could be convinced
> otherwise.

Here the TAP spec says that a skip starts with "ok" and has a "SKIP"
directive, and anyone can parse it to treat as it as a failure if
desirable.  But doing something else should be treated simply as a
violation of the spec, it's not a matter of "right" or "wrong".

So, if you want to use "not ok ... # SKIP", don't call it TAP.

However, I noticed now that there is another instance of "not ok.*SKIP"
in testing/selftests/kselftest.h (and also one in a comment).  So they
should all be fixed at the same time, and I'm okay with holding this patch.

Paolo

> But I think we should discuss this among CI users of
> kselftest before making the change.
> 
> I started work quite a while ago on an effort to document the
> conventions used by kselftest (particularly where it deviates
> from the TAP spec),  but never submitted it.
> 
> I'm going to submit what I've got as an RFC now, for discussion,
> even though it's not finished.  I'll do that in a separate thread.
> 
> 
>>>   		elif [ $rc -eq $timeout_rc ]; then \
>>>   			echo "#"
>>>   			echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT"
>>>
>>
>> Thanks. I will pull this in for Linux 5.8-rc2
> Shuah - can you hold off on this until we discuss it?
> 
> Thanks,
>  -- Tim
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kselftest: runner: fix TAP output for skipped tests
  2020-06-10 17:43   ` Bird, Tim
  2020-06-10 18:22     ` Paolo Bonzini
@ 2020-06-10 20:02     ` Shuah Khan
  1 sibling, 0 replies; 6+ messages in thread
From: Shuah Khan @ 2020-06-10 20:02 UTC (permalink / raw)
  To: Bird, Tim, Paolo Bonzini, linux-kernel, kvm
  Cc: shuah, linux-kselftest, Shuah Khan

On 6/10/20 11:43 AM, Bird, Tim wrote:
> 
> 
>> -----Original Message-----
>> From: linux-kselftest-owner@vger.kernel.org <linux-kselftest-owner@vger.kernel.org> On Behalf Of Shuah Khan
>>
>> On 6/10/20 9:44 AM, Paolo Bonzini wrote:
>>> According to the TAP specification, a skipped test must be marked as "ok"
>>> and annotated with the SKIP directive, for example
>>>
>>>      ok 23 # skip Insufficient flogiston pressure.
>>>      (https://testanything.org/tap-specification.html)
>>>
>>> Fix the runner script to match this.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>    tools/testing/selftests/kselftest/runner.sh | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
>>> index 676b3a8b114d..f4815cbcd60f 100644
>>> --- a/tools/testing/selftests/kselftest/runner.sh
>>> +++ b/tools/testing/selftests/kselftest/runner.sh
>>> @@ -77,7 +77,7 @@ run_one()
>>>    		echo "ok $test_num $TEST_HDR_MSG") ||
>>>    		(rc=$?;	\
>>>    		if [ $rc -eq $skip_rc ]; then	\
>>> -			echo "not ok $test_num $TEST_HDR_MSG # SKIP"
>>> +			echo "ok $test_num $TEST_HDR_MSG # SKIP"
> 
> This is a pretty big change, and might break upstream CIs that have come to
> rely on kselftest's existing behavior.  I know it's going to break Fuego's parsing
> of results.
> 

Thanks for chiming in. We don't want to break CI workflow.

> kselftest has a few conventions that are different from the TAP spec,
> and a few items it does that are extensions to the TAP spec.
> IMHO, the TAP spec got this one wrong, but I could be convinced
> otherwise.  But I think we should discuss this among CI users of
> kselftest before making the change.
> 
> I started work quite a while ago on an effort to document the
> conventions used by kselftest (particularly where it deviates
> from the TAP spec),  but never submitted it.
> 
> I'm going to submit what I've got as an RFC now, for discussion,
> even though it's not finished.  I'll do that in a separate thread.
> 
> 
>>>    		elif [ $rc -eq $timeout_rc ]; then \
>>>    			echo "#"
>>>    			echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT"
>>>
>>
>> Thanks. I will pull this in for Linux 5.8-rc2
> Shuah - can you hold off on this until we discuss it?
> 

Of course. Thanks for getting my attention before I pulled it in.

thanks,
-- Shuah


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] kselftest: runner: fix TAP output for skipped tests
  2020-06-10 18:22     ` Paolo Bonzini
@ 2020-06-15 19:28       ` Bird, Tim
  0 siblings, 0 replies; 6+ messages in thread
From: Bird, Tim @ 2020-06-15 19:28 UTC (permalink / raw)
  To: Paolo Bonzini, Shuah Khan, linux-kernel, kvm
  Cc: shuah, linux-kselftest, Kees Cook



> -----Original Message-----
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> On 10/06/20 19:43, Bird, Tim wrote:
> >>>   		(rc=$?;	\
> >>>   		if [ $rc -eq $skip_rc ]; then	\
> >>> -			echo "not ok $test_num $TEST_HDR_MSG # SKIP"
> >>> +			echo "ok $test_num $TEST_HDR_MSG # SKIP"
> >
> > This is a pretty big change, and might break upstream CIs that have come to
> > rely on kselftest's existing behavior.  I know it's going to break Fuego's parsing
> > of results.
> 
> Do you have a pointer to this code?
> 
> > kselftest has a few conventions that are different from the TAP spec,
> > and a few items it does that are extensions to the TAP spec.
> 
> Yes, there are extensions to directives are not a problem and parsers
> might raise an error on them.  That can be an issue, but it's a separate
> one (and it's easier to ignore it as long as test pass...).
> 
> > IMHO, the TAP spec got this one wrong, but I could be convinced
> > otherwise.
> 
> Here the TAP spec says that a skip starts with "ok" and has a "SKIP"
> directive, and anyone can parse it to treat as it as a failure if
> desirable.  But doing something else should be treated simply as a
> violation of the spec, it's not a matter of "right" or "wrong".
> 
> So, if you want to use "not ok ... # SKIP", don't call it TAP.

Well, for some time now I've been in favor of us calling the
kernel tests' format "KTAP".
But I agree if there are no strong reasons to be different we should
have the same convention as TAP 13.

I looked at Fuego, and it's trivial to change it's parser (and I've wanted
to consolidate the kselftest parser with other TAP parsers in Fuego,
so it's probably best for Fuego to go with the SKIP="ok" convention anyway.)
I haven't released an updated kselftest for Fuego in a while, so I
don't think many of Fuego's users will be affected if I switch.  Plus
I know most of them so I can smooth it over if it's a problem.  SKIP 
really should be treated differently than "ok" or "not ok" anyway.

I raised this issue on our automated testing conference call last week.
The call had people from the KernelCI, CKI, LKFT and Fuego projects,
so at least those projects have gotten the word and have a chance to
object if this will affect their results parsers.

> 
> However, I noticed now that there is another instance of "not ok.*SKIP"
> in testing/selftests/kselftest.h (and also one in a comment).  So they
> should all be fixed at the same time, and I'm okay with holding this patch.

Agree that we should change it everywhere once we decide.  Nice catch.

For my part, I'm OK with moving to the "SKIP=ok" convention (in agreement
with TAP13).  I don't know if we need to wait longer for others to have a chance
to review this or not, but maybe wait a few days and then go for it?
 -- Tim

> Paolo
> 
> > But I think we should discuss this among CI users of
> > kselftest before making the change.
> >
> > I started work quite a while ago on an effort to document the
> > conventions used by kselftest (particularly where it deviates
> > from the TAP spec),  but never submitted it.
> >
> > I'm going to submit what I've got as an RFC now, for discussion,
> > even though it's not finished.  I'll do that in a separate thread.
> >
> >
> >>>   		elif [ $rc -eq $timeout_rc ]; then \
> >>>   			echo "#"
> >>>   			echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT"
> >>>
> >>
> >> Thanks. I will pull this in for Linux 5.8-rc2
> > Shuah - can you hold off on this until we discuss it?
> >
> > Thanks,
> >  -- Tim
> >


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-06-15 19:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10 15:44 [PATCH] kselftest: runner: fix TAP output for skipped tests Paolo Bonzini
2020-06-10 16:19 ` Shuah Khan
2020-06-10 17:43   ` Bird, Tim
2020-06-10 18:22     ` Paolo Bonzini
2020-06-15 19:28       ` Bird, Tim
2020-06-10 20:02     ` Shuah Khan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).