lkml.org 
[lkml]   [2017]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 37/37] selftests: ftrace: Add inter-event hist triggers testcases
    On Mon, 30 Oct 2017 15:52:19 -0500
    Tom Zanussi <tom.zanussi@linux.intel.com> wrote:

    > From: Rajvi Jingar <rajvi.jingar@intel.com>
    >
    > This adds inter-event hist triggers testcases which covers following:
    > - create/remove synthetic event
    > - disable histogram for synthetic event
    > - extended error support
    > - field variable support
    > - histogram variables
    > - histogram trigger onmatch action
    > - histogram trigger onmax action
    > - histogram trigger onmatch-onmax action
    > - simple expression support
    > - combined histogram

    Thank you for adding testcases :)
    That helps me to understand how to use it.
    I have some comments, see below;


    [..]
    > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-extended-error-support.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-extended-error-support.tc
    > new file mode 100644
    > index 0000000..41dbf4c
    > --- /dev/null
    > +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-extended-error-support.tc
    > @@ -0,0 +1,39 @@
    > +#!/bin/sh
    > +# description: event trigger - test extended error support
    > +
    > +
    > +do_reset() {
    > + reset_trigger
    > + echo > set_event
    > + clear_trace
    > +}
    > +
    > +fail() { #msg
    > + do_reset
    > + echo $1
    > + exit $FAIL
    > +}
    > +
    > +if [ ! -f set_event ]; then
    > + echo "event tracing is not supported"
    > + exit_unsupported
    > +fi
    > +
    > +if [ ! -f synthetic_events ]; then
    > + echo "synthetic event is not supported"
    > + exit_unsupported
    > +fi
    > +
    > +reset_tracer
    > +do_reset
    > +
    > +echo "Test extended error support"
    > +echo 'hist:keys=pid:ts0=$common_timestamp.usecs if comm=="ping"' >> events/sched/sched_wakeup/trigger
    > +echo 'hist:keys=pid:ts0=$common_timestamp.usecs if comm=="ping"' >> events/sched/sched_wakeup/trigger &&

    What is the last "&&"? Would you expect that the second echo is fail?

    > +if ! grep "ERROR:" events/sched/sched_wakeup/hist; then

    Would you like to show the grep result? or you can use -q option for grep command.

    [...]
    > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-createremove.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-createremove.tc
    > new file mode 100644
    > index 0000000..c73e491
    > --- /dev/null
    > +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-createremove.tc
    > @@ -0,0 +1,46 @@
    > +#!/bin/sh
    > +# description: event trigger - test synthetic event create remove
    > +do_reset() {
    > + reset_trigger
    > + echo > set_event
    > + clear_trace
    > +}
    > +
    > +fail() { #msg
    > + do_reset
    > + echo $1
    > + exit $FAIL
    > +}
    > +
    > +if [ ! -f set_event ]; then
    > + echo "event tracing is not supported"
    > + exit_unsupported
    > +fi
    > +
    > +if [ ! -f synthetic_events ]; then
    > + echo "synthetic event is not supported"
    > + exit_unsupported
    > +fi
    > +
    > +clear_synthetic_events
    > +reset_tracer
    > +do_reset
    > +
    > +echo "Test create synthetic event"
    > +
    > +echo 'wakeup_latency u64 lat pid_t pid char comm[16]' > synthetic_events
    > +if [ ! -d events/synthetic/wakeup_latency ]; then
    > + fail "Failed to create wakeup_latency synthetic event"
    > +fi
    > +
    > +reset_trigger
    > +
    > +echo "Test remove synthetic event"
    > +echo '!wakeup_latency u64 lat; pid_t pid; char[16] comm' > synthetic_events

    both char[16] and char comm[16] are acceptable?
    But I would like to see same style in creating and removing.

    > +if [ -d events/synthetic/wakeup_latency ]; then
    > + fail "Failed to delete wakeup_latency synthetic event"
    > +fi

    There seems no expected failure test cases. Could you add such test?
    like as echoing events with invalid format etc.


    <off topic>
    BTW, $FAIL is not for expressing failure, it is a value
    for $SIG_RESULT. But it seems many test cases use it for return code.

    Note that any failure in this script treated as error and exit soon.
    Which means caller can get any return code from this script, because
    the return code depends on executed command. So, any NON-ZERO code
    can be treated as FAILURE code.

    "exit $FAIL" is OK, but just making myself review easier, I'll
    introduce exit_pass()/exit_fail().
    <off topic>

    Thank you,

    --
    Masami Hiramatsu <mhiramat@kernel.org>

    \
     
     \ /
      Last update: 2017-10-31 15:30    [W:2.564 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site