* [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
@ 2023-03-16 6:39 Manu Bretelle
2023-03-16 15:03 ` Eduard Zingerman
2023-03-16 23:21 ` Andrii Nakryiko
0 siblings, 2 replies; 10+ messages in thread
From: Manu Bretelle @ 2023-03-16 6:39 UTC (permalink / raw)
To: chantr4, bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
Currently, test_progs outputs all stdout/stderr as it runs, and when it
is done, prints a summary.
It is non-trivial for tooling to parse that output and extract meaningful
information from it.
This change adds a new option, `--json-summary`/`-J` that let the caller
specify a file where `test_progs{,-no_alu32}` can write a summary of the
run in a json format that can later be parsed by tooling.
Currently, it creates a summary section with successes/skipped/failures
followed by a list of failed tests/subtests.
A test contains the following fields:
- test_name: the name of the test
- test_number: the number of the test
- message: the log message that was printed by the test.
- failed: A boolean indicating whether the test failed or not. Currently
we only output failed tests, but in the future, successful tests could
be added.
A subtest contains the following fields:
- test_name: same as above
- test_number: sanme as above
- subtest_name: the name of the subtest
- subtest_number: the number of the subtest.
- message: the log message that was printed by the subtest.
- is_subtest: a boolean indicating that the entry is a subtest.
- failed: same as above but for the subtest
```
$ sudo ./test_progs -a $(grep -v '^#' ./DENYLIST.aarch64 | awk '{print
$1","}' | tr -d '\n') -j -J /tmp/test_progs.json
$ jq . < /tmp/test_progs.json | head -n 30
$ head -n 30 /tmp/test_progs.json
{
"success": 29,
"success_subtest": 23,
"skipped": 3,
"failed": 27,
"results": [{
"test_name": "bpf_cookie",
"test_number": 10,
"message": "test_bpf_cookie:PASS:skel_open 0 nsec\n",
"failed": true
},{
"test_name": "bpf_cookie",
"subtest_name": "multi_kprobe_link_api",
"test_number": 10,
"subtest_number": 2,
"message": "kprobe_multi_link_api_subtest:PASS:load_kallsyms
0 nsec\nlibbpf: extern 'bpf_testmod_fentry_test1' (strong): not
resolved\nlibbpf: failed to load object 'kprobe_multi'\nlibbpf: failed
to load BPF skeleton 'kprobe_multi':
-3\nkprobe_multi_link_api_subtest:FAIL:fentry_raw_skel_load unexpected
error: -3\n",
"is_subtest": true,
"failed": true
},{
"test_name": "bpf_cookie",
"subtest_name": "multi_kprobe_attach_api",
"test_number": 10,
"subtest_number": 3,
"message": "libbpf: extern 'bpf_testmod_fentry_test1'
(strong): not resolved\nlibbpf: failed to load object
'kprobe_multi'\nlibbpf: failed to load BPF skeleton 'kprobe_multi':
-3\nkprobe_multi_attach_api_subtest:FAIL:fentry_raw_skel_load unexpected
error: -3\n",
"is_subtest": true,
"failed": true
},{
"test_name": "bpf_cookie",
"subtest_name": "lsm",
"test_number": 10,
```
The file can then be used to print a summary of the test run and list of
failing tests/subtests:
```
$ jq -r < /tmp/test_progs.json '"Success:
\(.success)/\(.success_subtest), Skipped: \(.skipped), Failed:
\(.failed)"'
Success: 29/23, Skipped: 3, Failed: 27
$ jq -r <
/tmp/test_progs.json '.results[] | if .is_subtest then
"#\(.test_number)/\(.subtest_number) \(.test_name)/\(.subtest_name)"
else "#\(.test_number) \(.test_name)" end'
```
Signed-off-by: Manu Bretelle <chantr4@gmail.com>
---
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/json_writer.c | 1 +
tools/testing/selftests/bpf/json_writer.h | 1 +
tools/testing/selftests/bpf/test_progs.c | 83 +++++++++++++++++++++--
tools/testing/selftests/bpf/test_progs.h | 1 +
5 files changed, 84 insertions(+), 6 deletions(-)
create mode 120000 tools/testing/selftests/bpf/json_writer.c
create mode 120000 tools/testing/selftests/bpf/json_writer.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 55811c448eb7..fc092582d16d 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -234,6 +234,7 @@ $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(BPFOBJ)
CGROUP_HELPERS := $(OUTPUT)/cgroup_helpers.o
TESTING_HELPERS := $(OUTPUT)/testing_helpers.o
TRACE_HELPERS := $(OUTPUT)/trace_helpers.o
+JSON_WRITER := $(OUTPUT)/json_writer.o
CAP_HELPERS := $(OUTPUT)/cap_helpers.o
$(OUTPUT)/test_dev_cgroup: $(CGROUP_HELPERS) $(TESTING_HELPERS)
@@ -559,7 +560,8 @@ TRUNNER_BPF_PROGS_DIR := progs
TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c \
network_helpers.c testing_helpers.c \
btf_helpers.c flow_dissector_load.h \
- cap_helpers.c test_loader.c xsk.c disasm.c
+ cap_helpers.c test_loader.c xsk.c disasm.c \
+ json_writer.c
TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
$(OUTPUT)/liburandom_read.so \
$(OUTPUT)/xdp_synproxy \
diff --git a/tools/testing/selftests/bpf/json_writer.c b/tools/testing/selftests/bpf/json_writer.c
new file mode 120000
index 000000000000..5effa31e2f39
--- /dev/null
+++ b/tools/testing/selftests/bpf/json_writer.c
@@ -0,0 +1 @@
+../../../bpf/bpftool/json_writer.c
\ No newline at end of file
diff --git a/tools/testing/selftests/bpf/json_writer.h b/tools/testing/selftests/bpf/json_writer.h
new file mode 120000
index 000000000000..e0a264c26752
--- /dev/null
+++ b/tools/testing/selftests/bpf/json_writer.h
@@ -0,0 +1 @@
+../../../bpf/bpftool/json_writer.h
\ No newline at end of file
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 6d5e3022c75f..cf56f6a4e1af 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -18,6 +18,7 @@
#include <sys/socket.h>
#include <sys/un.h>
#include <bpf/btf.h>
+#include "json_writer.h"
static bool verbose(void)
{
@@ -269,10 +270,22 @@ static void print_subtest_name(int test_num, int subtest_num,
fprintf(env.stdout, "\n");
}
+static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt)
+{
+ // open_memstream (from stdio_hijack_init) ensures that log_bug is terminated by a
+ // null byte. Yet in parralel mode, log_buf will be NULL if there is no message.
+ if (log_cnt) {
+ jsonw_string_field(w, "message", log_buf);
+ } else {
+ jsonw_string_field(w, "message", "");
+ }
+}
+
static void dump_test_log(const struct prog_test_def *test,
const struct test_state *test_state,
bool skip_ok_subtests,
- bool par_exec_result)
+ bool par_exec_result,
+ json_writer_t *w)
{
bool test_failed = test_state->error_cnt > 0;
bool force_log = test_state->force_log;
@@ -296,6 +309,15 @@ static void dump_test_log(const struct prog_test_def *test,
if (test_state->log_cnt && print_test)
print_test_log(test_state->log_buf, test_state->log_cnt);
+ if (w && print_test) {
+ jsonw_start_object(w);
+ jsonw_string_field(w, "test_name", test->test_name);
+ jsonw_uint_field(w, "test_number", test->test_num);
+ jsonw_write_log_message(w, test_state->log_buf, test_state->log_cnt);
+ jsonw_bool_field(w, "failed", true);
+ jsonw_end_object(w);
+ }
+
for (i = 0; i < test_state->subtest_num; i++) {
subtest_state = &test_state->subtest_states[i];
subtest_failed = subtest_state->error_cnt;
@@ -314,6 +336,19 @@ static void dump_test_log(const struct prog_test_def *test,
test->test_name, subtest_state->name,
test_result(subtest_state->error_cnt,
subtest_state->skipped));
+
+ if (w && print_subtest) {
+ jsonw_start_object(w);
+ jsonw_string_field(w, "test_name", test->test_name);
+ jsonw_string_field(w, "subtest_name", subtest_state->name);
+ jsonw_uint_field(w, "test_number", test->test_num);
+ jsonw_uint_field(w, "subtest_number", i+1);
+ jsonw_write_log_message(w, subtest_state->log_buf, subtest_state->log_cnt);
+ jsonw_bool_field(w, "is_subtest", true);
+ jsonw_bool_field(w, "failed", true);
+ jsonw_end_object(w);
+ }
+
}
print_test_result(test, test_state);
@@ -715,6 +750,7 @@ enum ARG_KEYS {
ARG_TEST_NAME_GLOB_DENYLIST = 'd',
ARG_NUM_WORKERS = 'j',
ARG_DEBUG = -1,
+ ARG_JSON_SUMMARY = 'J'
};
static const struct argp_option opts[] = {
@@ -740,6 +776,7 @@ static const struct argp_option opts[] = {
"Number of workers to run in parallel, default to number of cpus." },
{ "debug", ARG_DEBUG, NULL, 0,
"print extra debug information for test_progs." },
+ { "json-summary", ARG_JSON_SUMMARY, "FILE", 0, "Write report in json format to this file."},
{},
};
@@ -870,6 +907,13 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
case ARG_DEBUG:
env->debug = true;
break;
+ case ARG_JSON_SUMMARY:
+ env->json = fopen(arg, "w");
+ if (env->json == NULL) {
+ perror("Failed to open json summary file");
+ return -errno;
+ }
+ break;
case ARGP_KEY_ARG:
argp_usage(state);
break;
@@ -1017,7 +1061,7 @@ void crash_handler(int signum)
stdio_restore();
if (env.test) {
env.test_state->error_cnt++;
- dump_test_log(env.test, env.test_state, true, false);
+ dump_test_log(env.test, env.test_state, true, false, NULL);
}
if (env.worker_id != -1)
fprintf(stderr, "[%d]: ", env.worker_id);
@@ -1124,7 +1168,7 @@ static void run_one_test(int test_num)
stdio_restore();
- dump_test_log(test, state, false, false);
+ dump_test_log(test, state, false, false, NULL);
}
struct dispatch_data {
@@ -1283,7 +1327,7 @@ static void *dispatch_thread(void *ctx)
} while (false);
pthread_mutex_lock(&stdout_output_lock);
- dump_test_log(test, state, false, true);
+ dump_test_log(test, state, false, true, NULL);
pthread_mutex_unlock(&stdout_output_lock);
} /* while (true) */
error:
@@ -1322,8 +1366,28 @@ static void calculate_summary_and_print_errors(struct test_env *env)
fail_cnt++;
else
succ_cnt++;
+
+ }
+
+ json_writer_t *w = NULL;
+
+ if (env->json) {
+ w = jsonw_new(env->json);
+ if (!w)
+ fprintf(env->stderr, "Failed to create new JSON stream.");
}
+ if (w) {
+ jsonw_pretty(w, 1);
+ jsonw_start_object(w);
+ jsonw_uint_field(w, "success", succ_cnt);
+ jsonw_uint_field(w, "success_subtest", sub_succ_cnt);
+ jsonw_uint_field(w, "skipped", skip_cnt);
+ jsonw_uint_field(w, "failed", fail_cnt);
+ jsonw_name(w, "results");
+ jsonw_start_array(w);
+
+ }
/*
* We only print error logs summary when there are failed tests and
* verbose mode is not enabled. Otherwise, results may be incosistent.
@@ -1340,10 +1404,19 @@ static void calculate_summary_and_print_errors(struct test_env *env)
if (!state->tested || !state->error_cnt)
continue;
- dump_test_log(test, state, true, true);
+ dump_test_log(test, state, true, true, w);
}
}
+ if (w) {
+ jsonw_end_array(w);
+ jsonw_end_object(w);
+ jsonw_destroy(&w);
+ }
+
+ if (env->json)
+ fclose(env->json);
+
printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
succ_cnt, sub_succ_cnt, skip_cnt, fail_cnt);
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 3cbf005747ed..4b06b8347cd4 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -114,6 +114,7 @@ struct test_env {
FILE *stdout;
FILE *stderr;
int nr_cpus;
+ FILE *json;
int succ_cnt; /* successful tests */
int sub_succ_cnt; /* successful sub-tests */
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 6:39 [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs Manu Bretelle
@ 2023-03-16 15:03 ` Eduard Zingerman
2023-03-16 16:38 ` Manu Bretelle
2023-03-16 23:21 ` Andrii Nakryiko
1 sibling, 1 reply; 10+ messages in thread
From: Eduard Zingerman @ 2023-03-16 15:03 UTC (permalink / raw)
To: Manu Bretelle, bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Wed, 2023-03-15 at 23:39 -0700, Manu Bretelle wrote:
> Currently, test_progs outputs all stdout/stderr as it runs, and when it
> is done, prints a summary.
>
> It is non-trivial for tooling to parse that output and extract meaningful
> information from it.
>
> This change adds a new option, `--json-summary`/`-J` that let the caller
> specify a file where `test_progs{,-no_alu32}` can write a summary of the
> run in a json format that can later be parsed by tooling.
>
> Currently, it creates a summary section with successes/skipped/failures
> followed by a list of failed tests/subtests.
>
> A test contains the following fields:
> - test_name: the name of the test
> - test_number: the number of the test
> - message: the log message that was printed by the test.
> - failed: A boolean indicating whether the test failed or not. Currently
> we only output failed tests, but in the future, successful tests could
> be added.
>
> A subtest contains the following fields:
> - test_name: same as above
> - test_number: sanme as above
> - subtest_name: the name of the subtest
> - subtest_number: the number of the subtest.
> - message: the log message that was printed by the subtest.
> - is_subtest: a boolean indicating that the entry is a subtest.
> - failed: same as above but for the subtest
>
Looks like a great feature!
A few nitpicks below.
> ```
> $ sudo ./test_progs -a $(grep -v '^#' ./DENYLIST.aarch64 | awk '{print
> $1","}' | tr -d '\n') -j -J /tmp/test_progs.json
> $ jq . < /tmp/test_progs.json | head -n 30
> $ head -n 30 /tmp/test_progs.json
> {
> "success": 29,
> "success_subtest": 23,
> "skipped": 3,
> "failed": 27,
> "results": [{
> "test_name": "bpf_cookie",
> "test_number": 10,
> "message": "test_bpf_cookie:PASS:skel_open 0 nsec\n",
> "failed": true
> },{
> "test_name": "bpf_cookie",
> "subtest_name": "multi_kprobe_link_api",
> "test_number": 10,
> "subtest_number": 2,
> "message": "kprobe_multi_link_api_subtest:PASS:load_kallsyms
> 0 nsec\nlibbpf: extern 'bpf_testmod_fentry_test1' (strong): not
> resolved\nlibbpf: failed to load object 'kprobe_multi'\nlibbpf: failed
> to load BPF skeleton 'kprobe_multi':
> -3\nkprobe_multi_link_api_subtest:FAIL:fentry_raw_skel_load unexpected
> error: -3\n",
> "is_subtest": true,
> "failed": true
> },{
> "test_name": "bpf_cookie",
> "subtest_name": "multi_kprobe_attach_api",
> "test_number": 10,
> "subtest_number": 3,
> "message": "libbpf: extern 'bpf_testmod_fentry_test1'
> (strong): not resolved\nlibbpf: failed to load object
> 'kprobe_multi'\nlibbpf: failed to load BPF skeleton 'kprobe_multi':
> -3\nkprobe_multi_attach_api_subtest:FAIL:fentry_raw_skel_load unexpected
> error: -3\n",
> "is_subtest": true,
> "failed": true
> },{
> "test_name": "bpf_cookie",
> "subtest_name": "lsm",
> "test_number": 10,
> ```
>
> The file can then be used to print a summary of the test run and list of
> failing tests/subtests:
>
> ```
> $ jq -r < /tmp/test_progs.json '"Success:
> \(.success)/\(.success_subtest), Skipped: \(.skipped), Failed:
> \(.failed)"'
>
> Success: 29/23, Skipped: 3, Failed: 27
> $ jq -r <
> /tmp/test_progs.json '.results[] | if .is_subtest then
> "#\(.test_number)/\(.subtest_number) \(.test_name)/\(.subtest_name)"
> else "#\(.test_number) \(.test_name)" end'
> ```
>
> Signed-off-by: Manu Bretelle <chantr4@gmail.com>
> ---
> tools/testing/selftests/bpf/Makefile | 4 +-
> tools/testing/selftests/bpf/json_writer.c | 1 +
> tools/testing/selftests/bpf/json_writer.h | 1 +
> tools/testing/selftests/bpf/test_progs.c | 83 +++++++++++++++++++++--
> tools/testing/selftests/bpf/test_progs.h | 1 +
> 5 files changed, 84 insertions(+), 6 deletions(-)
> create mode 120000 tools/testing/selftests/bpf/json_writer.c
> create mode 120000 tools/testing/selftests/bpf/json_writer.h
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 55811c448eb7..fc092582d16d 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -234,6 +234,7 @@ $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(BPFOBJ)
> CGROUP_HELPERS := $(OUTPUT)/cgroup_helpers.o
> TESTING_HELPERS := $(OUTPUT)/testing_helpers.o
> TRACE_HELPERS := $(OUTPUT)/trace_helpers.o
> +JSON_WRITER := $(OUTPUT)/json_writer.o
> CAP_HELPERS := $(OUTPUT)/cap_helpers.o
>
> $(OUTPUT)/test_dev_cgroup: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> @@ -559,7 +560,8 @@ TRUNNER_BPF_PROGS_DIR := progs
> TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c \
> network_helpers.c testing_helpers.c \
> btf_helpers.c flow_dissector_load.h \
> - cap_helpers.c test_loader.c xsk.c disasm.c
> + cap_helpers.c test_loader.c xsk.c disasm.c \
> + json_writer.c
> TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
> $(OUTPUT)/liburandom_read.so \
> $(OUTPUT)/xdp_synproxy \
> diff --git a/tools/testing/selftests/bpf/json_writer.c b/tools/testing/selftests/bpf/json_writer.c
> new file mode 120000
> index 000000000000..5effa31e2f39
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/json_writer.c
> @@ -0,0 +1 @@
> +../../../bpf/bpftool/json_writer.c
> \ No newline at end of file
> diff --git a/tools/testing/selftests/bpf/json_writer.h b/tools/testing/selftests/bpf/json_writer.h
> new file mode 120000
> index 000000000000..e0a264c26752
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/json_writer.h
> @@ -0,0 +1 @@
> +../../../bpf/bpftool/json_writer.h
> \ No newline at end of file
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 6d5e3022c75f..cf56f6a4e1af 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -18,6 +18,7 @@
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <bpf/btf.h>
> +#include "json_writer.h"
>
> static bool verbose(void)
> {
> @@ -269,10 +270,22 @@ static void print_subtest_name(int test_num, int subtest_num,
> fprintf(env.stdout, "\n");
> }
>
> +static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt)
> +{
> + // open_memstream (from stdio_hijack_init) ensures that log_bug is terminated by a
> + // null byte. Yet in parralel mode, log_buf will be NULL if there is no message.
> + if (log_cnt) {
> + jsonw_string_field(w, "message", log_buf);
> + } else {
> + jsonw_string_field(w, "message", "");
> + }
> +}
> +
> static void dump_test_log(const struct prog_test_def *test,
> const struct test_state *test_state,
> bool skip_ok_subtests,
> - bool par_exec_result)
> + bool par_exec_result,
> + json_writer_t *w)
> {
> bool test_failed = test_state->error_cnt > 0;
> bool force_log = test_state->force_log;
> @@ -296,6 +309,15 @@ static void dump_test_log(const struct prog_test_def *test,
> if (test_state->log_cnt && print_test)
> print_test_log(test_state->log_buf, test_state->log_cnt);
>
> + if (w && print_test) {
> + jsonw_start_object(w);
> + jsonw_string_field(w, "test_name", test->test_name);
> + jsonw_uint_field(w, "test_number", test->test_num);
> + jsonw_write_log_message(w, test_state->log_buf, test_state->log_cnt);
> + jsonw_bool_field(w, "failed", true);
The `print_test` is used as a precondition, but it is defined as follows:
bool print_test = verbose() || force_log || test_failed;
Maybe change the line above to:
jsonw_bool_field(w, "failed", test_failed);
Or use `test_failed` as a precondition?
> + jsonw_end_object(w);
> + }
> +
> for (i = 0; i < test_state->subtest_num; i++) {
> subtest_state = &test_state->subtest_states[i];
> subtest_failed = subtest_state->error_cnt;
> @@ -314,6 +336,19 @@ static void dump_test_log(const struct prog_test_def *test,
> test->test_name, subtest_state->name,
> test_result(subtest_state->error_cnt,
> subtest_state->skipped));
> +
> + if (w && print_subtest) {
> + jsonw_start_object(w);
> + jsonw_string_field(w, "test_name", test->test_name);
> + jsonw_string_field(w, "subtest_name", subtest_state->name);
> + jsonw_uint_field(w, "test_number", test->test_num);
> + jsonw_uint_field(w, "subtest_number", i+1);
> + jsonw_write_log_message(w, subtest_state->log_buf, subtest_state->log_cnt);
> + jsonw_bool_field(w, "is_subtest", true);
> + jsonw_bool_field(w, "failed", true);
> + jsonw_end_object(w);
> + }
> +
Maybe organize failed subtests as a field of a top-level result?
E.g. as follows:
{
"success": 295,
"success_subtest": 1771,
"skipped": 18,
"failed": 3,
"results": [
{
"test_name": "btf_tag",
"test_number": 29,
"message": "",
"failed": true
"subtests": [
{
"test_name": "btf_tag",
"subtest_name": "btf_type_tag_percpu_mod1",
"test_number": 29,
"subtest_number": 6,
"message": "...",
"failed": true
}
]
}
]
}
> }
>
> print_test_result(test, test_state);
> @@ -715,6 +750,7 @@ enum ARG_KEYS {
> ARG_TEST_NAME_GLOB_DENYLIST = 'd',
> ARG_NUM_WORKERS = 'j',
> ARG_DEBUG = -1,
> + ARG_JSON_SUMMARY = 'J'
> };
>
> static const struct argp_option opts[] = {
> @@ -740,6 +776,7 @@ static const struct argp_option opts[] = {
> "Number of workers to run in parallel, default to number of cpus." },
> { "debug", ARG_DEBUG, NULL, 0,
> "print extra debug information for test_progs." },
> + { "json-summary", ARG_JSON_SUMMARY, "FILE", 0, "Write report in json format to this file."},
> {},
> };
>
> @@ -870,6 +907,13 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
> case ARG_DEBUG:
> env->debug = true;
> break;
> + case ARG_JSON_SUMMARY:
> + env->json = fopen(arg, "w");
> + if (env->json == NULL) {
> + perror("Failed to open json summary file");
> + return -errno;
> + }
> + break;
> case ARGP_KEY_ARG:
> argp_usage(state);
> break;
> @@ -1017,7 +1061,7 @@ void crash_handler(int signum)
> stdio_restore();
> if (env.test) {
> env.test_state->error_cnt++;
> - dump_test_log(env.test, env.test_state, true, false);
> + dump_test_log(env.test, env.test_state, true, false, NULL);
> }
> if (env.worker_id != -1)
> fprintf(stderr, "[%d]: ", env.worker_id);
> @@ -1124,7 +1168,7 @@ static void run_one_test(int test_num)
>
> stdio_restore();
>
> - dump_test_log(test, state, false, false);
> + dump_test_log(test, state, false, false, NULL);
> }
>
> struct dispatch_data {
> @@ -1283,7 +1327,7 @@ static void *dispatch_thread(void *ctx)
> } while (false);
>
> pthread_mutex_lock(&stdout_output_lock);
> - dump_test_log(test, state, false, true);
> + dump_test_log(test, state, false, true, NULL);
> pthread_mutex_unlock(&stdout_output_lock);
> } /* while (true) */
> error:
> @@ -1322,8 +1366,28 @@ static void calculate_summary_and_print_errors(struct test_env *env)
> fail_cnt++;
> else
> succ_cnt++;
> +
> + }
> +
> + json_writer_t *w = NULL;
> +
> + if (env->json) {
> + w = jsonw_new(env->json);
> + if (!w)
> + fprintf(env->stderr, "Failed to create new JSON stream.");
> }
>
> + if (w) {
> + jsonw_pretty(w, 1);
> + jsonw_start_object(w);
> + jsonw_uint_field(w, "success", succ_cnt);
> + jsonw_uint_field(w, "success_subtest", sub_succ_cnt);
> + jsonw_uint_field(w, "skipped", skip_cnt);
> + jsonw_uint_field(w, "failed", fail_cnt);
> + jsonw_name(w, "results");
> + jsonw_start_array(w);
> +
> + }
> /*
> * We only print error logs summary when there are failed tests and
> * verbose mode is not enabled. Otherwise, results may be incosistent.
> @@ -1340,10 +1404,19 @@ static void calculate_summary_and_print_errors(struct test_env *env)
> if (!state->tested || !state->error_cnt)
> continue;
>
> - dump_test_log(test, state, true, true);
> + dump_test_log(test, state, true, true, w);
> }
> }
>
> + if (w) {
> + jsonw_end_array(w);
> + jsonw_end_object(w);
> + jsonw_destroy(&w);
> + }
> +
> + if (env->json)
> + fclose(env->json);
> +
> printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> succ_cnt, sub_succ_cnt, skip_cnt, fail_cnt);
>
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 3cbf005747ed..4b06b8347cd4 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -114,6 +114,7 @@ struct test_env {
> FILE *stdout;
> FILE *stderr;
> int nr_cpus;
> + FILE *json;
>
> int succ_cnt; /* successful tests */
> int sub_succ_cnt; /* successful sub-tests */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 15:03 ` Eduard Zingerman
@ 2023-03-16 16:38 ` Manu Bretelle
2023-03-16 19:18 ` Eduard Zingerman
0 siblings, 1 reply; 10+ messages in thread
From: Manu Bretelle @ 2023-03-16 16:38 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Thu, Mar 16, 2023 at 05:03:01PM +0200, Eduard Zingerman wrote:
> On Wed, 2023-03-15 at 23:39 -0700, Manu Bretelle wrote:
> > Currently, test_progs outputs all stdout/stderr as it runs, and when it
> > is done, prints a summary.
> >
> > It is non-trivial for tooling to parse that output and extract meaningful
> > information from it.
> >
> > This change adds a new option, `--json-summary`/`-J` that let the caller
> > specify a file where `test_progs{,-no_alu32}` can write a summary of the
> > run in a json format that can later be parsed by tooling.
> >
> > Currently, it creates a summary section with successes/skipped/failures
> > followed by a list of failed tests/subtests.
> >
> > A test contains the following fields:
> > - test_name: the name of the test
> > - test_number: the number of the test
> > - message: the log message that was printed by the test.
> > - failed: A boolean indicating whether the test failed or not. Currently
> > we only output failed tests, but in the future, successful tests could
> > be added.
> >
> > A subtest contains the following fields:
> > - test_name: same as above
> > - test_number: sanme as above
> > - subtest_name: the name of the subtest
> > - subtest_number: the number of the subtest.
> > - message: the log message that was printed by the subtest.
> > - is_subtest: a boolean indicating that the entry is a subtest.
> > - failed: same as above but for the subtest
> >
>
> Looks like a great feature!
> A few nitpicks below.
>
> > ```
> > $ sudo ./test_progs -a $(grep -v '^#' ./DENYLIST.aarch64 | awk '{print
> > $1","}' | tr -d '\n') -j -J /tmp/test_progs.json
> > $ jq . < /tmp/test_progs.json | head -n 30
> > $ head -n 30 /tmp/test_progs.json
> > {
> > "success": 29,
> > "success_subtest": 23,
> > "skipped": 3,
> > "failed": 27,
> > "results": [{
> > "test_name": "bpf_cookie",
> > "test_number": 10,
> > "message": "test_bpf_cookie:PASS:skel_open 0 nsec\n",
> > "failed": true
> > },{
> > "test_name": "bpf_cookie",
> > "subtest_name": "multi_kprobe_link_api",
> > "test_number": 10,
> > "subtest_number": 2,
> > "message": "kprobe_multi_link_api_subtest:PASS:load_kallsyms
> > 0 nsec\nlibbpf: extern 'bpf_testmod_fentry_test1' (strong): not
> > resolved\nlibbpf: failed to load object 'kprobe_multi'\nlibbpf: failed
> > to load BPF skeleton 'kprobe_multi':
> > -3\nkprobe_multi_link_api_subtest:FAIL:fentry_raw_skel_load unexpected
> > error: -3\n",
> > "is_subtest": true,
> > "failed": true
> > },{
> > "test_name": "bpf_cookie",
> > "subtest_name": "multi_kprobe_attach_api",
> > "test_number": 10,
> > "subtest_number": 3,
> > "message": "libbpf: extern 'bpf_testmod_fentry_test1'
> > (strong): not resolved\nlibbpf: failed to load object
> > 'kprobe_multi'\nlibbpf: failed to load BPF skeleton 'kprobe_multi':
> > -3\nkprobe_multi_attach_api_subtest:FAIL:fentry_raw_skel_load unexpected
> > error: -3\n",
> > "is_subtest": true,
> > "failed": true
> > },{
> > "test_name": "bpf_cookie",
> > "subtest_name": "lsm",
> > "test_number": 10,
> > ```
> >
> > The file can then be used to print a summary of the test run and list of
> > failing tests/subtests:
> >
> > ```
> > $ jq -r < /tmp/test_progs.json '"Success:
> > \(.success)/\(.success_subtest), Skipped: \(.skipped), Failed:
> > \(.failed)"'
> >
> > Success: 29/23, Skipped: 3, Failed: 27
> > $ jq -r <
> > /tmp/test_progs.json '.results[] | if .is_subtest then
> > "#\(.test_number)/\(.subtest_number) \(.test_name)/\(.subtest_name)"
> > else "#\(.test_number) \(.test_name)" end'
> > ```
> >
> > Signed-off-by: Manu Bretelle <chantr4@gmail.com>
> > ---
> > tools/testing/selftests/bpf/Makefile | 4 +-
> > tools/testing/selftests/bpf/json_writer.c | 1 +
> > tools/testing/selftests/bpf/json_writer.h | 1 +
> > tools/testing/selftests/bpf/test_progs.c | 83 +++++++++++++++++++++--
> > tools/testing/selftests/bpf/test_progs.h | 1 +
> > 5 files changed, 84 insertions(+), 6 deletions(-)
> > create mode 120000 tools/testing/selftests/bpf/json_writer.c
> > create mode 120000 tools/testing/selftests/bpf/json_writer.h
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 55811c448eb7..fc092582d16d 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -234,6 +234,7 @@ $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(BPFOBJ)
> > CGROUP_HELPERS := $(OUTPUT)/cgroup_helpers.o
> > TESTING_HELPERS := $(OUTPUT)/testing_helpers.o
> > TRACE_HELPERS := $(OUTPUT)/trace_helpers.o
> > +JSON_WRITER := $(OUTPUT)/json_writer.o
> > CAP_HELPERS := $(OUTPUT)/cap_helpers.o
> >
> > $(OUTPUT)/test_dev_cgroup: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> > @@ -559,7 +560,8 @@ TRUNNER_BPF_PROGS_DIR := progs
> > TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c \
> > network_helpers.c testing_helpers.c \
> > btf_helpers.c flow_dissector_load.h \
> > - cap_helpers.c test_loader.c xsk.c disasm.c
> > + cap_helpers.c test_loader.c xsk.c disasm.c \
> > + json_writer.c
> > TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
> > $(OUTPUT)/liburandom_read.so \
> > $(OUTPUT)/xdp_synproxy \
> > diff --git a/tools/testing/selftests/bpf/json_writer.c b/tools/testing/selftests/bpf/json_writer.c
> > new file mode 120000
> > index 000000000000..5effa31e2f39
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/json_writer.c
> > @@ -0,0 +1 @@
> > +../../../bpf/bpftool/json_writer.c
> > \ No newline at end of file
> > diff --git a/tools/testing/selftests/bpf/json_writer.h b/tools/testing/selftests/bpf/json_writer.h
> > new file mode 120000
> > index 000000000000..e0a264c26752
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/json_writer.h
> > @@ -0,0 +1 @@
> > +../../../bpf/bpftool/json_writer.h
> > \ No newline at end of file
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 6d5e3022c75f..cf56f6a4e1af 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -18,6 +18,7 @@
> > #include <sys/socket.h>
> > #include <sys/un.h>
> > #include <bpf/btf.h>
> > +#include "json_writer.h"
> >
> > static bool verbose(void)
> > {
> > @@ -269,10 +270,22 @@ static void print_subtest_name(int test_num, int subtest_num,
> > fprintf(env.stdout, "\n");
> > }
> >
> > +static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt)
> > +{
> > + // open_memstream (from stdio_hijack_init) ensures that log_bug is terminated by a
> > + // null byte. Yet in parralel mode, log_buf will be NULL if there is no message.
> > + if (log_cnt) {
> > + jsonw_string_field(w, "message", log_buf);
> > + } else {
> > + jsonw_string_field(w, "message", "");
> > + }
> > +}
> > +
> > static void dump_test_log(const struct prog_test_def *test,
> > const struct test_state *test_state,
> > bool skip_ok_subtests,
> > - bool par_exec_result)
> > + bool par_exec_result,
> > + json_writer_t *w)
> > {
> > bool test_failed = test_state->error_cnt > 0;
> > bool force_log = test_state->force_log;
> > @@ -296,6 +309,15 @@ static void dump_test_log(const struct prog_test_def *test,
> > if (test_state->log_cnt && print_test)
> > print_test_log(test_state->log_buf, test_state->log_cnt);
> >
> > + if (w && print_test) {
> > + jsonw_start_object(w);
> > + jsonw_string_field(w, "test_name", test->test_name);
> > + jsonw_uint_field(w, "test_number", test->test_num);
> > + jsonw_write_log_message(w, test_state->log_buf, test_state->log_cnt);
> > + jsonw_bool_field(w, "failed", true);
>
> The `print_test` is used as a precondition, but it is defined as follows:
>
> bool print_test = verbose() || force_log || test_failed;
>
> Maybe change the line above to:
>
> jsonw_bool_field(w, "failed", test_failed);
>
Good point/catch. Thanks.
> Or use `test_failed` as a precondition?
>
> > + jsonw_end_object(w);
> > + }
> > +
> > for (i = 0; i < test_state->subtest_num; i++) {
> > subtest_state = &test_state->subtest_states[i];
> > subtest_failed = subtest_state->error_cnt;
> > @@ -314,6 +336,19 @@ static void dump_test_log(const struct prog_test_def *test,
> > test->test_name, subtest_state->name,
> > test_result(subtest_state->error_cnt,
> > subtest_state->skipped));
> > +
> > + if (w && print_subtest) {
> > + jsonw_start_object(w);
> > + jsonw_string_field(w, "test_name", test->test_name);
> > + jsonw_string_field(w, "subtest_name", subtest_state->name);
> > + jsonw_uint_field(w, "test_number", test->test_num);
> > + jsonw_uint_field(w, "subtest_number", i+1);
> > + jsonw_write_log_message(w, subtest_state->log_buf, subtest_state->log_cnt);
> > + jsonw_bool_field(w, "is_subtest", true);
> > + jsonw_bool_field(w, "failed", true);
> > + jsonw_end_object(w);
> > + }
> > +
>
> Maybe organize failed subtests as a field of a top-level result?
> E.g. as follows:
>
> {
> "success": 295,
> "success_subtest": 1771,
> "skipped": 18,
> "failed": 3,
> "results": [
> {
> "test_name": "btf_tag",
> "test_number": 29,
> "message": "",
> "failed": true
> "subtests": [
> {
> "test_name": "btf_tag",
> "subtest_name": "btf_type_tag_percpu_mod1",
> "test_number": 29,
> "subtest_number": 6,
> "message": "...",
> "failed": true
> }
> ]
> }
> ]
> }
I was originally going to do a nested structure similar to this too
(minus the repeat of test_* entries for subtests. But while discussing this
offline with Andrii, a flatter structured seemed to be easier to parse/manage
with tools such as `jq`. I am also very probably missing the right
incantation for `jq`.
Finding whether a test has subtests (currently only adding failed ones,
but this could change in the future) would be easier (essentially checking
length(subtests)). But neither is it difficult to reconstruct using
higher level language.
In term of logical structure and maybe extensibility, this is more appropriate,
in term of pragmatism maybe less.
I don't have strong opinions and can see benefit for both.
>
> > }
> >
> > print_test_result(test, test_state);
> > @@ -715,6 +750,7 @@ enum ARG_KEYS {
> > ARG_TEST_NAME_GLOB_DENYLIST = 'd',
> > ARG_NUM_WORKERS = 'j',
> > ARG_DEBUG = -1,
> > + ARG_JSON_SUMMARY = 'J'
> > };
> >
> > static const struct argp_option opts[] = {
> > @@ -740,6 +776,7 @@ static const struct argp_option opts[] = {
> > "Number of workers to run in parallel, default to number of cpus." },
> > { "debug", ARG_DEBUG, NULL, 0,
> > "print extra debug information for test_progs." },
> > + { "json-summary", ARG_JSON_SUMMARY, "FILE", 0, "Write report in json format to this file."},
> > {},
> > };
> >
> > @@ -870,6 +907,13 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
> > case ARG_DEBUG:
> > env->debug = true;
> > break;
> > + case ARG_JSON_SUMMARY:
> > + env->json = fopen(arg, "w");
> > + if (env->json == NULL) {
> > + perror("Failed to open json summary file");
> > + return -errno;
> > + }
> > + break;
> > case ARGP_KEY_ARG:
> > argp_usage(state);
> > break;
> > @@ -1017,7 +1061,7 @@ void crash_handler(int signum)
> > stdio_restore();
> > if (env.test) {
> > env.test_state->error_cnt++;
> > - dump_test_log(env.test, env.test_state, true, false);
> > + dump_test_log(env.test, env.test_state, true, false, NULL);
> > }
> > if (env.worker_id != -1)
> > fprintf(stderr, "[%d]: ", env.worker_id);
> > @@ -1124,7 +1168,7 @@ static void run_one_test(int test_num)
> >
> > stdio_restore();
> >
> > - dump_test_log(test, state, false, false);
> > + dump_test_log(test, state, false, false, NULL);
> > }
> >
> > struct dispatch_data {
> > @@ -1283,7 +1327,7 @@ static void *dispatch_thread(void *ctx)
> > } while (false);
> >
> > pthread_mutex_lock(&stdout_output_lock);
> > - dump_test_log(test, state, false, true);
> > + dump_test_log(test, state, false, true, NULL);
> > pthread_mutex_unlock(&stdout_output_lock);
> > } /* while (true) */
> > error:
> > @@ -1322,8 +1366,28 @@ static void calculate_summary_and_print_errors(struct test_env *env)
> > fail_cnt++;
> > else
> > succ_cnt++;
> > +
> > + }
> > +
> > + json_writer_t *w = NULL;
> > +
> > + if (env->json) {
> > + w = jsonw_new(env->json);
> > + if (!w)
> > + fprintf(env->stderr, "Failed to create new JSON stream.");
> > }
> >
> > + if (w) {
> > + jsonw_pretty(w, 1);
> > + jsonw_start_object(w);
> > + jsonw_uint_field(w, "success", succ_cnt);
> > + jsonw_uint_field(w, "success_subtest", sub_succ_cnt);
> > + jsonw_uint_field(w, "skipped", skip_cnt);
> > + jsonw_uint_field(w, "failed", fail_cnt);
> > + jsonw_name(w, "results");
> > + jsonw_start_array(w);
> > +
> > + }
> > /*
> > * We only print error logs summary when there are failed tests and
> > * verbose mode is not enabled. Otherwise, results may be incosistent.
> > @@ -1340,10 +1404,19 @@ static void calculate_summary_and_print_errors(struct test_env *env)
> > if (!state->tested || !state->error_cnt)
> > continue;
> >
> > - dump_test_log(test, state, true, true);
> > + dump_test_log(test, state, true, true, w);
> > }
> > }
> >
> > + if (w) {
> > + jsonw_end_array(w);
> > + jsonw_end_object(w);
> > + jsonw_destroy(&w);
> > + }
> > +
> > + if (env->json)
> > + fclose(env->json);
> > +
> > printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> > succ_cnt, sub_succ_cnt, skip_cnt, fail_cnt);
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > index 3cbf005747ed..4b06b8347cd4 100644
> > --- a/tools/testing/selftests/bpf/test_progs.h
> > +++ b/tools/testing/selftests/bpf/test_progs.h
> > @@ -114,6 +114,7 @@ struct test_env {
> > FILE *stdout;
> > FILE *stderr;
> > int nr_cpus;
> > + FILE *json;
> >
> > int succ_cnt; /* successful tests */
> > int sub_succ_cnt; /* successful sub-tests */
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 16:38 ` Manu Bretelle
@ 2023-03-16 19:18 ` Eduard Zingerman
2023-03-16 23:23 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Eduard Zingerman @ 2023-03-16 19:18 UTC (permalink / raw)
To: Manu Bretelle; +Cc: bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Thu, 2023-03-16 at 09:38 -0700, Manu Bretelle wrote:
> [...]
>
> I was originally going to do a nested structure similar to this too
> (minus the repeat of test_* entries for subtests. But while discussing this
> offline with Andrii, a flatter structured seemed to be easier to parse/manage
> with tools such as `jq`. I am also very probably missing the right
> incantation for `jq`.
>
> Finding whether a test has subtests (currently only adding failed ones,
> but this could change in the future) would be easier (essentially checking
> length(subtests)). But neither is it difficult to reconstruct using
> higher level language.
>
`jq` query is a bit more complicated with nested structure, but not terribly so:
$ cat query.jq
.results | map([
.test_name,
(.subtests | map([([.test_name, .subtest_name] | join("/")) ]))
])
| flatten
$ jq -f query.jq test.json
[
"test_global_funcs",
"test_global_funcs/global_func16"
]
Test data for reference:
$ cat test.json | sed -r 's/"[^"]{20,}"/"..."/g'
{
"success": 1,
"success_subtest": 24,
"skipped": 0,
"failed": 1,
"results": [{
"test_name": "test_global_funcs",
"test_number": 223,
"message": "...",
"failed": true,
"subtests": [{
"test_name": "test_global_funcs",
"subtest_name": "global_func16",
"test_number": 223,
"subtest_number": 16,
"message": "...",
"is_subtest": true,
"failed": true
}
]
}
]
}
> In term of logical structure and maybe extensibility, this is more appropriate,
> in term of pragmatism maybe less.
>
> I don't have strong opinions and can see benefit for both.
idk, I don't have a strong opinion either.
> [...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 6:39 [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs Manu Bretelle
2023-03-16 15:03 ` Eduard Zingerman
@ 2023-03-16 23:21 ` Andrii Nakryiko
2023-03-17 0:40 ` Manu Bretelle
1 sibling, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2023-03-16 23:21 UTC (permalink / raw)
To: Manu Bretelle; +Cc: bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Wed, Mar 15, 2023 at 11:39 PM Manu Bretelle <chantr4@gmail.com> wrote:
>
> Currently, test_progs outputs all stdout/stderr as it runs, and when it
> is done, prints a summary.
>
> It is non-trivial for tooling to parse that output and extract meaningful
> information from it.
>
> This change adds a new option, `--json-summary`/`-J` that let the caller
> specify a file where `test_progs{,-no_alu32}` can write a summary of the
> run in a json format that can later be parsed by tooling.
>
> Currently, it creates a summary section with successes/skipped/failures
> followed by a list of failed tests/subtests.
>
> A test contains the following fields:
> - test_name: the name of the test
> - test_number: the number of the test
> - message: the log message that was printed by the test.
> - failed: A boolean indicating whether the test failed or not. Currently
> we only output failed tests, but in the future, successful tests could
> be added.
>
> A subtest contains the following fields:
> - test_name: same as above
> - test_number: sanme as above
> - subtest_name: the name of the subtest
> - subtest_number: the number of the subtest.
> - message: the log message that was printed by the subtest.
> - is_subtest: a boolean indicating that the entry is a subtest.
"is_" prefix stands out compared to other bool field ("failed"),
should we call this just "subtest" for consistency?
> - failed: same as above but for the subtest
>
> ```
> $ sudo ./test_progs -a $(grep -v '^#' ./DENYLIST.aarch64 | awk '{print
> $1","}' | tr -d '\n') -j -J /tmp/test_progs.json
> $ jq . < /tmp/test_progs.json | head -n 30
> $ head -n 30 /tmp/test_progs.json
> {
> "success": 29,
> "success_subtest": 23,
> "skipped": 3,
> "failed": 27,
> "results": [{
> "test_name": "bpf_cookie",
> "test_number": 10,
> "message": "test_bpf_cookie:PASS:skel_open 0 nsec\n",
> "failed": true
> },{
> "test_name": "bpf_cookie",
> "subtest_name": "multi_kprobe_link_api",
> "test_number": 10,
> "subtest_number": 2,
> "message": "kprobe_multi_link_api_subtest:PASS:load_kallsyms
> 0 nsec\nlibbpf: extern 'bpf_testmod_fentry_test1' (strong): not
> resolved\nlibbpf: failed to load object 'kprobe_multi'\nlibbpf: failed
> to load BPF skeleton 'kprobe_multi':
> -3\nkprobe_multi_link_api_subtest:FAIL:fentry_raw_skel_load unexpected
> error: -3\n",
> "is_subtest": true,
> "failed": true
> },{
> "test_name": "bpf_cookie",
> "subtest_name": "multi_kprobe_attach_api",
> "test_number": 10,
> "subtest_number": 3,
> "message": "libbpf: extern 'bpf_testmod_fentry_test1'
> (strong): not resolved\nlibbpf: failed to load object
> 'kprobe_multi'\nlibbpf: failed to load BPF skeleton 'kprobe_multi':
> -3\nkprobe_multi_attach_api_subtest:FAIL:fentry_raw_skel_load unexpected
> error: -3\n",
> "is_subtest": true,
> "failed": true
> },{
> "test_name": "bpf_cookie",
> "subtest_name": "lsm",
> "test_number": 10,
> ```
>
> The file can then be used to print a summary of the test run and list of
> failing tests/subtests:
>
> ```
> $ jq -r < /tmp/test_progs.json '"Success:
> \(.success)/\(.success_subtest), Skipped: \(.skipped), Failed:
> \(.failed)"'
>
> Success: 29/23, Skipped: 3, Failed: 27
> $ jq -r <
> /tmp/test_progs.json '.results[] | if .is_subtest then
> "#\(.test_number)/\(.subtest_number) \(.test_name)/\(.subtest_name)"
> else "#\(.test_number) \(.test_name)" end'
> ```
>
> Signed-off-by: Manu Bretelle <chantr4@gmail.com>
> ---
Looks great, some nits below.
> tools/testing/selftests/bpf/Makefile | 4 +-
> tools/testing/selftests/bpf/json_writer.c | 1 +
> tools/testing/selftests/bpf/json_writer.h | 1 +
> tools/testing/selftests/bpf/test_progs.c | 83 +++++++++++++++++++++--
> tools/testing/selftests/bpf/test_progs.h | 1 +
> 5 files changed, 84 insertions(+), 6 deletions(-)
> create mode 120000 tools/testing/selftests/bpf/json_writer.c
> create mode 120000 tools/testing/selftests/bpf/json_writer.h
>
[...]
> @@ -269,10 +270,22 @@ static void print_subtest_name(int test_num, int subtest_num,
> fprintf(env.stdout, "\n");
> }
>
> +static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt)
> +{
> + // open_memstream (from stdio_hijack_init) ensures that log_bug is terminated by a
> + // null byte. Yet in parralel mode, log_buf will be NULL if there is no message.
please don't use C++-style comments, let's use /* */ consistently
also typo: parallel
> + if (log_cnt) {
> + jsonw_string_field(w, "message", log_buf);
> + } else {
> + jsonw_string_field(w, "message", "");
> + }
> +}
> +
[...]
> @@ -1283,7 +1327,7 @@ static void *dispatch_thread(void *ctx)
> } while (false);
>
> pthread_mutex_lock(&stdout_output_lock);
> - dump_test_log(test, state, false, true);
> + dump_test_log(test, state, false, true, NULL);
> pthread_mutex_unlock(&stdout_output_lock);
> } /* while (true) */
> error:
> @@ -1322,8 +1366,28 @@ static void calculate_summary_and_print_errors(struct test_env *env)
> fail_cnt++;
> else
> succ_cnt++;
> +
> + }
> +
> + json_writer_t *w = NULL;
please declare variable at the top to follow C89 style
> +
> + if (env->json) {
> + w = jsonw_new(env->json);
> + if (!w)
> + fprintf(env->stderr, "Failed to create new JSON stream.");
> }
>
> + if (w) {
> + jsonw_pretty(w, 1);
true, it's bool
> + jsonw_start_object(w);
> + jsonw_uint_field(w, "success", succ_cnt);
> + jsonw_uint_field(w, "success_subtest", sub_succ_cnt);
> + jsonw_uint_field(w, "skipped", skip_cnt);
> + jsonw_uint_field(w, "failed", fail_cnt);
> + jsonw_name(w, "results");
> + jsonw_start_array(w);
> +
> + }
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 19:18 ` Eduard Zingerman
@ 2023-03-16 23:23 ` Andrii Nakryiko
2023-03-16 23:33 ` Eduard Zingerman
0 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2023-03-16 23:23 UTC (permalink / raw)
To: Eduard Zingerman
Cc: Manu Bretelle, bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Thu, Mar 16, 2023 at 12:18 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2023-03-16 at 09:38 -0700, Manu Bretelle wrote:
> > [...]
> >
> > I was originally going to do a nested structure similar to this too
> > (minus the repeat of test_* entries for subtests. But while discussing this
> > offline with Andrii, a flatter structured seemed to be easier to parse/manage
> > with tools such as `jq`. I am also very probably missing the right
> > incantation for `jq`.
> >
> > Finding whether a test has subtests (currently only adding failed ones,
> > but this could change in the future) would be easier (essentially checking
> > length(subtests)). But neither is it difficult to reconstruct using
> > higher level language.
> >
>
> `jq` query is a bit more complicated with nested structure, but not terribly so:
>
> $ cat query.jq
> .results | map([
> .test_name,
> (.subtests | map([([.test_name, .subtest_name] | join("/")) ]))
> ])
> | flatten
we should record this in the commit, if we go with nested structure :)
it's not "terribly more complicated", but not obvious either for
someone who uses jq very occasionally :)
>
> $ jq -f query.jq test.json
> [
> "test_global_funcs",
> "test_global_funcs/global_func16"
> ]
>
> Test data for reference:
>
> $ cat test.json | sed -r 's/"[^"]{20,}"/"..."/g'
> {
> "success": 1,
> "success_subtest": 24,
> "skipped": 0,
> "failed": 1,
> "results": [{
> "test_name": "test_global_funcs",
> "test_number": 223,
> "message": "...",
> "failed": true,
> "subtests": [{
> "test_name": "test_global_funcs",
> "subtest_name": "global_func16",
> "test_number": 223,
> "subtest_number": 16,
> "message": "...",
> "is_subtest": true,
> "failed": true
> }
> ]
> }
> ]
> }
>
> > In term of logical structure and maybe extensibility, this is more appropriate,
> > in term of pragmatism maybe less.
> >
> > I don't have strong opinions and can see benefit for both.
>
> idk, I don't have a strong opinion either.
me neither, flatter struct would be simple to work with either with jq
or hacky grepping, so I guess the question would be how much do we
lose by using flatter structure?
>
> > [...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 23:23 ` Andrii Nakryiko
@ 2023-03-16 23:33 ` Eduard Zingerman
2023-03-17 0:07 ` Andrii Nakryiko
2023-03-17 0:24 ` Manu Bretelle
0 siblings, 2 replies; 10+ messages in thread
From: Eduard Zingerman @ 2023-03-16 23:33 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Manu Bretelle, bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Thu, 2023-03-16 at 16:23 -0700, Andrii Nakryiko wrote:
> > [...]
> >
> > > In term of logical structure and maybe extensibility, this is more appropriate,
> > > in term of pragmatism maybe less.
> > >
> > > I don't have strong opinions and can see benefit for both.
> >
> > idk, I don't have a strong opinion either.
>
> me neither, flatter struct would be simple to work with either with jq
> or hacky grepping, so I guess the question would be how much do we
> lose by using flatter structure?
Okay, okay, noone wants to read jq manual, I get it :)
I assume that current plan is to consume this output by some CI script
(and e.g. provide a foldable list of failed tests/sub-tests in the
final output). So, we should probably use whatever makes more sense
for those scripts. If you and Manu think that flat structure works
best -- so be it.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 23:33 ` Eduard Zingerman
@ 2023-03-17 0:07 ` Andrii Nakryiko
2023-03-17 0:24 ` Manu Bretelle
1 sibling, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2023-03-17 0:07 UTC (permalink / raw)
To: Eduard Zingerman
Cc: Manu Bretelle, bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Thu, Mar 16, 2023 at 4:34 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2023-03-16 at 16:23 -0700, Andrii Nakryiko wrote:
> > > [...]
> > >
> > > > In term of logical structure and maybe extensibility, this is more appropriate,
> > > > in term of pragmatism maybe less.
> > > >
> > > > I don't have strong opinions and can see benefit for both.
> > >
> > > idk, I don't have a strong opinion either.
> >
> > me neither, flatter struct would be simple to work with either with jq
> > or hacky grepping, so I guess the question would be how much do we
> > lose by using flatter structure?
>
> Okay, okay, noone wants to read jq manual, I get it :)
guilty ;)
>
> I assume that current plan is to consume this output by some CI script
> (and e.g. provide a foldable list of failed tests/sub-tests in the
> final output). So, we should probably use whatever makes more sense
> for those scripts. If you and Manu think that flat structure works
> best -- so be it.
As I said, I don't care. The immediate need is to produce error
summary output from this json. If you are a power user of jq and can
help us achieve this with nested structure json, that would be great.
Code wins.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 23:33 ` Eduard Zingerman
2023-03-17 0:07 ` Andrii Nakryiko
@ 2023-03-17 0:24 ` Manu Bretelle
1 sibling, 0 replies; 10+ messages in thread
From: Manu Bretelle @ 2023-03-17 0:24 UTC (permalink / raw)
To: Eduard Zingerman
Cc: Andrii Nakryiko, bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Fri, Mar 17, 2023 at 01:33:56AM +0200, Eduard Zingerman wrote:
> On Thu, 2023-03-16 at 16:23 -0700, Andrii Nakryiko wrote:
> > > [...]
> > >
> > > > In term of logical structure and maybe extensibility, this is more appropriate,
> > > > in term of pragmatism maybe less.
> > > >
> > > > I don't have strong opinions and can see benefit for both.
> > >
> > > idk, I don't have a strong opinion either.
> >
> > me neither, flatter struct would be simple to work with either with jq
> > or hacky grepping, so I guess the question would be how much do we
> > lose by using flatter structure?
>
> Okay, okay, noone wants to read jq manual, I get it :)
>
> I assume that current plan is to consume this output by some CI script
> (and e.g. provide a foldable list of failed tests/sub-tests in the
> final output).
Correct.
> So, we should probably use whatever makes more sense
> for those scripts. If you and Manu think that flat structure works
> best -- so be it.
Nested is the most logical representation of the data. Given the
one-liner you provided, let me play a bit more with it and see what it
would take to get minimal info out of it.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs
2023-03-16 23:21 ` Andrii Nakryiko
@ 2023-03-17 0:40 ` Manu Bretelle
0 siblings, 0 replies; 10+ messages in thread
From: Manu Bretelle @ 2023-03-17 0:40 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, andrii, mykolal, ast, daniel, martin.lau, yhs
On Thu, Mar 16, 2023 at 04:21:52PM -0700, Andrii Nakryiko wrote:
> On Wed, Mar 15, 2023 at 11:39 PM Manu Bretelle <chantr4@gmail.com> wrote:
> >
> > Currently, test_progs outputs all stdout/stderr as it runs, and when it
> > is done, prints a summary.
> >
> > It is non-trivial for tooling to parse that output and extract meaningful
> > information from it.
> >
> > This change adds a new option, `--json-summary`/`-J` that let the caller
> > specify a file where `test_progs{,-no_alu32}` can write a summary of the
> > run in a json format that can later be parsed by tooling.
> >
> > Currently, it creates a summary section with successes/skipped/failures
> > followed by a list of failed tests/subtests.
> >
> > A test contains the following fields:
> > - test_name: the name of the test
> > - test_number: the number of the test
> > - message: the log message that was printed by the test.
> > - failed: A boolean indicating whether the test failed or not. Currently
> > we only output failed tests, but in the future, successful tests could
> > be added.
> >
> > A subtest contains the following fields:
> > - test_name: same as above
> > - test_number: sanme as above
> > - subtest_name: the name of the subtest
> > - subtest_number: the number of the subtest.
> > - message: the log message that was printed by the subtest.
> > - is_subtest: a boolean indicating that the entry is a subtest.
>
> "is_" prefix stands out compared to other bool field ("failed"),
> should we call this just "subtest" for consistency?
>
yes, I will give a try to the nested representation and play a bit more
with jq. In any case, this will become `subtest[s]`.
> > - failed: same as above but for the subtest
> >
> > ```
> > $ sudo ./test_progs -a $(grep -v '^#' ./DENYLIST.aarch64 | awk '{print
> > $1","}' | tr -d '\n') -j -J /tmp/test_progs.json
> > $ jq . < /tmp/test_progs.json | head -n 30
> > $ head -n 30 /tmp/test_progs.json
> > {
> > "success": 29,
> > "success_subtest": 23,
> > "skipped": 3,
> > "failed": 27,
> > "results": [{
> > "test_name": "bpf_cookie",
> > "test_number": 10,
> > "message": "test_bpf_cookie:PASS:skel_open 0 nsec\n",
> > "failed": true
> > },{
> > "test_name": "bpf_cookie",
> > "subtest_name": "multi_kprobe_link_api",
> > "test_number": 10,
> > "subtest_number": 2,
> > "message": "kprobe_multi_link_api_subtest:PASS:load_kallsyms
> > 0 nsec\nlibbpf: extern 'bpf_testmod_fentry_test1' (strong): not
> > resolved\nlibbpf: failed to load object 'kprobe_multi'\nlibbpf: failed
> > to load BPF skeleton 'kprobe_multi':
> > -3\nkprobe_multi_link_api_subtest:FAIL:fentry_raw_skel_load unexpected
> > error: -3\n",
> > "is_subtest": true,
> > "failed": true
> > },{
> > "test_name": "bpf_cookie",
> > "subtest_name": "multi_kprobe_attach_api",
> > "test_number": 10,
> > "subtest_number": 3,
> > "message": "libbpf: extern 'bpf_testmod_fentry_test1'
> > (strong): not resolved\nlibbpf: failed to load object
> > 'kprobe_multi'\nlibbpf: failed to load BPF skeleton 'kprobe_multi':
> > -3\nkprobe_multi_attach_api_subtest:FAIL:fentry_raw_skel_load unexpected
> > error: -3\n",
> > "is_subtest": true,
> > "failed": true
> > },{
> > "test_name": "bpf_cookie",
> > "subtest_name": "lsm",
> > "test_number": 10,
> > ```
> >
> > The file can then be used to print a summary of the test run and list of
> > failing tests/subtests:
> >
> > ```
> > $ jq -r < /tmp/test_progs.json '"Success:
> > \(.success)/\(.success_subtest), Skipped: \(.skipped), Failed:
> > \(.failed)"'
> >
> > Success: 29/23, Skipped: 3, Failed: 27
> > $ jq -r <
> > /tmp/test_progs.json '.results[] | if .is_subtest then
> > "#\(.test_number)/\(.subtest_number) \(.test_name)/\(.subtest_name)"
> > else "#\(.test_number) \(.test_name)" end'
> > ```
> >
> > Signed-off-by: Manu Bretelle <chantr4@gmail.com>
> > ---
>
> Looks great, some nits below.
>
> > tools/testing/selftests/bpf/Makefile | 4 +-
> > tools/testing/selftests/bpf/json_writer.c | 1 +
> > tools/testing/selftests/bpf/json_writer.h | 1 +
> > tools/testing/selftests/bpf/test_progs.c | 83 +++++++++++++++++++++--
> > tools/testing/selftests/bpf/test_progs.h | 1 +
> > 5 files changed, 84 insertions(+), 6 deletions(-)
> > create mode 120000 tools/testing/selftests/bpf/json_writer.c
> > create mode 120000 tools/testing/selftests/bpf/json_writer.h
> >
>
> [...]
>
> > @@ -269,10 +270,22 @@ static void print_subtest_name(int test_num, int subtest_num,
> > fprintf(env.stdout, "\n");
> > }
> >
> > +static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt)
> > +{
> > + // open_memstream (from stdio_hijack_init) ensures that log_bug is terminated by a
> > + // null byte. Yet in parralel mode, log_buf will be NULL if there is no message.
>
> please don't use C++-style comments, let's use /* */ consistently
>
> also typo: parallel
>
ack
> > + if (log_cnt) {
> > + jsonw_string_field(w, "message", log_buf);
> > + } else {
> > + jsonw_string_field(w, "message", "");
> > + }
> > +}
> > +
>
> [...]
>
> > @@ -1283,7 +1327,7 @@ static void *dispatch_thread(void *ctx)
> > } while (false);
> >
> > pthread_mutex_lock(&stdout_output_lock);
> > - dump_test_log(test, state, false, true);
> > + dump_test_log(test, state, false, true, NULL);
> > pthread_mutex_unlock(&stdout_output_lock);
> > } /* while (true) */
> > error:
> > @@ -1322,8 +1366,28 @@ static void calculate_summary_and_print_errors(struct test_env *env)
> > fail_cnt++;
> > else
> > succ_cnt++;
> > +
> > + }
> > +
> > + json_writer_t *w = NULL;
>
> please declare variable at the top to follow C89 style
>
ack
> > +
> > + if (env->json) {
> > + w = jsonw_new(env->json);
> > + if (!w)
> > + fprintf(env->stderr, "Failed to create new JSON stream.");
> > }
> >
> > + if (w) {
> > + jsonw_pretty(w, 1);
>
> true, it's bool
>
actually, given that this is not printed to stdout, and jq is widely available...
probably no need to pretty the output.
> > + jsonw_start_object(w);
> > + jsonw_uint_field(w, "success", succ_cnt);
> > + jsonw_uint_field(w, "success_subtest", sub_succ_cnt);
> > + jsonw_uint_field(w, "skipped", skip_cnt);
> > + jsonw_uint_field(w, "failed", fail_cnt);
> > + jsonw_name(w, "results");
> > + jsonw_start_array(w);
> > +
> > + }
>
> [...]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-17 0:40 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16 6:39 [PATCH bpf-next] selftests/bpf: add --json-summary option to test_progs Manu Bretelle
2023-03-16 15:03 ` Eduard Zingerman
2023-03-16 16:38 ` Manu Bretelle
2023-03-16 19:18 ` Eduard Zingerman
2023-03-16 23:23 ` Andrii Nakryiko
2023-03-16 23:33 ` Eduard Zingerman
2023-03-17 0:07 ` Andrii Nakryiko
2023-03-17 0:24 ` Manu Bretelle
2023-03-16 23:21 ` Andrii Nakryiko
2023-03-17 0:40 ` Manu Bretelle
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.