Re: [PATCH v2 3/3] test: support trace-autotest when enable trace

[fengchengwen <fengchengwen@huawei.com>]

Hi David, thanks for your review.

On 2022/6/15 22:00, David Marchand wrote:
> On Tue, Jun 14, 2022 at 8:06 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>>
>> There are a bug[1] when exit application while enable tracing, this
>> bug has not been discovered for a long time, to quickly detect such
>> bugs, this patch was introduced.
>>
>> This patch adds a testcase with trace enabling, it also depends on
>> patch[2] because it has a long file-prefix.
>>
>> [1] eal: fix segment fault when exit trace
>> [2] eal: fix trace init fail with long file-prefix
> 
> This commitlog feels more like a cover letter.
> 
> Please describe what the impact of the patch is, like mention that the
> trace_autotest unit test is being called twice, once with traces
> disabled, and once with traces enabled.
> And that the traces file is being written in a directory part of the
> build directory.
> 
> 
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  app/test/meson.build | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/app/test/meson.build b/app/test/meson.build
>> index 7fe261cae8..eb37aa632a 100644
>> --- a/app/test/meson.build
>> +++ b/app/test/meson.build
>> @@ -509,6 +509,17 @@ foreach arg : fast_tests
>>                  is_parallel : false,
>>                  suite : 'fast-tests')
>>      endif
>> +
>> +    if run_test and arg[0] == 'trace_autotest' and (not is_windows)
> 
> Calling this test on Windows should not be an issue.
> Is it not the case?

I notice that eal_parse_common_option still keep windows macro for trace parameter parse:

#ifndef RTE_EXEC_ENV_WINDOWS
	case OPT_TRACE_NUM: {
		...
	case OPT_TRACE_MODE_NUM: {
		if (eal_trace_mode_args_save(optarg) < 0) {
			RTE_LOG(ERR, EAL, "invalid parameters for --"
				OPT_TRACE_MODE "\n");
			return -1;
		}
		break;
	}
#endif /* !RTE_EXEC_ENV_WINDOWS */

Maybe trace already support in windows, but I havn't the windows enviorment to verify it.

So I think we should keep '(not is_windows)' currently.

> 
> 
>> +        test_args += ['--trace=.*']
>> +        test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
>> +        test(arg[0], dpdk_test,
> 
> 58/68 DPDK:fast-tests / trace_autotest                   OK              0.16s
> 59/68 DPDK:fast-tests / trace_autotest                   OK              0.16s
> 
> By using the same test name, it's hard to tell what the difference is
> between those two lines.
> And practically, as a developer reproducing/troubleshooting a test
> failure, you can't call only one of the test case.
> 
> I am not sure what could be done to enhance this.., how about using a
> dedicated test name?
> Like below snippet, replacing the whole proposed patch:

Good idea, already fix in v3

> 
> @@ -508,6 +508,16 @@ foreach arg : fast_tests
>                  timeout : timeout_seconds_fast,
>                  is_parallel : false,
>                  suite : 'fast-tests')
> +        if arg[0] == 'trace_autotest'
> +            test_args += ['--trace=.*']
> +            test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
> +            test(arg[0] + '_with_traces', dpdk_test,
> +                    env : ['DPDK_TEST=' + arg[0]],
> +                    args : test_args,
> +                    timeout : timeout_seconds_fast,
> +                    is_parallel : false,
> +                    suite : 'fast-tests')
> +        endif
>      endif
>  endforeach
> 
>