diff mbox series

TPASS in new test lib

Message ID CY4PR13MB117565A7BA2A8371A3B94ED9FD200@CY4PR13MB1175.namprd13.prod.outlook.com
State Superseded
Headers show
Series TPASS in new test lib | expand

Commit Message

Bird, Tim Sept. 15, 2020, 8:54 p.m. UTC 
> -----Original Message-----
> From: Petr Vorel <pvorel@suse.cz>
> 
...
> > P.P.S How come some tests produce TPASS and some produce just PASS?
> Legacy C API and shell API (both legacy and new) add T (i.e. TPASS), new C API
> don't add it (i.e. PASS). It's a minor detail we could fix that.

Well, Fuego's parser only checks for PASS (probably due to the inconsistency),
but personally I'd prefer if it was consistent. The string "TPASS" is much less
likely to appear in unrelated output than "PASS" is.

It looks like it comes from print_result() in ltp/lib/tst_test.c.

Here's a patch, in case there's interest in changing it:

From 151168bf384135d7c79b0c09bb95267ba1293205 Mon Sep 17 00:00:00 2001
From: Tim Bird <tim.bird@sony.com>
Date: Tue, 15 Sep 2020 14:18:37 -0600
Subject: [PATCH] tst_test: Change result strings to use T prefix

Change PASS to TPASS in the new C library.
Change other results strings to also include the "T" prefix.
This makes the new library consistent with previous LTP
output, and with the shell output.

Signed-off-by: Tim Bird <tim.bird@sony.com>
---
 lib/tst_test.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Xiao Yang Sept. 16, 2020, 4:50 a.m. UTC | #1 
On 2020/9/16 4:54, Bird, Tim wrote:
>
>> -----Original Message-----
>> From: Petr Vorel<pvorel@suse.cz>
>>
> ...
>>> P.P.S How come some tests produce TPASS and some produce just PASS?
>> Legacy C API and shell API (both legacy and new) add T (i.e. TPASS), new C API
>> don't add it (i.e. PASS). It's a minor detail we could fix that.
> Well, Fuego's parser only checks for PASS (probably due to the inconsistency),
> but personally I'd prefer if it was consistent. The string "TPASS" is much less
> likely to appear in unrelated output than "PASS" is.
>
> It looks like it comes from print_result() in ltp/lib/tst_test.c.
>
> Here's a patch, in case there's interest in changing it:
>
> > From 151168bf384135d7c79b0c09bb95267ba1293205 Mon Sep 17 00:00:00 2001
> From: Tim Bird<tim.bird@sony.com>
> Date: Tue, 15 Sep 2020 14:18:37 -0600
> Subject: [PATCH] tst_test: Change result strings to use T prefix
>
> Change PASS to TPASS in the new C library.
> Change other results strings to also include the "T" prefix.
> This makes the new library consistent with previous LTP
> output, and with the shell output.
Hi Tim, Petr

Is it better to factor out old strttype() and call it in print_result()? 
:-)

Best Regards,
Xiao Yang
> Signed-off-by: Tim Bird<tim.bird@sony.com>
> ---
>   lib/tst_test.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 175dea7..8cc76d5 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -193,22 +193,22 @@ static void print_result(const char *file, const int lineno, int ttype,
>
>   	switch (TTYPE_RESULT(ttype)) {
>   	case TPASS:
> -		res = "PASS";
> +		res = "TPASS";
>   	break;
>   	case TFAIL:
> -		res = "FAIL";
> +		res = "TFAIL";
>   	break;
>   	case TBROK:
> -		res = "BROK";
> +		res = "TBROK";
>   	break;
>   	case TCONF:
> -		res = "CONF";
> +		res = "TCONF";
>   	break;
>   	case TWARN:
> -		res = "WARN";
> +		res = "TWARN";
>   	break;
>   	case TINFO:
> -		res = "INFO";
> +		res = "TINFO";
>   	break;
>   	default:
>   		tst_brk(TBROK, "Invalid ttype value %i", ttype);
Petr Vorel Sept. 16, 2020, 7:11 a.m. UTC | #2 
Hi Tim,

> > -----Original Message-----
> > From: Petr Vorel <pvorel@suse.cz>

> ...
> > > P.P.S How come some tests produce TPASS and some produce just PASS?
> > Legacy C API and shell API (both legacy and new) add T (i.e. TPASS), new C API
> > don't add it (i.e. PASS). It's a minor detail we could fix that.

> Well, Fuego's parser only checks for PASS (probably due to the inconsistency),
> but personally I'd prefer if it was consistent. The string "TPASS" is much less
> likely to appear in unrelated output than "PASS" is.

> It looks like it comes from print_result() in ltp/lib/tst_test.c.

> Here's a patch, in case there's interest in changing it:
Please next time use git format-patch and add comments which are not supposed to
be part of the commit message below ---.

> From 151168bf384135d7c79b0c09bb95267ba1293205 Mon Sep 17 00:00:00 2001
> From: Tim Bird <tim.bird@sony.com>
> Date: Tue, 15 Sep 2020 14:18:37 -0600
> Subject: [PATCH] tst_test: Change result strings to use T prefix

> Change PASS to TPASS in the new C library.
> Change other results strings to also include the "T" prefix.
> This makes the new library consistent with previous LTP
> output, and with the shell output.


Reviewed-by: Petr Vorel <pvorel@suse.cz>

Although leaving this to Cyril (although having T prefix or not is quite
cosmetic, he had a reason to omit it).  Also there is ongoing rewrite of tests
still using legacy API to use new API, thus the inconsistency will disappear in
the long term.

IMHO: don't care that much about legacy API, but synchronize new C and shell
API.

Kind regards,
Petr
Cyril Hrubis Sept. 16, 2020, 8:43 a.m. UTC | #3 
Hi!
> Well, Fuego's parser only checks for PASS (probably due to the inconsistency),
> but personally I'd prefer if it was consistent. The string "TPASS" is much less
> likely to appear in unrelated output than "PASS" is.

The official runltp script checks for exit values from the test
processes, anything else, such as parsing test output, may not work
properly and will not work for at least subset of the tests that does
not use the test library.
Cyril Hrubis Sept. 16, 2020, 8:54 a.m. UTC | #4 
Hi!
> Although leaving this to Cyril (although having T prefix or not is quite
> cosmetic, he had a reason to omit it).  Also there is ongoing rewrite of tests
> still using legacy API to use new API, thus the inconsistency will disappear in
> the long term.
> 
> IMHO: don't care that much about legacy API, but synchronize new C and shell
> API.

I do not care that much here, but I do not think that we should expect
exact test output unless we have specified it somewhere.
Bird, Tim Sept. 16, 2020, 4:29 p.m. UTC | #5 
> -----Original Message-----
> From: Cyril Hrubis <chrubis@suse.cz>
> 
> Hi!
> > Although leaving this to Cyril (although having T prefix or not is quite
> > cosmetic, he had a reason to omit it).  Also there is ongoing rewrite of tests
> > still using legacy API to use new API, thus the inconsistency will disappear in
> > the long term.
> >
> > IMHO: don't care that much about legacy API, but synchronize new C and shell
> > API.
> 
> I do not care that much here, but I do not think that we should expect
> exact test output unless we have specified it somewhere.

That's fine.  But it's probably better to avoid differences in
test output as much as possible, when the change is so trivial.  Petr seems to think
there was a reason that the result string was changed between the old and new C library.
If so, what is the reason?  If the change is only cosmetic, then I'd argue it's worth
being consistent, and I'll submit the patch properly.  (Sorry the earlier version was
inline in the message, rather than formatted correctly.)
 -- Tim
Cyril Hrubis Sept. 17, 2020, 8:40 a.m. UTC | #6 
Hi!
> That's fine.  But it's probably better to avoid differences in
> test output as much as possible, when the change is so trivial.  Petr seems to think
> there was a reason that the result string was changed between the old and new C library.
> If so, what is the reason?  If the change is only cosmetic, then I'd argue it's worth
> being consistent, and I'll submit the patch properly.  (Sorry the earlier version was
> inline in the message, rather than formatted correctly.)

I do not think that there was a strong reason for the difference apart
from an attempt to make the messages easier to read for a human reader
and to make it easier to figure out what exactly happened. The format of
the output messages was changed quite a bit in order to do that.

And as I said I do not have a strong opinion here, if you think that
this is important I'm okay with the changes.
Richard Palethorpe Sept. 17, 2020, 10:45 a.m. UTC | #7 
Hi,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> Although leaving this to Cyril (although having T prefix or not is quite
>> cosmetic, he had a reason to omit it).  Also there is ongoing rewrite of tests
>> still using legacy API to use new API, thus the inconsistency will disappear in
>> the long term.
>> 
>> IMHO: don't care that much about legacy API, but synchronize new C and shell
>> API.
>
> I do not care that much here, but I do not think that we should expect
> exact test output unless we have specified it somewhere.
>
> -- 
> Cyril Hrubis
> chrubis@suse.cz

It would be possible to output something like TAP and allow it to be set
at compile time.

Infact it should be possible to print JSON, xUnit or whatever
snippets. We could also print the meta data for each test at the
begining which would work quite well with structured formats.
Petr Vorel Sept. 17, 2020, 11:23 a.m. UTC | #8 
Hi,

> Cyril Hrubis <chrubis@suse.cz> writes:

> > Hi!
> >> Although leaving this to Cyril (although having T prefix or not is quite
> >> cosmetic, he had a reason to omit it).  Also there is ongoing rewrite of tests
> >> still using legacy API to use new API, thus the inconsistency will disappear in
> >> the long term.

> >> IMHO: don't care that much about legacy API, but synchronize new C and shell
> >> API.

> > I do not care that much here, but I do not think that we should expect
> > exact test output unless we have specified it somewhere.

> > -- 
> > Cyril Hrubis
> > chrubis@suse.cz

> It would be possible to output something like TAP and allow it to be set
> at compile time.

> Infact it should be possible to print JSON, xUnit or whatever
> snippets. We could also print the meta data for each test at the
> begining which would work quite well with structured formats.
+1. JSON or xUnit would certainly be useful for integrating LTP into other
testing frameworks. Sure, all frameworks which embeds LTP are fine with just
grep the output for T?PASS, (T?FAIL, ...), but this way we could also pass
test metadata to frameworks.

Kind regards,
Petr
Petr Vorel Sept. 17, 2020, 11:33 a.m. UTC | #9 
Hi,

...
> It would be possible to output something like TAP and allow it to be set
> at compile time.
And use env. variable for shell API (which is always behind C API).

> Infact it should be possible to print JSON, xUnit or whatever
> snippets. We could also print the meta data for each test at the
> begining which would work quite well with structured formats.

Kind regards,
Petr
Cyril Hrubis Sept. 17, 2020, 12:49 p.m. UTC | #10 
Hi!
> It would be possible to output something like TAP and allow it to be set
> at compile time.
> 
> Infact it should be possible to print JSON, xUnit or whatever
> snippets. We could also print the meta data for each test at the
> begining which would work quite well with structured formats.

The only donwside would be that we have converted less than half of the
test to the new test library, so generating any structured format will
not fly for more than half of the testcases.
Cyril Hrubis Sept. 18, 2020, 11:45 a.m. UTC | #11 
Hi!
> And as I said I do not have a strong opinion here, if you think that
> this is important I'm okay with the changes.

If you want the change to be included in the next release please send
the patch ASAP, I would like the freeze the git at the start of the next
week.
diff mbox series

Patch

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 175dea7..8cc76d5 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -193,22 +193,22 @@  static void print_result(const char *file, const int lineno, int ttype,
 
 	switch (TTYPE_RESULT(ttype)) {
 	case TPASS:
-		res = "PASS";
+		res = "TPASS";
 	break;
 	case TFAIL:
-		res = "FAIL";
+		res = "TFAIL";
 	break;
 	case TBROK:
-		res = "BROK";
+		res = "TBROK";
 	break;
 	case TCONF:
-		res = "CONF";
+		res = "TCONF";
 	break;
 	case TWARN:
-		res = "WARN";
+		res = "TWARN";
 	break;
 	case TINFO:
-		res = "INFO";
+		res = "TINFO";
 	break;
 	default:
 		tst_brk(TBROK, "Invalid ttype value %i", ttype);