Message ID | CY4PR13MB117565A7BA2A8371A3B94ED9FD200@CY4PR13MB1175.namprd13.prod.outlook.com |
---|---|
State | Superseded |
Headers | show |
Series | TPASS in new test lib | expand |
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);
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
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.
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.
> -----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
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.
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.
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
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
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.
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 --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);