diff mbox series

[2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

Message ID 1541681733-18845-2-git-send-email-yangx.jy@cn.fujitsu.com
State Superseded
Delegated to: Petr Vorel
Headers show
Series [1/2] lib: Introduce tst_strttype() | expand

Commit Message

Xiao Yang Nov. 8, 2018, 12:55 p.m. UTC
1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is
   passed into tst_brk() and convert it to TBROK forcely.
2) Only update test result in library process and main test process
   because the exit status of child can be passed into main test
   process by check_child_status().
3) Increase the number of skipped when calling tst_brk(TCONF).
4) Increase the number of warnings when calling tst_brk(TBROK) in
   test cleanup(), other than that print "Test broken!" when calling
   tst_brk(TBROK).

Fix: #408

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 lib/tst_test.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Comments

Jan Stancek Nov. 8, 2018, 5:53 p.m. UTC | #1
----- Original Message -----
> 1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is
>    passed into tst_brk() and convert it to TBROK forcely.
> 2) Only update test result in library process and main test process
>    because the exit status of child can be passed into main test
>    process by check_child_status().
> 3) Increase the number of skipped when calling tst_brk(TCONF).
> 4) Increase the number of warnings when calling tst_brk(TBROK) in
>    test cleanup(), other than that print "Test broken!" when calling
>    tst_brk(TBROK).
> 
> Fix: #408
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 661fbbf..c8d8eff 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -55,6 +55,7 @@ struct results {
>  	int skipped;
>  	int failed;
>  	int warnings;
> +	int broken;

Hi,

I don't follow what benefit this provides. It generates message "Test broken",
but we already know that test is broken by message in tst_vbrk_() / tst_cvres().

Regards,
Jan

>  	unsigned int timeout;
>  };
>  
> @@ -159,6 +160,18 @@ void tst_reinit(void)
>  	SAFE_CLOSE(fd);
>  }
>  
> +static int tst_check_ttype(int ttype)
> +{
> +	if (TTYPE_RESULT(ttype) != TCONF && TTYPE_RESULT(ttype) != TBROK) {
> +		tst_res(TINFO, "tst_brk(): invalid type %s, use TBROK forcely",
> +			tst_strttype(ttype));
> +		ttype &= ~TTYPE_MASK;
> +		ttype |= TBROK;
> +	}
> +
> +	return ttype;
> +}
> +
>  static void update_results(int ttype)
>  {
>  	if (!results)
> @@ -177,6 +190,9 @@ static void update_results(int ttype)
>  	case TFAIL:
>  		tst_atomic_inc(&results->failed);
>  	break;
> +	case TBROK:
> +		tst_atomic_inc(&results->broken);
> +	break;
>  	}
>  }
>  
> @@ -305,11 +321,15 @@ void tst_vbrk_(const char *file, const int lineno, int
> ttype,
>  	 * specified but CLONE_THREAD is not. Use direct syscall to avoid
>  	 * cleanup running in the child.
>  	 */
> -	if (syscall(SYS_getpid) == main_pid)
> +	if (syscall(SYS_getpid) == main_pid) {
> +		update_results(ttype);
>  		do_test_cleanup();
> +	}
>  
> -	if (getpid() == lib_pid)
> +	if (getpid() == lib_pid) {
> +		update_results(ttype);
>  		do_exit(TTYPE_RESULT(ttype));
> +	}
>  
>  	exit(TTYPE_RESULT(ttype));
>  }
> @@ -330,6 +350,7 @@ void tst_brk_(const char *file, const int lineno, int
> ttype,
>  	va_list va;
>  
>  	va_start(va, fmt);
> +	ttype = tst_check_ttype(ttype);
>  	tst_brk_handler(file, lineno, ttype, fmt, va);
>  	va_end(va);
>  }
> @@ -605,6 +626,9 @@ static void do_exit(int ret)
>  			ret |= TWARN;
>  	}
>  
> +	if (results->broken)
> +		printf("Test broken!\n");
> +
>  	do_cleanup();
>  
>  	exit(ret);
> --
> 1.8.3.1
> 
> 
> 
>
Xiao Yang Nov. 9, 2018, 2:46 a.m. UTC | #2
On 2018/11/09 1:53, Jan Stancek wrote:
> ----- Original Message -----
>> 1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is
>>     passed into tst_brk() and convert it to TBROK forcely.
>> 2) Only update test result in library process and main test process
>>     because the exit status of child can be passed into main test
>>     process by check_child_status().
>> 3) Increase the number of skipped when calling tst_brk(TCONF).
>> 4) Increase the number of warnings when calling tst_brk(TBROK) in
>>     test cleanup(), other than that print "Test broken!" when calling
>>     tst_brk(TBROK).
>>
>> Fix: #408
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>> index 661fbbf..c8d8eff 100644
>> --- a/lib/tst_test.c
>> +++ b/lib/tst_test.c
>> @@ -55,6 +55,7 @@ struct results {
>>   	int skipped;
>>   	int failed;
>>   	int warnings;
>> +	int broken;
> Hi,
>
> I don't follow what benefit this provides. It generates message "Test broken",
> but we already know that test is broken by message in tst_vbrk_() / tst_cvres().
Hi Jan,

We can remove the unnecessary message "Test broken", and also apply the check for
ttype in tst_brk() written by your patch.

According to the issue #$08, we want to increase result counters when calling tst_brk().
e.g.
1) Increase the skipped counter when calling tst_brk(TCONF).
2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in
    test cleanup(), other than that increase the failed counter when
    calling tst_brk(TBROK/FAIL).

Best Regards,
Xiao Yang

> Regards,
> Jan
>
>>   	unsigned int timeout;
>>   };
>>
>> @@ -159,6 +160,18 @@ void tst_reinit(void)
>>   	SAFE_CLOSE(fd);
>>   }
>>
>> +static int tst_check_ttype(int ttype)
>> +{
>> +	if (TTYPE_RESULT(ttype) != TCONF&&  TTYPE_RESULT(ttype) != TBROK) {
>> +		tst_res(TINFO, "tst_brk(): invalid type %s, use TBROK forcely",
>> +			tst_strttype(ttype));
>> +		ttype&= ~TTYPE_MASK;
>> +		ttype |= TBROK;
>> +	}
>> +
>> +	return ttype;
>> +}
>> +
>>   static void update_results(int ttype)
>>   {
>>   	if (!results)
>> @@ -177,6 +190,9 @@ static void update_results(int ttype)
>>   	case TFAIL:
>>   		tst_atomic_inc(&results->failed);
>>   	break;
>> +	case TBROK:
>> +		tst_atomic_inc(&results->broken);
>> +	break;
>>   	}
>>   }
>>
>> @@ -305,11 +321,15 @@ void tst_vbrk_(const char *file, const int lineno, int
>> ttype,
>>   	 * specified but CLONE_THREAD is not. Use direct syscall to avoid
>>   	 * cleanup running in the child.
>>   	 */
>> -	if (syscall(SYS_getpid) == main_pid)
>> +	if (syscall(SYS_getpid) == main_pid) {
>> +		update_results(ttype);
>>   		do_test_cleanup();
>> +	}
>>
>> -	if (getpid() == lib_pid)
>> +	if (getpid() == lib_pid) {
>> +		update_results(ttype);
>>   		do_exit(TTYPE_RESULT(ttype));
>> +	}
>>
>>   	exit(TTYPE_RESULT(ttype));
>>   }
>> @@ -330,6 +350,7 @@ void tst_brk_(const char *file, const int lineno, int
>> ttype,
>>   	va_list va;
>>
>>   	va_start(va, fmt);
>> +	ttype = tst_check_ttype(ttype);
>>   	tst_brk_handler(file, lineno, ttype, fmt, va);
>>   	va_end(va);
>>   }
>> @@ -605,6 +626,9 @@ static void do_exit(int ret)
>>   			ret |= TWARN;
>>   	}
>>
>> +	if (results->broken)
>> +		printf("Test broken!\n");
>> +
>>   	do_cleanup();
>>
>>   	exit(ret);
>> --
>> 1.8.3.1
>>
>>
>>
>>
>
> .
>
Xiao Yang Nov. 9, 2018, 3:12 a.m. UTC | #3
于 2018/11/09 10:46, Xiao Yang 写道:
> On 2018/11/09 1:53, Jan Stancek wrote:
>> ----- Original Message -----
>>> 1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is
>>>     passed into tst_brk() and convert it to TBROK forcely.
>>> 2) Only update test result in library process and main test process
>>>     because the exit status of child can be passed into main test
>>>     process by check_child_status().
>>> 3) Increase the number of skipped when calling tst_brk(TCONF).
>>> 4) Increase the number of warnings when calling tst_brk(TBROK) in
>>>     test cleanup(), other than that print "Test broken!" when calling
>>>     tst_brk(TBROK).
>>>
>>> Fix: #408
>>>
>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>>> ---
>>>   lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>>> index 661fbbf..c8d8eff 100644
>>> --- a/lib/tst_test.c
>>> +++ b/lib/tst_test.c
>>> @@ -55,6 +55,7 @@ struct results {
>>>       int skipped;
>>>       int failed;
>>>       int warnings;
>>> +    int broken;
>> Hi,
>>
>> I don't follow what benefit this provides. It generates message "Test 
>> broken",
>> but we already know that test is broken by message in tst_vbrk_() / 
>> tst_cvres().
> Hi Jan,
>
> We can remove the unnecessary message "Test broken", and also apply 
> the check for
> ttype in tst_brk() written by your patch.
>
> According to the issue #$08, we want to increase result counters when 
> calling tst_brk().
Hi,

Sorry, The correct number is #408 :-(

Best Regards,
Xiao Yang
> e.g.
> 1) Increase the skipped counter when calling tst_brk(TCONF).
> 2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in
>    test cleanup(), other than that increase the failed counter when
>    calling tst_brk(TBROK/FAIL).
>
> Best Regards,
> Xiao Yang
>
>> Regards,
>> Jan
>>
>>>       unsigned int timeout;
>>>   };
>>>
>>> @@ -159,6 +160,18 @@ void tst_reinit(void)
>>>       SAFE_CLOSE(fd);
>>>   }
>>>
>>> +static int tst_check_ttype(int ttype)
>>> +{
>>> +    if (TTYPE_RESULT(ttype) != TCONF&&  TTYPE_RESULT(ttype) != 
>>> TBROK) {
>>> +        tst_res(TINFO, "tst_brk(): invalid type %s, use TBROK 
>>> forcely",
>>> +            tst_strttype(ttype));
>>> +        ttype&= ~TTYPE_MASK;
>>> +        ttype |= TBROK;
>>> +    }
>>> +
>>> +    return ttype;
>>> +}
>>> +
>>>   static void update_results(int ttype)
>>>   {
>>>       if (!results)
>>> @@ -177,6 +190,9 @@ static void update_results(int ttype)
>>>       case TFAIL:
>>>           tst_atomic_inc(&results->failed);
>>>       break;
>>> +    case TBROK:
>>> +        tst_atomic_inc(&results->broken);
>>> +    break;
>>>       }
>>>   }
>>>
>>> @@ -305,11 +321,15 @@ void tst_vbrk_(const char *file, const int 
>>> lineno, int
>>> ttype,
>>>        * specified but CLONE_THREAD is not. Use direct syscall to avoid
>>>        * cleanup running in the child.
>>>        */
>>> -    if (syscall(SYS_getpid) == main_pid)
>>> +    if (syscall(SYS_getpid) == main_pid) {
>>> +        update_results(ttype);
>>>           do_test_cleanup();
>>> +    }
>>>
>>> -    if (getpid() == lib_pid)
>>> +    if (getpid() == lib_pid) {
>>> +        update_results(ttype);
>>>           do_exit(TTYPE_RESULT(ttype));
>>> +    }
>>>
>>>       exit(TTYPE_RESULT(ttype));
>>>   }
>>> @@ -330,6 +350,7 @@ void tst_brk_(const char *file, const int 
>>> lineno, int
>>> ttype,
>>>       va_list va;
>>>
>>>       va_start(va, fmt);
>>> +    ttype = tst_check_ttype(ttype);
>>>       tst_brk_handler(file, lineno, ttype, fmt, va);
>>>       va_end(va);
>>>   }
>>> @@ -605,6 +626,9 @@ static void do_exit(int ret)
>>>               ret |= TWARN;
>>>       }
>>>
>>> +    if (results->broken)
>>> +        printf("Test broken!\n");
>>> +
>>>       do_cleanup();
>>>
>>>       exit(ret);
>>> -- 
>>> 1.8.3.1
>>>
>>>
>>>
>>>
>>
>> .
>>
>
>
>
>
Jan Stancek Nov. 9, 2018, 7:54 a.m. UTC | #4
----- Original Message -----
> On 2018/11/09 1:53, Jan Stancek wrote:
> > ----- Original Message -----
> >> 1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is
> >>     passed into tst_brk() and convert it to TBROK forcely.
> >> 2) Only update test result in library process and main test process
> >>     because the exit status of child can be passed into main test
> >>     process by check_child_status().
> >> 3) Increase the number of skipped when calling tst_brk(TCONF).
> >> 4) Increase the number of warnings when calling tst_brk(TBROK) in
> >>     test cleanup(), other than that print "Test broken!" when calling
> >>     tst_brk(TBROK).
> >>
> >> Fix: #408
> >>
> >> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> >> ---
> >>   lib/tst_test.c | 28 ++++++++++++++++++++++++++--
> >>   1 file changed, 26 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/tst_test.c b/lib/tst_test.c
> >> index 661fbbf..c8d8eff 100644
> >> --- a/lib/tst_test.c
> >> +++ b/lib/tst_test.c
> >> @@ -55,6 +55,7 @@ struct results {
> >>   	int skipped;
> >>   	int failed;
> >>   	int warnings;
> >> +	int broken;
> > Hi,
> >
> > I don't follow what benefit this provides. It generates message "Test
> > broken",
> > but we already know that test is broken by message in tst_vbrk_() /
> > tst_cvres().
> Hi Jan,
> 
> We can remove the unnecessary message "Test broken", and also apply the check
> for
> ttype in tst_brk() written by your patch.

Or maybe add "Broken: " to summary?

> 
> According to the issue #$08, we want to increase result counters when calling
> tst_brk().
> e.g.
> 1) Increase the skipped counter when calling tst_brk(TCONF).
> 2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in
>     test cleanup(), other than that increase the failed counter when
>     calling tst_brk(TBROK/FAIL).

I'd keep counters reflecting the messages. I imagine if summary says
"warnings: 1", people would be searching for 'WARN' in output.

TCONF - print CONF message, increase skipped
TFAIL - print FAIL message, increase failed
TBROK - print BROK message, increase broken

What do you think?

Regards,
Jan
Xiao Yang Nov. 9, 2018, 8:17 a.m. UTC | #5
On 2018/11/09 15:54, Jan Stancek wrote:
>
> ----- Original Message -----
>> On 2018/11/09 1:53, Jan Stancek wrote:
>>> ----- Original Message -----
>>>> 1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is
>>>>      passed into tst_brk() and convert it to TBROK forcely.
>>>> 2) Only update test result in library process and main test process
>>>>      because the exit status of child can be passed into main test
>>>>      process by check_child_status().
>>>> 3) Increase the number of skipped when calling tst_brk(TCONF).
>>>> 4) Increase the number of warnings when calling tst_brk(TBROK) in
>>>>      test cleanup(), other than that print "Test broken!" when calling
>>>>      tst_brk(TBROK).
>>>>
>>>> Fix: #408
>>>>
>>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>>>> ---
>>>>    lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>>>>    1 file changed, 26 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>>>> index 661fbbf..c8d8eff 100644
>>>> --- a/lib/tst_test.c
>>>> +++ b/lib/tst_test.c
>>>> @@ -55,6 +55,7 @@ struct results {
>>>>    	int skipped;
>>>>    	int failed;
>>>>    	int warnings;
>>>> +	int broken;
>>> Hi,
>>>
>>> I don't follow what benefit this provides. It generates message "Test
>>> broken",
>>> but we already know that test is broken by message in tst_vbrk_() /
>>> tst_cvres().
>> Hi Jan,
>>
>> We can remove the unnecessary message "Test broken", and also apply the check
>> for
>> ttype in tst_brk() written by your patch.
> Or maybe add "Broken: " to summary?
>
>> According to the issue #$08, we want to increase result counters when calling
>> tst_brk().
>> e.g.
>> 1) Increase the skipped counter when calling tst_brk(TCONF).
>> 2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in
>>      test cleanup(), other than that increase the failed counter when
>>      calling tst_brk(TBROK/FAIL).
> I'd keep counters reflecting the messages. I imagine if summary says
> "warnings: 1", people would be searching for 'WARN' in output.
>
> TCONF - print CONF message, increase skipped
> TFAIL - print FAIL message, increase failed
> TBROK - print BROK message, increase broken
>
> What do you think?
Hi Jan,

Usually, calling tst_brk() can do above behaviors as you said.

How about doing the following behaviors when calling tst_brk() in test cleanup?
-----------------------------------------------
TCONF - print CONF message, increase skipped
TFAIL - print FAIL message, increase warnings
TBROK - print BROK message, increase warnings
-----------------------------------------------

Best Reagrds,
Xiao Yang

> Regards,
> Jan
>
>
>
> .
>
Jan Stancek Nov. 9, 2018, 5:52 p.m. UTC | #6
----- Original Message -----
> On 2018/11/09 15:54, Jan Stancek wrote:
> >
> > ----- Original Message -----
> >> On 2018/11/09 1:53, Jan Stancek wrote:
> >>> ----- Original Message -----
> >>>> 1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is
> >>>>      passed into tst_brk() and convert it to TBROK forcely.
> >>>> 2) Only update test result in library process and main test process
> >>>>      because the exit status of child can be passed into main test
> >>>>      process by check_child_status().
> >>>> 3) Increase the number of skipped when calling tst_brk(TCONF).
> >>>> 4) Increase the number of warnings when calling tst_brk(TBROK) in
> >>>>      test cleanup(), other than that print "Test broken!" when calling
> >>>>      tst_brk(TBROK).
> >>>>
> >>>> Fix: #408
> >>>>
> >>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> >>>> ---
> >>>>    lib/tst_test.c | 28 ++++++++++++++++++++++++++--
> >>>>    1 file changed, 26 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/lib/tst_test.c b/lib/tst_test.c
> >>>> index 661fbbf..c8d8eff 100644
> >>>> --- a/lib/tst_test.c
> >>>> +++ b/lib/tst_test.c
> >>>> @@ -55,6 +55,7 @@ struct results {
> >>>>    	int skipped;
> >>>>    	int failed;
> >>>>    	int warnings;
> >>>> +	int broken;
> >>> Hi,
> >>>
> >>> I don't follow what benefit this provides. It generates message "Test
> >>> broken",
> >>> but we already know that test is broken by message in tst_vbrk_() /
> >>> tst_cvres().
> >> Hi Jan,
> >>
> >> We can remove the unnecessary message "Test broken", and also apply the
> >> check
> >> for
> >> ttype in tst_brk() written by your patch.
> > Or maybe add "Broken: " to summary?
> >
> >> According to the issue #$08, we want to increase result counters when
> >> calling
> >> tst_brk().
> >> e.g.
> >> 1) Increase the skipped counter when calling tst_brk(TCONF).
> >> 2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in
> >>      test cleanup(), other than that increase the failed counter when
> >>      calling tst_brk(TBROK/FAIL).
> > I'd keep counters reflecting the messages. I imagine if summary says
> > "warnings: 1", people would be searching for 'WARN' in output.
> >
> > TCONF - print CONF message, increase skipped
> > TFAIL - print FAIL message, increase failed
> > TBROK - print BROK message, increase broken
> >
> > What do you think?
> Hi Jan,
> 
> Usually, calling tst_brk() can do above behaviors as you said.
> 
> How about doing the following behaviors when calling tst_brk() in test
> cleanup?
> -----------------------------------------------
> TCONF - print CONF message, increase skipped
> TFAIL - print FAIL message, increase warnings
> TBROK - print BROK message, increase warnings
> -----------------------------------------------

I don't think this matches your v2 patch. In your v2, we would
convert FAIL and BROK during test cleanup to WARN. This happens
before message is printed. So I think your v2 is proposing:

TCONF - print CONF message, increase skipped
TFAIL - print WARN message, increase warnings
TBROK - print WARN message, increase warnings

I find v2 style more clear, because message in log matches
summary at the end.

Regards,
Jan

> 
> Best Reagrds,
> Xiao Yang
> 
> > Regards,
> > Jan
> >
> >
> >
> > .
> >
> 
> 
> 
>
Xiao Yang Nov. 12, 2018, 2:29 a.m. UTC | #7
On 2018/11/10 1:52, Jan Stancek wrote:
>
> ----- Original Message -----
>> On 2018/11/09 15:54, Jan Stancek wrote:
>>> ----- Original Message -----
>>>> On 2018/11/09 1:53, Jan Stancek wrote:
>>>>> ----- Original Message -----
>>>>>> 1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is
>>>>>>       passed into tst_brk() and convert it to TBROK forcely.
>>>>>> 2) Only update test result in library process and main test process
>>>>>>       because the exit status of child can be passed into main test
>>>>>>       process by check_child_status().
>>>>>> 3) Increase the number of skipped when calling tst_brk(TCONF).
>>>>>> 4) Increase the number of warnings when calling tst_brk(TBROK) in
>>>>>>       test cleanup(), other than that print "Test broken!" when calling
>>>>>>       tst_brk(TBROK).
>>>>>>
>>>>>> Fix: #408
>>>>>>
>>>>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>>>>>> ---
>>>>>>     lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>>>>>>     1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>>>>>> index 661fbbf..c8d8eff 100644
>>>>>> --- a/lib/tst_test.c
>>>>>> +++ b/lib/tst_test.c
>>>>>> @@ -55,6 +55,7 @@ struct results {
>>>>>>     	int skipped;
>>>>>>     	int failed;
>>>>>>     	int warnings;
>>>>>> +	int broken;
>>>>> Hi,
>>>>>
>>>>> I don't follow what benefit this provides. It generates message "Test
>>>>> broken",
>>>>> but we already know that test is broken by message in tst_vbrk_() /
>>>>> tst_cvres().
>>>> Hi Jan,
>>>>
>>>> We can remove the unnecessary message "Test broken", and also apply the
>>>> check
>>>> for
>>>> ttype in tst_brk() written by your patch.
>>> Or maybe add "Broken: " to summary?
>>>
>>>> According to the issue #$08, we want to increase result counters when
>>>> calling
>>>> tst_brk().
>>>> e.g.
>>>> 1) Increase the skipped counter when calling tst_brk(TCONF).
>>>> 2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in
>>>>       test cleanup(), other than that increase the failed counter when
>>>>       calling tst_brk(TBROK/FAIL).
>>> I'd keep counters reflecting the messages. I imagine if summary says
>>> "warnings: 1", people would be searching for 'WARN' in output.
>>>
>>> TCONF - print CONF message, increase skipped
>>> TFAIL - print FAIL message, increase failed
>>> TBROK - print BROK message, increase broken
>>>
>>> What do you think?
>> Hi Jan,
>>
>> Usually, calling tst_brk() can do above behaviors as you said.
>>
>> How about doing the following behaviors when calling tst_brk() in test
>> cleanup?
>> -----------------------------------------------
>> TCONF - print CONF message, increase skipped
>> TFAIL - print FAIL message, increase warnings
>> TBROK - print BROK message, increase warnings
>> -----------------------------------------------
> I don't think this matches your v2 patch. In your v2, we would
> convert FAIL and BROK during test cleanup to WARN. This happens
> before message is printed. So I think your v2 is proposing:
>
> TCONF - print CONF message, increase skipped
> TFAIL - print WARN message, increase warnings
> TBROK - print WARN message, increase warnings
>
> I find v2 style more clear, because message in log matches
> summary at the end.
Hi Jan,

Sorry, i gave a mismatched example.

You are right, and i will send a v3 patch as you suggested.

Best Regards,
Xiao Yang
> Regards,
> Jan
>
>> Best Reagrds,
>> Xiao Yang
>>
>>> Regards,
>>> Jan
>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>
> .
>
Cyril Hrubis Dec. 11, 2018, 3:17 p.m. UTC | #8
Hi!
> You are right, and i will send a v3 patch as you suggested.

Are you going to send v3?

I think that we really should increase counters on tst_brk(), that was
an oversight when I designed the library.

However summing up TBROK and TFAIL under results->failed does not look
right to me.

Also I'm not sure if we want to convert TFAIL to TWARN inside of test
cleanup. And even if we wanted that change it should most likely go in
in a separate patch.
Xiao Yang Dec. 12, 2018, 7:14 a.m. UTC | #9
On 2018/12/11 23:17, Cyril Hrubis wrote:
> Hi!
>> You are right, and i will send a v3 patch as you suggested.
> Are you going to send v3?
>
> I think that we really should increase counters on tst_brk(), that was
> an oversight when I designed the library.
>
> However summing up TBROK and TFAIL under results->failed does not look
> right to me.
Hi Cyril,

Yes, I am going to send v3.
I will pass TBROK to results->broken and  TFAIL to results->failed in my 
v3 patch.
> Also I'm not sure if we want to convert TFAIL to TWARN inside of test
> cleanup. And even if we wanted that change it should most likely go in
> in a separate patch.
Why do we need to convert TBROK to TWARN in test cleanup?  Your fix 
patch(commit 6440c5d)
has avoided the Infinite loop in test cleanup, so can we just keep the 
actual result by removing
the existed conversion?

Best Regards,
Xiao Yang
Cyril Hrubis Jan. 7, 2019, 1:30 p.m. UTC | #10
Hi!
> > Also I'm not sure if we want to convert TFAIL to TWARN inside of test
> > cleanup. And even if we wanted that change it should most likely go in
> > in a separate patch.
> Why do we need to convert TBROK to TWARN in test cleanup?  Your fix 
> patch(commit 6440c5d)
> has avoided the Infinite loop in test cleanup, so can we just keep the 
> actual result by removing
> the existed conversion?

That is just to be backward compatible with the conventions we used to
have before we allowed for SAFE_MACROS() to be used in cleanups. Before
that most of the tests were coded so that failure in cleanup produced a
warning.
diff mbox series

Patch

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 661fbbf..c8d8eff 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -55,6 +55,7 @@  struct results {
 	int skipped;
 	int failed;
 	int warnings;
+	int broken;
 	unsigned int timeout;
 };
 
@@ -159,6 +160,18 @@  void tst_reinit(void)
 	SAFE_CLOSE(fd);
 }
 
+static int tst_check_ttype(int ttype)
+{
+	if (TTYPE_RESULT(ttype) != TCONF && TTYPE_RESULT(ttype) != TBROK) {
+		tst_res(TINFO, "tst_brk(): invalid type %s, use TBROK forcely",
+			tst_strttype(ttype));
+		ttype &= ~TTYPE_MASK;
+		ttype |= TBROK;
+	}
+
+	return ttype;
+}
+
 static void update_results(int ttype)
 {
 	if (!results)
@@ -177,6 +190,9 @@  static void update_results(int ttype)
 	case TFAIL:
 		tst_atomic_inc(&results->failed);
 	break;
+	case TBROK:
+		tst_atomic_inc(&results->broken);
+	break;
 	}
 }
 
@@ -305,11 +321,15 @@  void tst_vbrk_(const char *file, const int lineno, int ttype,
 	 * specified but CLONE_THREAD is not. Use direct syscall to avoid
 	 * cleanup running in the child.
 	 */
-	if (syscall(SYS_getpid) == main_pid)
+	if (syscall(SYS_getpid) == main_pid) {
+		update_results(ttype);
 		do_test_cleanup();
+	}
 
-	if (getpid() == lib_pid)
+	if (getpid() == lib_pid) {
+		update_results(ttype);
 		do_exit(TTYPE_RESULT(ttype));
+	}
 
 	exit(TTYPE_RESULT(ttype));
 }
@@ -330,6 +350,7 @@  void tst_brk_(const char *file, const int lineno, int ttype,
 	va_list va;
 
 	va_start(va, fmt);
+	ttype = tst_check_ttype(ttype);
 	tst_brk_handler(file, lineno, ttype, fmt, va);
 	va_end(va);
 }
@@ -605,6 +626,9 @@  static void do_exit(int ret)
 			ret |= TWARN;
 	}
 
+	if (results->broken)
+		printf("Test broken!\n");
+
 	do_cleanup();
 
 	exit(ret);