From: Ian Rogers <irogers@google.com>
To: Riccardo Mancini <rickyman7@gmail.com>
Cc: eranian@google.com, Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] perf test: Be more consistent in use of TEST_*
Date: Thu, 29 Jul 2021 11:24:52 -0700 [thread overview]
Message-ID: <CAP-5=fV6E5BOSdxOBYCW3KyfgbpxMzHoUh1juxxm5s3tX9Mg-w@mail.gmail.com> (raw)
In-Reply-To: <cb2ebcbb2ea963169823ad052be8ebf9290cc97b.camel@gmail.com>
On Thu, Jul 29, 2021 at 6:41 AM Riccardo Mancini <rickyman7@gmail.com> wrote:
>
> Hi Ian,
>
> On Wed, 2021-07-28 at 23:24 -0700, Ian Rogers wrote:
> > The TEST_OK, TEST_FAIL and TEST_SKIP enum values are used
> > inconsistently. Try to reduce this by swapping constants for enum values
> > to try to be more intention revealing.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/arch/x86/tests/rdpmc.c | 8 +-
> > tools/perf/tests/attr.c | 2 +-
> > tools/perf/tests/bitmap.c | 2 +-
> > tools/perf/tests/bp_account.c | 4 +-
> > tools/perf/tests/bp_signal.c | 51 +++++++--
> > tools/perf/tests/code-reading.c | 12 +-
> > tools/perf/tests/cpumap.c | 10 +-
> > tools/perf/tests/dso-data.c | 8 +-
> > tools/perf/tests/dwarf-unwind.c | 14 ++-
> > tools/perf/tests/event-times.c | 2 +-
> > tools/perf/tests/evsel-roundtrip-name.c | 14 +--
> > tools/perf/tests/evsel-tp-sched.c | 28 ++---
> > tools/perf/tests/expr.c | 4 +-
> > tools/perf/tests/fdarray.c | 4 +-
> > tools/perf/tests/genelf.c | 2 +-
> > tools/perf/tests/hists_cumulate.c | 2 +-
> > tools/perf/tests/hists_filter.c | 12 +-
> > tools/perf/tests/hists_link.c | 33 +++---
> > tools/perf/tests/keep-tracking.c | 4 +-
> > tools/perf/tests/kmod-path.c | 6 +-
> > tools/perf/tests/mem.c | 4 +-
> > tools/perf/tests/mem2node.c | 2 +-
> > tools/perf/tests/mmap-basic.c | 10 +-
> > tools/perf/tests/mmap-thread-lookup.c | 2 +-
> > tools/perf/tests/openat-syscall-all-cpus.c | 4 +-
> > tools/perf/tests/openat-syscall-tp-fields.c | 4 +-
> > tools/perf/tests/openat-syscall.c | 6 +-
> > tools/perf/tests/parse-events.c | 118 ++++++++++----------
> > tools/perf/tests/parse-metric.c | 16 +--
> > tools/perf/tests/parse-no-sample-id-all.c | 4 +-
> > tools/perf/tests/perf-hooks.c | 2 +-
> > tools/perf/tests/perf-record.c | 2 +-
> > tools/perf/tests/perf-time-to-tsc.c | 4 +-
> > tools/perf/tests/pfm.c | 4 +-
> > tools/perf/tests/pmu-events.c | 36 +++---
> > tools/perf/tests/pmu.c | 16 +--
> > tools/perf/tests/python-use.c | 2 +-
> > tools/perf/tests/sample-parsing.c | 10 +-
> > tools/perf/tests/stat.c | 12 +-
> > tools/perf/tests/sw-clock.c | 8 +-
> > tools/perf/tests/switch-tracking.c | 9 +-
> > tools/perf/tests/task-exit.c | 12 +-
> > tools/perf/tests/tests.h | 4 +-
> > tools/perf/tests/thread-map.c | 8 +-
> > tools/perf/tests/thread-maps-share.c | 2 +-
> > tools/perf/tests/time-utils-test.c | 2 +-
> > tools/perf/tests/topology.c | 2 +-
> > tools/perf/tests/vmlinux-kallsyms.c | 6 +-
> > tools/perf/tests/wp.c | 10 +-
> > 49 files changed, 292 insertions(+), 251 deletions(-)
> >
> <SNIP>
> > diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> > index 9866cddebf23..70e92e074dba 100644
> > --- a/tools/perf/tests/code-reading.c
> > +++ b/tools/perf/tests/code-reading.c
> > @@ -725,20 +725,20 @@ int test__code_reading(struct test *test __maybe_unused,
> > int subtest __maybe_unu
> >
> > switch (ret) {
> > case TEST_CODE_READING_OK:
> > - return 0;
> > + return TEST_OK;
> > case TEST_CODE_READING_NO_VMLINUX:
> > pr_debug("no vmlinux\n");
> > - return 0;
> > + return TEST_SKIP;
> > case TEST_CODE_READING_NO_KCORE:
> > pr_debug("no kcore\n");
> > - return 0;
> > + return TEST_SKIP;
> > case TEST_CODE_READING_NO_ACCESS:
> > pr_debug("no access\n");
> > - return 0;
> > + return TEST_SKIP;
> > case TEST_CODE_READING_NO_KERNEL_OBJ:
> > pr_debug("no kernel obj\n");
> > - return 0;
> > + return TEST_SKIP;
> > default:
> > - return -1;
> > + return TEST_FAIL;
> > };
> > }
>
>
> I think it's better to separate changes that do not change the current behaviour
> from these changes (0 -> TEST_SKIP) into different patches.
Ack. This is the only case of this I see as TEST_OK would contradict
the debug output. There are also cases where I've swapped things like
-ENOMEM or -EINVAL for TEST_FAIL for things that generally won't
happen like malloc failures - the test calling function doesn't handle
-ENOMEM. Separating all of these out into CLs is work I'd prefer to
avoid, but I agree it makes just eye-balling this as a substitution CL
hard.
Thanks,
Ian
> Riccardo
>
> > diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> > index 0472b110fe65..bfcb85a965bb 100644
> > --- a/tools/perf/tests/cpumap.c
> > +++ b/tools/perf/tests/cpumap.c
> > @@ -42,7 +42,7 @@ static int process_event_mask(struct perf_tool *tool
> > __maybe_unused,
> > }
> >
> > perf_cpu_map__put(map);
> > - return 0;
> > + return TEST_OK;
> > }
> >
> > static int process_event_cpus(struct perf_tool *tool __maybe_unused,
> > @@ -71,7 +71,7 @@ static int process_event_cpus(struct perf_tool *tool
> > __maybe_unused,
> > TEST_ASSERT_VAL("wrong cpu", map->map[1] == 256);
> > TEST_ASSERT_VAL("wrong refcnt", refcount_read(&map->refcnt) == 1);
> > perf_cpu_map__put(map);
> > - return 0;
> > + return TEST_OK;
> > }
> >
> >
> > @@ -94,7 +94,7 @@ int test__cpu_map_synthesize(struct test *test
> > __maybe_unused, int subtest __may
> > !perf_event__synthesize_cpu_map(NULL, cpus,
> > process_event_cpus, NULL));
> >
> > perf_cpu_map__put(cpus);
> > - return 0;
> > + return TEST_OK;
> > }
> >
> > static int cpu_map_print(const char *str)
> > @@ -120,7 +120,7 @@ int test__cpu_map_print(struct test *test __maybe_unused,
> > int subtest __maybe_un
> > TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,3-6,8-
> > 10,24,35-37"));
> > TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,3-6,8-
> > 10,24,35-37"));
> > TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1-10,12-20,22-
> > 30,32-40"));
> > - return 0;
> > + return TEST_OK;
> > }
> >
> > int test__cpu_map_merge(struct test *test __maybe_unused, int subtest
> > __maybe_unused)
> > @@ -135,5 +135,5 @@ int test__cpu_map_merge(struct test *test __maybe_unused,
> > int subtest __maybe_un
> > TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, "1-
> > 2,4-5,7"));
> > perf_cpu_map__put(b);
> > perf_cpu_map__put(c);
> > - return 0;
> > + return TEST_OK;
> > }
> <SNIP>
>
prev parent reply other threads:[~2021-07-29 18:25 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-29 6:24 [PATCH 0/3] Some exit code tidying Ian Rogers
2021-07-29 6:24 ` [PATCH 1/3] libperf: Prefer exit(EXIT_SUCCESS) over exit(0) Ian Rogers
2021-07-29 6:24 ` [PATCH 2/3] tools perf: Prefer exit(EXIT_*) over exit(0|1) Ian Rogers
2021-07-29 6:24 ` [PATCH 3/3] perf test: Be more consistent in use of TEST_* Ian Rogers
2021-07-29 13:39 ` Riccardo Mancini
2021-07-29 18:24 ` Ian Rogers [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAP-5=fV6E5BOSdxOBYCW3KyfgbpxMzHoUh1juxxm5s3tX9Mg-w@mail.gmail.com' \
--to=irogers@google.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=eranian@google.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=rickyman7@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.