[RFC,2/2] include/tst_test_macros.h: Report TINFO when TST_EXP_FD() succeeded

In Summary output, avoid counting the double passed for one test:
-------------------------------------
./open01
tst_test.c:1261: TINFO: Timeout per run is 0h 05m 00s
open01.c:48: TPASS: open() with sticky bit returned fd 3
open01.c:59: TPASS: sticky bit is set as expected
open01.c:48: TPASS: open() with sirectory bit returned fd 3
open01.c:59: TPASS: sirectory bit is set as expected

Summary:
passed   4
failed   0
broken   0
skipped  0
warnings 0
-------------------------------------

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 include/tst_test_macros.h               | 2 +-
 lib/newlib_tests/test_macros01.c        | 3 ++-
 testcases/kernel/syscalls/open/open11.c | 2 ++
 3 files changed, 5 insertions(+), 2 deletions(-)

Hi Cyril,

I will change the TST_EXP_PASS as well if you approve the patch.

Best Regards,
Xiao Yang
On 2021/1/4 20:54, Xiao Yang wrote:
> In Summary output, avoid counting the double passed for one test:
> -------------------------------------
> ./open01
> tst_test.c:1261: TINFO: Timeout per run is 0h 05m 00s
> open01.c:48: TPASS: open() with sticky bit returned fd 3
> open01.c:59: TPASS: sticky bit is set as expected
> open01.c:48: TPASS: open() with sirectory bit returned fd 3
> open01.c:59: TPASS: sirectory bit is set as expected
>
> Summary:
> passed   4
> failed   0
> broken   0
> skipped  0
> warnings 0
> -------------------------------------
>
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  include/tst_test_macros.h               | 2 +-
>  lib/newlib_tests/test_macros01.c        | 3 ++-
>  testcases/kernel/syscalls/open/open11.c | 2 ++
>  3 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/tst_test_macros.h b/include/tst_test_macros.h
> index 3016d95c2..d1fc3cf70 100644
> --- a/include/tst_test_macros.h
> +++ b/include/tst_test_macros.h
> @@ -64,7 +64,7 @@ extern void *TST_RET_PTR;
>  			break;                                                 \
>  		}                                                              \
>                                                                                 \
> -		TST_MSGP_(TPASS, " returned fd %ld", TST_RET,                  \
> +		TST_MSGP_(TINFO, " returned fd %ld", TST_RET,                  \
>  		         #SCALL, ##__VA_ARGS__);                               \
>                                                                                 \
>  		TST_PASS = 1;                                                  \
> diff --git a/lib/newlib_tests/test_macros01.c b/lib/newlib_tests/test_macros01.c
> index 9a920f8e4..9aa3885c7 100644
> --- a/lib/newlib_tests/test_macros01.c
> +++ b/lib/newlib_tests/test_macros01.c
> @@ -30,7 +30,8 @@ static void do_test(void)
>  	TST_EXP_FD(fail_fd(), "TEST DESCRIPTION");
>  	tst_res(TINFO, "TST_PASS = %i", TST_PASS);
>  	TST_EXP_FD(pass_fd(), "%s", "TEST DESCRIPTION PARAM");
> -	tst_res(TINFO, "TST_PASS = %i", TST_PASS);
> +	if (TST_PASS)
> +		tst_res(TPASS, "TST_PASS = %i", TST_PASS);
>  	TST_EXP_FD(inval_val());
>  	tst_res(TINFO, "TST_PASS = %i", TST_PASS);
>  }
> diff --git a/testcases/kernel/syscalls/open/open11.c b/testcases/kernel/syscalls/open/open11.c
> index ded384fa8..f7ac96d90 100644
> --- a/testcases/kernel/syscalls/open/open11.c
> +++ b/testcases/kernel/syscalls/open/open11.c
> @@ -283,6 +283,8 @@ static void verify_open(unsigned int n)
>  	} else if (tc[n].err == 0) {
>  		TST_EXP_FD(open(tc[n].path, tc[n].flags, tc[n].mode),
>  		           "%s", tc[n].desc);
> +		if (TST_PASS)
> +			tst_res(TPASS, "%s", tc[n].desc);
>  	} else {
>  		TEST(open(tc[n].path, tc[n].flags, tc[n].mode));
>  		tst_res(TPASS, "%s", tc[n].desc);

Hi!
> In Summary output, avoid counting the double passed for one test:
> -------------------------------------
> ./open01
> tst_test.c:1261: TINFO: Timeout per run is 0h 05m 00s
> open01.c:48: TPASS: open() with sticky bit returned fd 3
> open01.c:59: TPASS: sticky bit is set as expected
> open01.c:48: TPASS: open() with sirectory bit returned fd 3
> open01.c:59: TPASS: sirectory bit is set as expected
> 
> Summary:
> passed   4
> failed   0
> broken   0
> skipped  0
> warnings 0
> -------------------------------------

What exactly is the problem of having TPASS generated for each open()
that produces a valid file descriptor in the open testcases?

These macros are especially tailored to generate TPASS/TFAIL messages so
that the caller does not have to. So even if having two TPASS per run()
function in open01 was a problem, which I do not think is the case, we
should just use SAFE_OPEN() there instead.

Hi!
> I will change the TST_EXP_PASS as well if you approve the patch.

Please do not. The very purpose of TST_EXP_PASS() is to produce
TFAIL or TPASS.

On 2021/1/5 21:36, Cyril Hrubis wrote:
> Hi!
>> In Summary output, avoid counting the double passed for one test:
>> -------------------------------------
>> ./open01
>> tst_test.c:1261: TINFO: Timeout per run is 0h 05m 00s
>> open01.c:48: TPASS: open() with sticky bit returned fd 3
>> open01.c:59: TPASS: sticky bit is set as expected
>> open01.c:48: TPASS: open() with sirectory bit returned fd 3
>> open01.c:59: TPASS: sirectory bit is set as expected
>>
>> Summary:
>> passed   4
>> failed   0
>> broken   0
>> skipped  0
>> warnings 0
>> -------------------------------------
> What exactly is the problem of having TPASS generated for each open()
> that produces a valid file descriptor in the open testcases?
>
> These macros are especially tailored to generate TPASS/TFAIL messages so
> that the caller does not have to. So even if having two TPASS per run()
> function in open01 was a problem, which I do not think is the case, we
> should just use SAFE_OPEN() there instead.
Hi Cyril,

I perfer to report one TPASS message when finishing one subtest instead 
of one step.
     because of two reasons:
    a) It seems clearer for user to know how many subtests were run 
sucessfully.
    b) There are too many TPASS/TFAIL messages when a testcase(e.g. 
open11) contains many subtests or multiple TST_EXP_* macros.
Could we make TST_EXP_* macros do common check and generate only TFAIL 
messages? (Make testcases report TPASS message by themself)

Best Regards,
Xiao Yang

Hi!
> > What exactly is the problem of having TPASS generated for each open()
> > that produces a valid file descriptor in the open testcases?
> >
> > These macros are especially tailored to generate TPASS/TFAIL messages so
> > that the caller does not have to. So even if having two TPASS per run()
> > function in open01 was a problem, which I do not think is the case, we
> > should just use SAFE_OPEN() there instead.
> Hi Cyril,
> 
> I perfer to report one TPASS message when finishing one subtest instead 
> of one step.
>      because of two reasons:
>     a) It seems clearer for user to know how many subtests were run 
> sucessfully.

This depends on a definition of subtest which, as far as I can tell, is
not that well defined.

If you want to make the output easier to read in the case of a test
function that is called more than once we should change the test library
to insert empty lines between the iterations or something along these
lines, so that the output would look like:

./open01
tst_test.c:1261: TINFO: Timeout per run is 0h 05m 00s

open01.c:48: TPASS: open() with sticky bit returned fd 3
open01.c:56: TPASS: sticky bit is set as expected

open01.c:48: TPASS: open() with sirectory bit returned fd 3
open01.c:56: TPASS: sirectory bit is set as expected

Summary:
passed   4
failed   0
broken   0
skipped  0
warnings 0

Which is way better than imposing any rules on how many TPASS/TFAIL
messages should be printed per iteration.

>     b) There are too many TPASS/TFAIL messages when a testcase(e.g. 
> open11) contains many subtests or multiple TST_EXP_* macros.

There are 28 testcases in open11 and we print 28 messages TPASS/TFAIL
messages, we just use different EXP macro for different testcases. I do
not think that silencing the output would make things any better.

> Could we make TST_EXP_* macros do common check and generate only TFAIL 
> messages? (Make testcases report TPASS message by themself)

The whole idea of having the TST_EXP_* macros is that they replace code
that looks like:

	if (condition)
		tst_res(TPASS, "...");
	else
		tst_res(TFAIL, "...");

And that they actually unify how these messages are formatted.

If you think that there are places where we use these macros where
SAFE_MACRO() would be better we can talk about changing these.

Hi Cyril,

Sorry for the late reply because I am busy with other things recently.

On 2021/1/5 23:03, Cyril Hrubis wrote:
> Hi!
>>> What exactly is the problem of having TPASS generated for each open()
>>> that produces a valid file descriptor in the open testcases?
>>>
>>> These macros are especially tailored to generate TPASS/TFAIL messages so
>>> that the caller does not have to. So even if having two TPASS per run()
>>> function in open01 was a problem, which I do not think is the case, we
>>> should just use SAFE_OPEN() there instead.
>> Hi Cyril,
>>
>> I perfer to report one TPASS message when finishing one subtest instead
>> of one step.
>>       because of two reasons:
>>      a) It seems clearer for user to know how many subtests were run
>> sucessfully.
> This depends on a definition of subtest which, as far as I can tell, is
> not that well defined.
>
> If you want to make the output easier to read in the case of a test
> function that is called more than once we should change the test library
> to insert empty lines between the iterations or something along these
> lines, so that the output would look like:
>
> ./open01
> tst_test.c:1261: TINFO: Timeout per run is 0h 05m 00s
>
> open01.c:48: TPASS: open() with sticky bit returned fd 3
> open01.c:56: TPASS: sticky bit is set as expected
>
> open01.c:48: TPASS: open() with sirectory bit returned fd 3
> open01.c:56: TPASS: sirectory bit is set as expected
>
> Summary:
> passed   4
> failed   0
> broken   0
> skipped  0
> warnings 0
>
> Which is way better than imposing any rules on how many TPASS/TFAIL
> messages should be printed per iteration.
>
>>      b) There are too many TPASS/TFAIL messages when a testcase(e.g.
>> open11) contains many subtests or multiple TST_EXP_* macros.
> There are 28 testcases in open11 and we print 28 messages TPASS/TFAIL
> messages, we just use different EXP macro for different testcases. I do
> not think that silencing the output would make things any better.
I gave a wrong example.  access02 may be the correct one:
-----------------------------------------------------------------
# ./access02
tst_test.c:1261: TINFO: Timeout per run is 0h 05m 00s
access02.c:62: TPASS: access(file_f, F_OK) as root passed
access02.c:141: TPASS: access(file_f, F_OK) as root behaviour is correct.
access02.c:62: TPASS: access(file_f, F_OK) as nobody passed
access02.c:141: TPASS: access(file_f, F_OK) as nobody behaviour is correct.
access02.c:62: TPASS: access(file_r, R_OK) as root passed
access02.c:141: TPASS: access(file_r, R_OK) as root behaviour is correct.
access02.c:62: TPASS: access(file_r, R_OK) as nobody passed
access02.c:141: TPASS: access(file_r, R_OK) as nobody behaviour is correct.
...

Summary:
passed   32
failed   0
broken   0
skipped  0
warnings 0
-----------------------------------------------------------------
>> Could we make TST_EXP_* macros do common check and generate only TFAIL
>> messages? (Make testcases report TPASS message by themself)
> The whole idea of having the TST_EXP_* macros is that they replace code
> that looks like:
>
> 	if (condition)
> 		tst_res(TPASS, "...");
> 	else
> 		tst_res(TFAIL, "...");
>
> And that they actually unify how these messages are formatted.
>
> If you think that there are places where we use these macros where
> SAFE_MACRO() would be better we can talk about changing these.
I thought about it again yesterday and wanted to introduce new TST_EXP_ 
SILENT_{FD,PASS} macros.
1) TST_EXP_SILENT_{PASS,FD} do common checks and don't report TPASS when 
SCALL succeeds.
2) TST_EXP_{PASS,FD} take use of TST_EXP_SILENT_{PASS,FD} and report 
TPASS when SCALL succeeds.
BTW: Make testcase use which macros by themselves.

Please see my v2 patch set for detailed implement:
http://lists.linux.it/pipermail/ltp/2021-January/020472.html
http://lists.linux.it/pipermail/ltp/2021-January/020473.html

Best Regards,
Xiao Yang