[PATCH 1/4] tests: add option_d_r.test

["Zhou, Wenjian/周文剑"]

On 03/25/2016 06:41 AM, Dmitry V. Levin wrote:
> On Wed, Mar 23, 2016 at 04:56:36PM +0800, Zhou Wenjian wrote:
>> * tests/option_d_r.c: New file.
>> * tests/option_d_r.expected: New file.
>> * tests/option_d_r.test: New test.
>> * tests/.gitignore: Add option_d_r.
>> * tests/Makefile.am (check_PROGRAMS): Likewise.
>> (TESTS): Add option_d_r.test.
>
> Well, what can I say?  This patch is so spectacularly wrong
> in various aspects that I hardly can do it justice.
>

Sorry for that and thanks for your comments.

I have never contributed to strace before, so some misunderstandings exits.
It will be much better in next version.

>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -151,3 +151,4 @@ xetitimer
>>   xetpgid
>>   xetpriority
>>   xettimeofday
>> +option_d_r
>
> An element is added at the end of the sorted list.
>
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -199,6 +199,7 @@ check_PROGRAMS = \
>>   	xetpgid \
>>   	xetpriority \
>>   	xettimeofday \
>> +	option_d_r \
>>   	# end of check_PROGRAMS
>
> An element is added at the end of the sorted list.
>
>> @@ -378,6 +379,7 @@ TESTS = \
>>   	opipe.test \
>>   	redirect.test \
>>   	restart_syscall.test \
>> +	option_d_r.test \
>>   	$(LIBUNWIND_TESTS)
>
> An element is added at the end of the sorted list.
>

I saw count.test is not in order, so I regarded it as unsorted list.
It seems that I misunderstood it.

>> +int
>> +main()
>> +{
>> +        int pid = fork();
>> +	assert(pid != -1);
>
> Inconsistent indentation.
>
>> +        return 0;
>> +}
>> +
>
> Trailing whitespace.
>

That's my fault.
I won't make such mistakes.

>> --- /dev/null
>> +++ b/tests/option_d_r.expected
>> @@ -0,0 +1,6 @@
>> +.*/strace: new tcb for pid [0-9]*, active tcbs:1
>> +.*/strace: pid [0-9]* has TCB_STARTUP, initializing it
>> +.*/strace: new tcb for pid [0-9]*, active tcbs:2
>> +.*/strace: dropped tcb for pid [0-9]*, 1 remain
>> +.*/strace: dropped tcb for pid [0-9]*, 0 remain
>> +^.*0.[0-9] execve.*
>
> Two completely unrelated options -d and -r are tested together.
>

These options are not core functions of strace.
I thought it not worth separate case for each such option.
But it seems I was wrong.

> -d enables debug output which is not a part of any API and therefore
> can change (and sometimes changes) without notice, so hardly worth a test.
>
> Does it test whether the output produced by -r option makes any sense?
> No, it doesn't.
>
> Compare with tests/strace-r.expected that does a strict check.
>
>

I think there are two kinds of case for each option.
One is testing if it works, the other is testing if it works well.

For example, if -d is specified, one is testing if strace can output debug information,
and the other is testing if the debug output is correct.

Just as you said, the debug output can change without notice,
it is hardly to test whether -d option works well.
I think it is reasonable to test if it works.
What do you think about it?

For option r, ttt, t, T, it is almost the same issue.
It is hard to do strict check, for the outputs are based on the exact time.

-- 
Thanks
Zhou

>
>
> ------------------------------------------------------------------------------
> Transform Data into Opportunity.
> Accelerate data analysis in your applications with
> Intel Data Analytics Acceleration Library.
> Click to learn more.
> http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140
>
>
>
> _______________________________________________
> Strace-devel mailing list
> Strace-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/strace-devel
>