[v2,3/3] test: support trace-autotest when enable trace
Checks
Commit Message
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
Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
app/test/meson.build | 11 +++++++++++
1 file changed, 11 insertions(+)
Comments
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?
> + 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:
@@ -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
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
>
>
On Fri, Jun 17, 2022 at 4:43 AM fengchengwen <fengchengwen@huawei.com> wrote:
>
> 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.
Indeed.
@@ -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)
+ test_args += ['--trace=.*']
+ test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
+ test(arg[0], dpdk_test,
+ env : ['DPDK_TEST=' + arg[0]],
+ args : test_args,
+ timeout : timeout_seconds_fast,
+ is_parallel : false,
+ suite : 'fast-tests')
+ endif
endforeach
foreach arg : perf_test_names