bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/4] selftests/bpf: test_progs: misc fixes
@ 2019-08-14 16:47 Stanislav Fomichev
  2019-08-14 16:47 ` [PATCH bpf-next 1/4] selftests/bpf: test_progs: change formatting of the condenced output Stanislav Fomichev
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Stanislav Fomichev @ 2019-08-14 16:47 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko

* make output a bit easier to follow
* add test__skip to indicate skipped tests
* remove global success/error counts (use environment)
* remove asserts from the tests

Cc: Andrii Nakryiko <andriin@fb.com>

Stanislav Fomichev (4):
  selftests/bpf: test_progs: change formatting of the condenced output
  selftests/bpf: test_progs: test__skip
  selftests/bpf: test_progs: remove global fail/success counts
  selftests/bpf: test_progs: remove asserts from subtests

 .../selftests/bpf/prog_tests/bpf_obj_id.c     | 32 +++++--
 .../bpf/prog_tests/bpf_verif_scale.c          | 10 +-
 .../selftests/bpf/prog_tests/flow_dissector.c |  2 +-
 .../bpf/prog_tests/get_stack_raw_tp.c         |  2 +-
 .../selftests/bpf/prog_tests/global_data.c    | 10 +-
 .../selftests/bpf/prog_tests/l4lb_all.c       |  4 +-
 .../selftests/bpf/prog_tests/map_lock.c       | 28 +++---
 .../selftests/bpf/prog_tests/pkt_access.c     |  2 +-
 .../selftests/bpf/prog_tests/pkt_md_access.c  |  2 +-
 .../bpf/prog_tests/queue_stack_map.c          |  4 +-
 .../bpf/prog_tests/reference_tracking.c       |  2 +-
 .../selftests/bpf/prog_tests/send_signal.c    |  1 +
 .../selftests/bpf/prog_tests/spinlock.c       | 12 ++-
 .../bpf/prog_tests/stacktrace_build_id.c      | 11 ++-
 .../bpf/prog_tests/stacktrace_build_id_nmi.c  | 11 ++-
 .../selftests/bpf/prog_tests/stacktrace_map.c |  2 +-
 .../bpf/prog_tests/stacktrace_map_raw_tp.c    |  2 +-
 .../bpf/prog_tests/task_fd_query_rawtp.c      |  2 +-
 .../bpf/prog_tests/task_fd_query_tp.c         |  2 +-
 .../selftests/bpf/prog_tests/tcp_estats.c     |  2 +-
 tools/testing/selftests/bpf/prog_tests/xdp.c  |  2 +-
 .../bpf/prog_tests/xdp_adjust_tail.c          |  2 +-
 .../selftests/bpf/prog_tests/xdp_noinline.c   |  4 +-
 tools/testing/selftests/bpf/test_progs.c      | 93 +++++++++++--------
 tools/testing/selftests/bpf/test_progs.h      | 28 +++++-
 25 files changed, 165 insertions(+), 107 deletions(-)

-- 
2.23.0.rc1.153.gdeed80330f-goog

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH bpf-next 1/4] selftests/bpf: test_progs: change formatting of the condenced output
  2019-08-14 16:47 [PATCH bpf-next 0/4] selftests/bpf: test_progs: misc fixes Stanislav Fomichev
@ 2019-08-14 16:47 ` Stanislav Fomichev
  2019-08-14 17:00   ` Alexei Starovoitov
  2019-08-14 16:47 ` [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip Stanislav Fomichev
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Stanislav Fomichev @ 2019-08-14 16:47 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko

This makes it visually simpler to follow the output.
Also, highlight with red color failures when outputting to tty.

Before:
  #1 attach_probe:FAIL
  #2 bpf_obj_id:OK
  #3/1 bpf_verif_scale:loop3.o:OK
  #3/2 bpf_verif_scale:test_verif_scale1.o:OK
  #3/3 bpf_verif_scale:test_verif_scale2.o:OK
  #3/4 bpf_verif_scale:test_verif_scale3.o:OK
  #3/5 bpf_verif_scale:pyperf50.o:OK
  #3/6 bpf_verif_scale:pyperf100.o:OK
  #3/7 bpf_verif_scale:pyperf180.o:OK
  #3/8 bpf_verif_scale:pyperf600.o:OK
  #3/9 bpf_verif_scale:pyperf600_nounroll.o:OK
  #3/10 bpf_verif_scale:loop1.o:OK
  #3/11 bpf_verif_scale:loop2.o:OK
  #3/12 bpf_verif_scale:loop4.o:OK
  #3/13 bpf_verif_scale:loop5.o:OK
  #3/14 bpf_verif_scale:strobemeta.o:OK
  #3/15 bpf_verif_scale:strobemeta_nounroll1.o:OK
  #3/16 bpf_verif_scale:strobemeta_nounroll2.o:OK
  #3/17 bpf_verif_scale:test_sysctl_loop1.o:OK
  #3/18 bpf_verif_scale:test_sysctl_loop2.o:OK
  #3/19 bpf_verif_scale:test_xdp_loop.o:OK
  #3/20 bpf_verif_scale:test_seg6_loop.o:OK
  #3 bpf_verif_scale:OK
  #4 flow_dissector:OK

After:
  #  1     FAIL attach_probe
  #  2       OK bpf_obj_id
  #  3/1     OK bpf_verif_scale:loop3.o
  #  3/2     OK bpf_verif_scale:test_verif_scale1.o
  #  3/3     OK bpf_verif_scale:test_verif_scale2.o
  #  3/4     OK bpf_verif_scale:test_verif_scale3.o
  #  3/5     OK bpf_verif_scale:pyperf50.o
  #  3/6     OK bpf_verif_scale:pyperf100.o
  #  3/7     OK bpf_verif_scale:pyperf180.o
  #  3/8     OK bpf_verif_scale:pyperf600.o
  #  3/9     OK bpf_verif_scale:pyperf600_nounroll.o
  #  3/10    OK bpf_verif_scale:loop1.o
  #  3/11    OK bpf_verif_scale:loop2.o
  #  3/12    OK bpf_verif_scale:loop4.o
  #  3/13    OK bpf_verif_scale:loop5.o
  #  3/14    OK bpf_verif_scale:strobemeta.o
  #  3/15    OK bpf_verif_scale:strobemeta_nounroll1.o
  #  3/16    OK bpf_verif_scale:strobemeta_nounroll2.o
  #  3/17    OK bpf_verif_scale:test_sysctl_loop1.o
  #  3/18    OK bpf_verif_scale:test_sysctl_loop2.o
  #  3/19    OK bpf_verif_scale:test_xdp_loop.o
  #  3/20    OK bpf_verif_scale:test_seg6_loop.o
  #  3       OK bpf_verif_scale
  #  4       OK flow_dissector

Cc: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/test_progs.c | 29 +++++++++++++++++++-----
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 12895d03d58b..1a7a2a0c0a11 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -56,6 +56,21 @@ static void dump_test_log(const struct prog_test_def *test, bool failed)
 	fseeko(stdout, 0, SEEK_SET); /* rewind */
 }
 
+static const char *test_status_string(bool success)
+{
+#define COLOR_RED	"\033[31m"
+#define COLOR_RESET	"\033[m"
+	if (success)
+		return "OK";
+
+	if (isatty(fileno(env.stdout)))
+		return COLOR_RED "FAIL" COLOR_RESET;
+	else
+		return "FAIL";
+#undef COLOR_RED
+#undef COLOR_RESET
+}
+
 void test__end_subtest()
 {
 	struct prog_test_def *test = env.test;
@@ -68,9 +83,10 @@ void test__end_subtest()
 
 	dump_test_log(test, sub_error_cnt);
 
-	fprintf(env.stdout, "#%d/%d %s:%s\n",
-	       test->test_num, test->subtest_num,
-	       test->subtest_name, sub_error_cnt ? "FAIL" : "OK");
+	fprintf(env.stdout, "#%3d/%-3d %4s %s:%s\n",
+		test->test_num, test->subtest_num,
+		test_status_string(test->fail_cnt == 0),
+		test->test_name, test->subtest_name);
 }
 
 bool test__start_subtest(const char *name)
@@ -513,9 +529,10 @@ int main(int argc, char **argv)
 
 		dump_test_log(test, test->error_cnt);
 
-		fprintf(env.stdout, "#%d %s:%s\n",
-			test->test_num, test->test_name,
-			test->error_cnt ? "FAIL" : "OK");
+		fprintf(env.stdout, "#%3d     %4s %s\n",
+			test->test_num,
+			test_status_string(test->fail_cnt == 0),
+			test->test_name);
 	}
 	stdio_restore();
 	printf("Summary: %d/%d PASSED, %d FAILED\n",
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip
  2019-08-14 16:47 [PATCH bpf-next 0/4] selftests/bpf: test_progs: misc fixes Stanislav Fomichev
  2019-08-14 16:47 ` [PATCH bpf-next 1/4] selftests/bpf: test_progs: change formatting of the condenced output Stanislav Fomichev
@ 2019-08-14 16:47 ` Stanislav Fomichev
  2019-08-14 19:22   ` Andrii Nakryiko
  2019-08-14 16:47 ` [PATCH bpf-next 3/4] selftests/bpf: test_progs: remove global fail/success counts Stanislav Fomichev
  2019-08-14 16:47 ` [PATCH bpf-next 4/4] selftests/bpf: test_progs: remove asserts from subtests Stanislav Fomichev
  3 siblings, 1 reply; 17+ messages in thread
From: Stanislav Fomichev @ 2019-08-14 16:47 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko

Export test__skip() to indicate skipped tests and use it in
test_send_signal_nmi().

Cc: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 +
 tools/testing/selftests/bpf/test_progs.c             | 9 +++++++--
 tools/testing/selftests/bpf/test_progs.h             | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 1575f0a1f586..40c2c5efdd3e 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -204,6 +204,7 @@ static int test_send_signal_nmi(void)
 		if (errno == ENOENT) {
 			printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
 			       __func__);
+			test__skip();
 			return 0;
 		}
 		/* Let the test fail with a more informative message */
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 1a7a2a0c0a11..1993f2ce0d23 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -121,6 +121,11 @@ void test__force_log() {
 	env.test->force_log = true;
 }
 
+void test__skip(void)
+{
+	env.skip_cnt++;
+}
+
 struct ipv4_packet pkt_v4 = {
 	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
 	.iph.ihl = 5,
@@ -535,8 +540,8 @@ int main(int argc, char **argv)
 			test->test_name);
 	}
 	stdio_restore();
-	printf("Summary: %d/%d PASSED, %d FAILED\n",
-	       env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
+	printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
+	       env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
 
 	free(env.test_selector.num_set);
 	free(env.subtest_selector.num_set);
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 37d427f5a1e5..9defd35cb6c0 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -64,6 +64,7 @@ struct test_env {
 	int succ_cnt; /* successful tests */
 	int sub_succ_cnt; /* successful sub-tests */
 	int fail_cnt; /* total failed tests + sub-tests */
+	int skip_cnt; /* skipped tests */
 };
 
 extern int error_cnt;
@@ -72,6 +73,7 @@ extern struct test_env env;
 
 extern void test__force_log();
 extern bool test__start_subtest(const char *name);
+extern void test__skip(void);
 
 #define MAGIC_BYTES 123
 
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH bpf-next 3/4] selftests/bpf: test_progs: remove global fail/success counts
  2019-08-14 16:47 [PATCH bpf-next 0/4] selftests/bpf: test_progs: misc fixes Stanislav Fomichev
  2019-08-14 16:47 ` [PATCH bpf-next 1/4] selftests/bpf: test_progs: change formatting of the condenced output Stanislav Fomichev
  2019-08-14 16:47 ` [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip Stanislav Fomichev
@ 2019-08-14 16:47 ` Stanislav Fomichev
  2019-08-14 19:47   ` Andrii Nakryiko
  2019-08-14 16:47 ` [PATCH bpf-next 4/4] selftests/bpf: test_progs: remove asserts from subtests Stanislav Fomichev
  3 siblings, 1 reply; 17+ messages in thread
From: Stanislav Fomichev @ 2019-08-14 16:47 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko

Now that we have a global per-test/per-environment state, there
is no longer the need to have global fail/success counters
(and there is no need to save/get the diff before/after the
test).

Cc: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 .../selftests/bpf/prog_tests/bpf_obj_id.c     |  2 +-
 .../bpf/prog_tests/bpf_verif_scale.c          | 10 +---
 .../selftests/bpf/prog_tests/flow_dissector.c |  2 +-
 .../bpf/prog_tests/get_stack_raw_tp.c         |  2 +-
 .../selftests/bpf/prog_tests/global_data.c    | 10 ++--
 .../selftests/bpf/prog_tests/l4lb_all.c       |  4 +-
 .../selftests/bpf/prog_tests/map_lock.c       |  8 +--
 .../selftests/bpf/prog_tests/pkt_access.c     |  2 +-
 .../selftests/bpf/prog_tests/pkt_md_access.c  |  2 +-
 .../bpf/prog_tests/queue_stack_map.c          |  4 +-
 .../bpf/prog_tests/reference_tracking.c       |  2 +-
 .../selftests/bpf/prog_tests/spinlock.c       |  2 +-
 .../selftests/bpf/prog_tests/stacktrace_map.c |  2 +-
 .../bpf/prog_tests/stacktrace_map_raw_tp.c    |  2 +-
 .../bpf/prog_tests/task_fd_query_rawtp.c      |  2 +-
 .../bpf/prog_tests/task_fd_query_tp.c         |  2 +-
 .../selftests/bpf/prog_tests/tcp_estats.c     |  2 +-
 tools/testing/selftests/bpf/prog_tests/xdp.c  |  2 +-
 .../bpf/prog_tests/xdp_adjust_tail.c          |  2 +-
 .../selftests/bpf/prog_tests/xdp_noinline.c   |  4 +-
 tools/testing/selftests/bpf/test_progs.c      | 55 ++++++++-----------
 tools/testing/selftests/bpf/test_progs.h      | 26 +++++++--
 22 files changed, 75 insertions(+), 74 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
index fb5840a62548..f57e0c625de3 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
@@ -49,7 +49,7 @@ void test_bpf_obj_id(void)
 		 * to load.
 		 */
 		if (err)
-			error_cnt++;
+			test__fail();
 		assert(!err);
 
 		/* Insert a magic value to the map */
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index 1a1eae356f81..217988243077 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
@@ -28,8 +28,6 @@ static int check_load(const char *file, enum bpf_prog_type type)
 	attr.prog_flags = BPF_F_TEST_RND_HI32;
 	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
 	bpf_object__close(obj);
-	if (err)
-		error_cnt++;
 	return err;
 }
 
@@ -105,12 +103,8 @@ void test_bpf_verif_scale(void)
 			continue;
 
 		err = check_load(test->file, test->attach_type);
-		if (test->fails) { /* expected to fail */
-			if (err)
-				error_cnt--;
-			else
-				error_cnt++;
-		}
+		if (err && !test->fails)
+			test__fail();
 	}
 
 	if (env.verifier_stats)
diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index 6892b88ae065..e9d882c05ded 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -453,7 +453,7 @@ void test_flow_dissector(void)
 	err = bpf_flow_load(&obj, "./bpf_flow.o", "flow_dissector",
 			    "jmp_table", "last_dissection", &prog_fd, &keys_fd);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
index 3d59b3c841fe..afc60f62e2a8 100644
--- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
@@ -137,7 +137,7 @@ void test_get_stack_raw_tp(void)
 
 	goto close_prog_noerr;
 close_prog:
-	error_cnt++;
+	test__fail();
 close_prog_noerr:
 	if (!IS_ERR_OR_NULL(link))
 		bpf_link__destroy(link);
diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
index d011079fb0bf..db13bfee6bb9 100644
--- a/tools/testing/selftests/bpf/prog_tests/global_data.c
+++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
@@ -8,7 +8,7 @@ static void test_global_data_number(struct bpf_object *obj, __u32 duration)
 
 	map_fd = bpf_find_map(__func__, obj, "result_number");
 	if (map_fd < 0) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
@@ -45,7 +45,7 @@ static void test_global_data_string(struct bpf_object *obj, __u32 duration)
 
 	map_fd = bpf_find_map(__func__, obj, "result_string");
 	if (map_fd < 0) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
@@ -82,7 +82,7 @@ static void test_global_data_struct(struct bpf_object *obj, __u32 duration)
 
 	map_fd = bpf_find_map(__func__, obj, "result_struct");
 	if (map_fd < 0) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
@@ -113,13 +113,13 @@ static void test_global_data_rdonly(struct bpf_object *obj, __u32 duration)
 
 	map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
 	if (!map || !bpf_map__is_internal(map)) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
 	map_fd = bpf_map__fd(map);
 	if (map_fd < 0) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
index 20ddca830e68..724bb40de1f8 100644
--- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
+++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
@@ -31,7 +31,7 @@ static void test_l4lb(const char *file)
 
 	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
@@ -73,7 +73,7 @@ static void test_l4lb(const char *file)
 		pkts += stats[i].pkts;
 	}
 	if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
-		error_cnt++;
+		test__fail();
 		printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
 	}
 out:
diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
index ee99368c595c..12123ff1f31f 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
@@ -10,12 +10,12 @@ static void *parallel_map_access(void *arg)
 		err = bpf_map_lookup_elem_flags(map_fd, &key, vars, BPF_F_LOCK);
 		if (err) {
 			printf("lookup failed\n");
-			error_cnt++;
+			test__fail();
 			goto out;
 		}
 		if (vars[0] != 0) {
 			printf("lookup #%d var[0]=%d\n", i, vars[0]);
-			error_cnt++;
+			test__fail();
 			goto out;
 		}
 		rnd = vars[1];
@@ -24,7 +24,7 @@ static void *parallel_map_access(void *arg)
 				continue;
 			printf("lookup #%d var[1]=%d var[%d]=%d\n",
 			       i, rnd, j, vars[j]);
-			error_cnt++;
+			test__fail();
 			goto out;
 		}
 	}
@@ -69,7 +69,7 @@ void test_map_lock(void)
 		       ret == (void *)&map_fd[i - 4]);
 	goto close_prog_noerr;
 close_prog:
-	error_cnt++;
+	test__fail();
 close_prog_noerr:
 	bpf_object__close(obj);
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
index 4ecfd721a044..9ef4e4ffb379 100644
--- a/tools/testing/selftests/bpf/prog_tests/pkt_access.c
+++ b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
@@ -10,7 +10,7 @@ void test_pkt_access(void)
 
 	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
index ac0d43435806..c354b9d21f4f 100644
--- a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
+++ b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
@@ -10,7 +10,7 @@ void test_pkt_md_access(void)
 
 	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
index e60cd5ff1f55..48a8cd144bd1 100644
--- a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
+++ b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
@@ -28,7 +28,7 @@ static void test_queue_stack_map_by_type(int type)
 
 	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
@@ -44,7 +44,7 @@ static void test_queue_stack_map_by_type(int type)
 	for (i = 0; i < MAP_SIZE; i++) {
 		err = bpf_map_update_elem(map_in_fd, NULL, &vals[i], 0);
 		if (err) {
-			error_cnt++;
+			test__fail();
 			goto out;
 		}
 	}
diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
index 4a4f428d1a78..f6987e3dd28c 100644
--- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
+++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
@@ -11,7 +11,7 @@ void test_reference_tracking(void)
 
 	obj = bpf_object__open(file);
 	if (IS_ERR(obj)) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
index 114ebe6a438e..e843336713e8 100644
--- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
@@ -23,7 +23,7 @@ void test_spinlock(void)
 		       ret == (void *)&prog_fd);
 	goto close_prog_noerr;
 close_prog:
-	error_cnt++;
+	test__fail();
 close_prog_noerr:
 	bpf_object__close(obj);
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
index fc539335c5b3..9dba1cc3da60 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
@@ -70,7 +70,7 @@ void test_stacktrace_map(void)
 
 	goto disable_pmu_noerr;
 disable_pmu:
-	error_cnt++;
+	test__fail();
 disable_pmu_noerr:
 	bpf_link__destroy(link);
 close_prog:
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
index fbfa8e76cf63..4e7cf2e663f7 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
@@ -60,7 +60,7 @@ void test_stacktrace_map_raw_tp(void)
 
 	goto close_prog_noerr;
 close_prog:
-	error_cnt++;
+	test__fail();
 close_prog_noerr:
 	if (!IS_ERR_OR_NULL(link))
 		bpf_link__destroy(link);
diff --git a/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c b/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
index 958a3d88de99..d9ad1aa8a026 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
@@ -72,7 +72,7 @@ void test_task_fd_query_rawtp(void)
 
 	goto close_prog_noerr;
 close_prog:
-	error_cnt++;
+	test__fail();
 close_prog_noerr:
 	bpf_object__close(obj);
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
index f9b70e81682b..76209f2386c8 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
@@ -68,7 +68,7 @@ static void test_task_fd_query_tp_core(const char *probe_name,
 close_pmu:
 	close(pmu_fd);
 close_prog:
-	error_cnt++;
+	test__fail();
 close_prog_noerr:
 	bpf_object__close(obj);
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_estats.c b/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
index bb8759d69099..e241e5d7c71f 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
@@ -11,7 +11,7 @@ void test_tcp_estats(void)
 	err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
 	CHECK(err, "", "err %d errno %d\n", err, errno);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp.c b/tools/testing/selftests/bpf/prog_tests/xdp.c
index a74167289545..7c9f89fa1d02 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp.c
@@ -17,7 +17,7 @@ void test_xdp(void)
 
 	err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
index 922aa0a19764..a479a3303c3b 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
@@ -11,7 +11,7 @@ void test_xdp_adjust_tail(void)
 
 	err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
index 15f7c272edb0..10bef9d5ab81 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
@@ -32,7 +32,7 @@ void test_xdp_noinline(void)
 
 	err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
 	if (err) {
-		error_cnt++;
+		test__fail();
 		return;
 	}
 
@@ -74,7 +74,7 @@ void test_xdp_noinline(void)
 		pkts += stats[i].pkts;
 	}
 	if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
-		error_cnt++;
+		test__fail();
 		printf("test_xdp_noinline:FAIL:stats %lld %lld\n",
 		       bytes, pkts);
 	}
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 1993f2ce0d23..ad90e45768ce 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -8,24 +8,6 @@
 
 /* defined in test_progs.h */
 struct test_env env;
-int error_cnt, pass_cnt;
-
-struct prog_test_def {
-	const char *test_name;
-	int test_num;
-	void (*run_test)(void);
-	bool force_log;
-	int pass_cnt;
-	int error_cnt;
-	bool tested;
-
-	const char *subtest_name;
-	int subtest_num;
-
-	/* store counts before subtest started */
-	int old_pass_cnt;
-	int old_error_cnt;
-};
 
 static bool should_run(struct test_selector *sel, int num, const char *name)
 {
@@ -74,14 +56,14 @@ static const char *test_status_string(bool success)
 void test__end_subtest()
 {
 	struct prog_test_def *test = env.test;
-	int sub_error_cnt = error_cnt - test->old_error_cnt;
+	int sub_fail_cnt = test->fail_cnt - test->old_fail_cnt;
 
-	if (sub_error_cnt)
-		env.fail_cnt++;
+	if (sub_fail_cnt)
+		test->fail_cnt++;
 	else
 		env.sub_succ_cnt++;
 
-	dump_test_log(test, sub_error_cnt);
+	dump_test_log(test, sub_fail_cnt);
 
 	fprintf(env.stdout, "#%3d/%-3d %4s %s:%s\n",
 		test->test_num, test->subtest_num,
@@ -111,8 +93,8 @@ bool test__start_subtest(const char *name)
 		return false;
 
 	test->subtest_name = name;
-	env.test->old_pass_cnt = pass_cnt;
-	env.test->old_error_cnt = error_cnt;
+	env.test->old_succ_cnt = env.test->succ_cnt;
+	env.test->old_fail_cnt = env.test->fail_cnt;
 
 	return true;
 }
@@ -126,6 +108,19 @@ void test__skip(void)
 	env.skip_cnt++;
 }
 
+void __test__fail(const char *file, int line)
+{
+	if (env.test->subtest_name)
+		fprintf(stderr, "%s:%s failed at %s:%d, errno=%d\n",
+			env.test->test_name, env.test->subtest_name,
+			file, line, errno);
+	else
+		fprintf(stderr, "%s failed at %s:%d, errno=%d\n",
+			env.test->test_name, file, line, errno);
+
+	env.test->fail_cnt++;
+}
+
 struct ipv4_packet pkt_v4 = {
 	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
 	.iph.ihl = 5,
@@ -150,7 +145,7 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name)
 	map = bpf_object__find_map_by_name(obj, name);
 	if (!map) {
 		printf("%s:FAIL:map '%s' not found\n", test, name);
-		error_cnt++;
+		test__fail();
 		return -1;
 	}
 	return bpf_map__fd(map);
@@ -509,8 +504,6 @@ int main(int argc, char **argv)
 	stdio_hijack();
 	for (i = 0; i < prog_test_cnt; i++) {
 		struct prog_test_def *test = &prog_test_defs[i];
-		int old_pass_cnt = pass_cnt;
-		int old_error_cnt = error_cnt;
 
 		env.test = test;
 		test->test_num = i + 1;
@@ -525,14 +518,12 @@ int main(int argc, char **argv)
 			test__end_subtest();
 
 		test->tested = true;
-		test->pass_cnt = pass_cnt - old_pass_cnt;
-		test->error_cnt = error_cnt - old_error_cnt;
-		if (test->error_cnt)
+		if (test->fail_cnt)
 			env.fail_cnt++;
 		else
 			env.succ_cnt++;
 
-		dump_test_log(test, test->error_cnt);
+		dump_test_log(test, test->fail_cnt);
 
 		fprintf(env.stdout, "#%3d     %4s %s\n",
 			test->test_num,
@@ -546,5 +537,5 @@ int main(int argc, char **argv)
 	free(env.test_selector.num_set);
 	free(env.subtest_selector.num_set);
 
-	return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
+	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
 }
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 9defd35cb6c0..7b05921784a4 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -38,7 +38,23 @@ typedef __u16 __sum16;
 #include "trace_helpers.h"
 #include "flow_dissector_load.h"
 
-struct prog_test_def;
+struct prog_test_def {
+	const char *test_name;
+	int test_num;
+	void (*run_test)(void);
+	bool force_log;
+	bool tested;
+
+	const char *subtest_name;
+	int subtest_num;
+
+	int succ_cnt;
+	int fail_cnt;
+
+	/* store counts before subtest started */
+	int old_succ_cnt;
+	int old_fail_cnt;
+};
 
 struct test_selector {
 	const char *name;
@@ -67,13 +83,13 @@ struct test_env {
 	int skip_cnt; /* skipped tests */
 };
 
-extern int error_cnt;
-extern int pass_cnt;
 extern struct test_env env;
 
 extern void test__force_log();
 extern bool test__start_subtest(const char *name);
 extern void test__skip(void);
+#define test__fail() __test__fail(__FILE__, __LINE__)
+extern void __test__fail(const char *file, int line);
 
 #define MAGIC_BYTES 123
 
@@ -96,11 +112,11 @@ extern struct ipv6_packet pkt_v6;
 #define _CHECK(condition, tag, duration, format...) ({			\
 	int __ret = !!(condition);					\
 	if (__ret) {							\
-		error_cnt++;						\
+		test__fail();						\
 		printf("%s:FAIL:%s ", __func__, tag);			\
 		printf(format);						\
 	} else {							\
-		pass_cnt++;						\
+		env.test->succ_cnt++;					\
 		printf("%s:PASS:%s %d nsec\n",				\
 		       __func__, tag, duration);			\
 	}								\
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH bpf-next 4/4] selftests/bpf: test_progs: remove asserts from subtests
  2019-08-14 16:47 [PATCH bpf-next 0/4] selftests/bpf: test_progs: misc fixes Stanislav Fomichev
                   ` (2 preceding siblings ...)
  2019-08-14 16:47 ` [PATCH bpf-next 3/4] selftests/bpf: test_progs: remove global fail/success counts Stanislav Fomichev
@ 2019-08-14 16:47 ` Stanislav Fomichev
  2019-08-14 19:52   ` Andrii Nakryiko
  3 siblings, 1 reply; 17+ messages in thread
From: Stanislav Fomichev @ 2019-08-14 16:47 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko

Otherwise they can bring the whole process down.

Cc: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 .../selftests/bpf/prog_tests/bpf_obj_id.c     | 30 ++++++++++++++-----
 .../selftests/bpf/prog_tests/map_lock.c       | 20 ++++++++-----
 .../selftests/bpf/prog_tests/spinlock.c       | 10 ++++---
 .../bpf/prog_tests/stacktrace_build_id.c      | 11 +++++--
 .../bpf/prog_tests/stacktrace_build_id_nmi.c  | 11 +++++--
 5 files changed, 57 insertions(+), 25 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
index f57e0c625de3..4ec8c4e9e9a1 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
@@ -48,16 +48,23 @@ void test_bpf_obj_id(void)
 		/* test_obj_id.o is a dumb prog. It should never fail
 		 * to load.
 		 */
-		if (err)
+		if (err) {
 			test__fail();
-		assert(!err);
+			continue;
+		}
 
 		/* Insert a magic value to the map */
 		map_fds[i] = bpf_find_map(__func__, objs[i], "test_map_id");
-		assert(map_fds[i] >= 0);
+		if (map_fds[i] < 0) {
+			test__fail();
+			goto done;
+		}
 		err = bpf_map_update_elem(map_fds[i], &array_key,
 					  &array_magic_value, 0);
-		assert(!err);
+		if (err) {
+			test__fail();
+			goto done;
+		}
 
 		/* Check getting map info */
 		info_len = sizeof(struct bpf_map_info) * 2;
@@ -96,9 +103,15 @@ void test_bpf_obj_id(void)
 		prog_infos[i].map_ids = ptr_to_u64(map_ids + i);
 		prog_infos[i].nr_map_ids = 2;
 		err = clock_gettime(CLOCK_REALTIME, &real_time_ts);
-		assert(!err);
+		if (err) {
+			test__fail();
+			goto done;
+		}
 		err = clock_gettime(CLOCK_BOOTTIME, &boot_time_ts);
-		assert(!err);
+		if (err) {
+			test__fail();
+			goto done;
+		}
 		err = bpf_obj_get_info_by_fd(prog_fds[i], &prog_infos[i],
 					     &info_len);
 		load_time = (real_time_ts.tv_sec - boot_time_ts.tv_sec)
@@ -224,7 +237,10 @@ void test_bpf_obj_id(void)
 		nr_id_found++;
 
 		err = bpf_map_lookup_elem(map_fd, &array_key, &array_value);
-		assert(!err);
+		if (err) {
+			test__fail();
+			goto done;
+		}
 
 		err = bpf_obj_get_info_by_fd(map_fd, &map_info, &info_len);
 		CHECK(err || info_len != sizeof(struct bpf_map_info) ||
diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
index 12123ff1f31f..e7663721fb57 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
@@ -56,17 +56,21 @@ void test_map_lock(void)
 	bpf_map_update_elem(map_fd[0], &key, vars, BPF_F_LOCK);
 
 	for (i = 0; i < 4; i++)
-		assert(pthread_create(&thread_id[i], NULL,
-				      &spin_lock_thread, &prog_fd) == 0);
+		if (pthread_create(&thread_id[i], NULL,
+				   &spin_lock_thread, &prog_fd))
+			goto close_prog;
 	for (i = 4; i < 6; i++)
-		assert(pthread_create(&thread_id[i], NULL,
-				      &parallel_map_access, &map_fd[i - 4]) == 0);
+		if (pthread_create(&thread_id[i], NULL,
+				   &parallel_map_access, &map_fd[i - 4]))
+			goto close_prog;
 	for (i = 0; i < 4; i++)
-		assert(pthread_join(thread_id[i], &ret) == 0 &&
-		       ret == (void *)&prog_fd);
+		if (pthread_join(thread_id[i], &ret) ||
+		    ret != (void *)&prog_fd)
+			goto close_prog;
 	for (i = 4; i < 6; i++)
-		assert(pthread_join(thread_id[i], &ret) == 0 &&
-		       ret == (void *)&map_fd[i - 4]);
+		if (pthread_join(thread_id[i], &ret) ||
+		    ret != (void *)&map_fd[i - 4])
+			goto close_prog;
 	goto close_prog_noerr;
 close_prog:
 	test__fail();
diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
index e843336713e8..5f32a913f732 100644
--- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
@@ -16,11 +16,13 @@ void test_spinlock(void)
 		goto close_prog;
 	}
 	for (i = 0; i < 4; i++)
-		assert(pthread_create(&thread_id[i], NULL,
-				      &spin_lock_thread, &prog_fd) == 0);
+		if (pthread_create(&thread_id[i], NULL,
+				   &spin_lock_thread, &prog_fd))
+			goto close_prog;
+
 	for (i = 0; i < 4; i++)
-		assert(pthread_join(thread_id[i], &ret) == 0 &&
-		       ret == (void *)&prog_fd);
+		if (pthread_join(thread_id[i], &ret) || ret != (void *)&prog_fd)
+			goto close_prog;
 	goto close_prog_noerr;
 close_prog:
 	test__fail();
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
index ac44fda84833..d74464faebd7 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
@@ -51,9 +51,14 @@ void test_stacktrace_build_id(void)
 		  "err %d errno %d\n", err, errno))
 		goto disable_pmu;
 
-	assert(system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")
-	       == 0);
-	assert(system("./urandom_read") == 0);
+	if (system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")) {
+		test__fail();
+		goto disable_pmu;
+	}
+	if (system("./urandom_read")) {
+		test__fail();
+		goto disable_pmu;
+	}
 	/* disable stack trace collection */
 	key = 0;
 	val = 1;
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
index 9557b7dfb782..e886911928bc 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
@@ -82,9 +82,14 @@ void test_stacktrace_build_id_nmi(void)
 		  "err %d errno %d\n", err, errno))
 		goto disable_pmu;
 
-	assert(system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")
-	       == 0);
-	assert(system("taskset 0x1 ./urandom_read 100000") == 0);
+	if (system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")) {
+		test__fail();
+		goto disable_pmu;
+	}
+	if (system("taskset 0x1 ./urandom_read 100000")) {
+		test__fail();
+		goto disable_pmu;
+	}
 	/* disable stack trace collection */
 	key = 0;
 	val = 1;
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 1/4] selftests/bpf: test_progs: change formatting of the condenced output
  2019-08-14 16:47 ` [PATCH bpf-next 1/4] selftests/bpf: test_progs: change formatting of the condenced output Stanislav Fomichev
@ 2019-08-14 17:00   ` Alexei Starovoitov
  2019-08-14 17:07     ` Stanislav Fomichev
  0 siblings, 1 reply; 17+ messages in thread
From: Alexei Starovoitov @ 2019-08-14 17:00 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Network Development, bpf, David S. Miller, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko

On Wed, Aug 14, 2019 at 9:47 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> This makes it visually simpler to follow the output.
> Also, highlight with red color failures when outputting to tty.
>
> Before:
>   #1 attach_probe:FAIL
>   #2 bpf_obj_id:OK
>   #3/1 bpf_verif_scale:loop3.o:OK
>   #3/2 bpf_verif_scale:test_verif_scale1.o:OK
>   #3/3 bpf_verif_scale:test_verif_scale2.o:OK
>   #3/4 bpf_verif_scale:test_verif_scale3.o:OK
>   #3/5 bpf_verif_scale:pyperf50.o:OK
>   #3/6 bpf_verif_scale:pyperf100.o:OK
>   #3/7 bpf_verif_scale:pyperf180.o:OK
>   #3/8 bpf_verif_scale:pyperf600.o:OK
>   #3/9 bpf_verif_scale:pyperf600_nounroll.o:OK
>   #3/10 bpf_verif_scale:loop1.o:OK
>   #3/11 bpf_verif_scale:loop2.o:OK
>   #3/12 bpf_verif_scale:loop4.o:OK
>   #3/13 bpf_verif_scale:loop5.o:OK
>   #3/14 bpf_verif_scale:strobemeta.o:OK
>   #3/15 bpf_verif_scale:strobemeta_nounroll1.o:OK
>   #3/16 bpf_verif_scale:strobemeta_nounroll2.o:OK
>   #3/17 bpf_verif_scale:test_sysctl_loop1.o:OK
>   #3/18 bpf_verif_scale:test_sysctl_loop2.o:OK
>   #3/19 bpf_verif_scale:test_xdp_loop.o:OK
>   #3/20 bpf_verif_scale:test_seg6_loop.o:OK
>   #3 bpf_verif_scale:OK
>   #4 flow_dissector:OK
>
> After:
>   #  1     FAIL attach_probe
>   #  2       OK bpf_obj_id
>   #  3/1     OK bpf_verif_scale:loop3.o
>   #  3/2     OK bpf_verif_scale:test_verif_scale1.o
>   #  3/3     OK bpf_verif_scale:test_verif_scale2.o
>   #  3/4     OK bpf_verif_scale:test_verif_scale3.o
>   #  3/5     OK bpf_verif_scale:pyperf50.o
>   #  3/6     OK bpf_verif_scale:pyperf100.o
>   #  3/7     OK bpf_verif_scale:pyperf180.o
>   #  3/8     OK bpf_verif_scale:pyperf600.o
>   #  3/9     OK bpf_verif_scale:pyperf600_nounroll.o
>   #  3/10    OK bpf_verif_scale:loop1.o
>   #  3/11    OK bpf_verif_scale:loop2.o
>   #  3/12    OK bpf_verif_scale:loop4.o
>   #  3/13    OK bpf_verif_scale:loop5.o
>   #  3/14    OK bpf_verif_scale:strobemeta.o
>   #  3/15    OK bpf_verif_scale:strobemeta_nounroll1.o
>   #  3/16    OK bpf_verif_scale:strobemeta_nounroll2.o
>   #  3/17    OK bpf_verif_scale:test_sysctl_loop1.o
>   #  3/18    OK bpf_verif_scale:test_sysctl_loop2.o
>   #  3/19    OK bpf_verif_scale:test_xdp_loop.o
>   #  3/20    OK bpf_verif_scale:test_seg6_loop.o
>   #  3       OK bpf_verif_scale
>   #  4       OK flow_dissector

sorry this is nack.
I prefer consistency with test_verifier output.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 1/4] selftests/bpf: test_progs: change formatting of the condenced output
  2019-08-14 17:00   ` Alexei Starovoitov
@ 2019-08-14 17:07     ` Stanislav Fomichev
  0 siblings, 0 replies; 17+ messages in thread
From: Stanislav Fomichev @ 2019-08-14 17:07 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Stanislav Fomichev, Network Development, bpf, David S. Miller,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On 08/14, Alexei Starovoitov wrote:
> On Wed, Aug 14, 2019 at 9:47 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > This makes it visually simpler to follow the output.
> > Also, highlight with red color failures when outputting to tty.
> >
> > Before:
> >   #1 attach_probe:FAIL
> >   #2 bpf_obj_id:OK
> >   #3/1 bpf_verif_scale:loop3.o:OK
> >   #3/2 bpf_verif_scale:test_verif_scale1.o:OK
> >   #3/3 bpf_verif_scale:test_verif_scale2.o:OK
> >   #3/4 bpf_verif_scale:test_verif_scale3.o:OK
> >   #3/5 bpf_verif_scale:pyperf50.o:OK
> >   #3/6 bpf_verif_scale:pyperf100.o:OK
> >   #3/7 bpf_verif_scale:pyperf180.o:OK
> >   #3/8 bpf_verif_scale:pyperf600.o:OK
> >   #3/9 bpf_verif_scale:pyperf600_nounroll.o:OK
> >   #3/10 bpf_verif_scale:loop1.o:OK
> >   #3/11 bpf_verif_scale:loop2.o:OK
> >   #3/12 bpf_verif_scale:loop4.o:OK
> >   #3/13 bpf_verif_scale:loop5.o:OK
> >   #3/14 bpf_verif_scale:strobemeta.o:OK
> >   #3/15 bpf_verif_scale:strobemeta_nounroll1.o:OK
> >   #3/16 bpf_verif_scale:strobemeta_nounroll2.o:OK
> >   #3/17 bpf_verif_scale:test_sysctl_loop1.o:OK
> >   #3/18 bpf_verif_scale:test_sysctl_loop2.o:OK
> >   #3/19 bpf_verif_scale:test_xdp_loop.o:OK
> >   #3/20 bpf_verif_scale:test_seg6_loop.o:OK
> >   #3 bpf_verif_scale:OK
> >   #4 flow_dissector:OK
> >
> > After:
> >   #  1     FAIL attach_probe
> >   #  2       OK bpf_obj_id
> >   #  3/1     OK bpf_verif_scale:loop3.o
> >   #  3/2     OK bpf_verif_scale:test_verif_scale1.o
> >   #  3/3     OK bpf_verif_scale:test_verif_scale2.o
> >   #  3/4     OK bpf_verif_scale:test_verif_scale3.o
> >   #  3/5     OK bpf_verif_scale:pyperf50.o
> >   #  3/6     OK bpf_verif_scale:pyperf100.o
> >   #  3/7     OK bpf_verif_scale:pyperf180.o
> >   #  3/8     OK bpf_verif_scale:pyperf600.o
> >   #  3/9     OK bpf_verif_scale:pyperf600_nounroll.o
> >   #  3/10    OK bpf_verif_scale:loop1.o
> >   #  3/11    OK bpf_verif_scale:loop2.o
> >   #  3/12    OK bpf_verif_scale:loop4.o
> >   #  3/13    OK bpf_verif_scale:loop5.o
> >   #  3/14    OK bpf_verif_scale:strobemeta.o
> >   #  3/15    OK bpf_verif_scale:strobemeta_nounroll1.o
> >   #  3/16    OK bpf_verif_scale:strobemeta_nounroll2.o
> >   #  3/17    OK bpf_verif_scale:test_sysctl_loop1.o
> >   #  3/18    OK bpf_verif_scale:test_sysctl_loop2.o
> >   #  3/19    OK bpf_verif_scale:test_xdp_loop.o
> >   #  3/20    OK bpf_verif_scale:test_seg6_loop.o
> >   #  3       OK bpf_verif_scale
> >   #  4       OK flow_dissector
> 
> sorry this is nack.
> I prefer consistency with test_verifier output.
No problem, let me know how you feel about the other patches
in the series, can drop this one.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip
  2019-08-14 16:47 ` [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip Stanislav Fomichev
@ 2019-08-14 19:22   ` Andrii Nakryiko
  2019-08-14 19:30     ` Andrii Nakryiko
  0 siblings, 1 reply; 17+ messages in thread
From: Andrii Nakryiko @ 2019-08-14 19:22 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, David S. Miller, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko

On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Export test__skip() to indicate skipped tests and use it in
> test_send_signal_nmi().
>
> Cc: Andrii Nakryiko <andriin@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---

For completeness, we should probably also support test__skip_subtest()
eventually, but it's fine until we don't have a use case.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 +
>  tools/testing/selftests/bpf/test_progs.c             | 9 +++++++--
>  tools/testing/selftests/bpf/test_progs.h             | 2 ++
>  3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 1575f0a1f586..40c2c5efdd3e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void)
>                 if (errno == ENOENT) {
>                         printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
>                                __func__);
> +                       test__skip();
>                         return 0;
>                 }
>                 /* Let the test fail with a more informative message */
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 1a7a2a0c0a11..1993f2ce0d23 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -121,6 +121,11 @@ void test__force_log() {
>         env.test->force_log = true;
>  }
>
> +void test__skip(void)
> +{
> +       env.skip_cnt++;
> +}
> +
>  struct ipv4_packet pkt_v4 = {
>         .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
>         .iph.ihl = 5,
> @@ -535,8 +540,8 @@ int main(int argc, char **argv)
>                         test->test_name);
>         }
>         stdio_restore();
> -       printf("Summary: %d/%d PASSED, %d FAILED\n",
> -              env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
> +       printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> +              env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
>
>         free(env.test_selector.num_set);
>         free(env.subtest_selector.num_set);
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 37d427f5a1e5..9defd35cb6c0 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -64,6 +64,7 @@ struct test_env {
>         int succ_cnt; /* successful tests */
>         int sub_succ_cnt; /* successful sub-tests */
>         int fail_cnt; /* total failed tests + sub-tests */
> +       int skip_cnt; /* skipped tests */
>  };
>
>  extern int error_cnt;
> @@ -72,6 +73,7 @@ extern struct test_env env;
>
>  extern void test__force_log();
>  extern bool test__start_subtest(const char *name);
> +extern void test__skip(void);
>
>  #define MAGIC_BYTES 123
>
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip
  2019-08-14 19:22   ` Andrii Nakryiko
@ 2019-08-14 19:30     ` Andrii Nakryiko
  2019-08-14 19:53       ` Stanislav Fomichev
  0 siblings, 1 reply; 17+ messages in thread
From: Andrii Nakryiko @ 2019-08-14 19:30 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, David S. Miller, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko

On Wed, Aug 14, 2019 at 12:22 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > Export test__skip() to indicate skipped tests and use it in
> > test_send_signal_nmi().
> >
> > Cc: Andrii Nakryiko <andriin@fb.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
>
> For completeness, we should probably also support test__skip_subtest()
> eventually, but it's fine until we don't have a use case.

Hm.. so I think we don't need separate test__skip_subtest().
test__skip() should skip either test or sub-test, depending on which
context we are running in. So maybe just make sure this is handled
correctly?

>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> >  tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 +
> >  tools/testing/selftests/bpf/test_progs.c             | 9 +++++++--
> >  tools/testing/selftests/bpf/test_progs.h             | 2 ++
> >  3 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > index 1575f0a1f586..40c2c5efdd3e 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void)
> >                 if (errno == ENOENT) {
> >                         printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
> >                                __func__);
> > +                       test__skip();
> >                         return 0;
> >                 }
> >                 /* Let the test fail with a more informative message */
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 1a7a2a0c0a11..1993f2ce0d23 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -121,6 +121,11 @@ void test__force_log() {
> >         env.test->force_log = true;
> >  }
> >
> > +void test__skip(void)
> > +{
> > +       env.skip_cnt++;
> > +}
> > +
> >  struct ipv4_packet pkt_v4 = {
> >         .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> >         .iph.ihl = 5,
> > @@ -535,8 +540,8 @@ int main(int argc, char **argv)
> >                         test->test_name);
> >         }
> >         stdio_restore();
> > -       printf("Summary: %d/%d PASSED, %d FAILED\n",
> > -              env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
> > +       printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",

So because some sub-tests might be skipped, while others will be
running, let's keep output consistent with SUCCESS and use
<test>/<subtests> format for SKIPPED as well?

> > +              env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
> >
> >         free(env.test_selector.num_set);
> >         free(env.subtest_selector.num_set);
> > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > index 37d427f5a1e5..9defd35cb6c0 100644
> > --- a/tools/testing/selftests/bpf/test_progs.h
> > +++ b/tools/testing/selftests/bpf/test_progs.h
> > @@ -64,6 +64,7 @@ struct test_env {
> >         int succ_cnt; /* successful tests */
> >         int sub_succ_cnt; /* successful sub-tests */
> >         int fail_cnt; /* total failed tests + sub-tests */
> > +       int skip_cnt; /* skipped tests */
> >  };
> >
> >  extern int error_cnt;
> > @@ -72,6 +73,7 @@ extern struct test_env env;
> >
> >  extern void test__force_log();
> >  extern bool test__start_subtest(const char *name);
> > +extern void test__skip(void);
> >
> >  #define MAGIC_BYTES 123
> >
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 3/4] selftests/bpf: test_progs: remove global fail/success counts
  2019-08-14 16:47 ` [PATCH bpf-next 3/4] selftests/bpf: test_progs: remove global fail/success counts Stanislav Fomichev
@ 2019-08-14 19:47   ` Andrii Nakryiko
  2019-08-14 20:07     ` Stanislav Fomichev
  0 siblings, 1 reply; 17+ messages in thread
From: Andrii Nakryiko @ 2019-08-14 19:47 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, David S. Miller, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko

On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Now that we have a global per-test/per-environment state, there
> is no longer the need to have global fail/success counters
> (and there is no need to save/get the diff before/after the
> test).
>
> Cc: Andrii Nakryiko <andriin@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  .../selftests/bpf/prog_tests/bpf_obj_id.c     |  2 +-
>  .../bpf/prog_tests/bpf_verif_scale.c          | 10 +---
>  .../selftests/bpf/prog_tests/flow_dissector.c |  2 +-
>  .../bpf/prog_tests/get_stack_raw_tp.c         |  2 +-
>  .../selftests/bpf/prog_tests/global_data.c    | 10 ++--
>  .../selftests/bpf/prog_tests/l4lb_all.c       |  4 +-
>  .../selftests/bpf/prog_tests/map_lock.c       |  8 +--
>  .../selftests/bpf/prog_tests/pkt_access.c     |  2 +-
>  .../selftests/bpf/prog_tests/pkt_md_access.c  |  2 +-
>  .../bpf/prog_tests/queue_stack_map.c          |  4 +-
>  .../bpf/prog_tests/reference_tracking.c       |  2 +-
>  .../selftests/bpf/prog_tests/spinlock.c       |  2 +-
>  .../selftests/bpf/prog_tests/stacktrace_map.c |  2 +-
>  .../bpf/prog_tests/stacktrace_map_raw_tp.c    |  2 +-
>  .../bpf/prog_tests/task_fd_query_rawtp.c      |  2 +-
>  .../bpf/prog_tests/task_fd_query_tp.c         |  2 +-
>  .../selftests/bpf/prog_tests/tcp_estats.c     |  2 +-
>  tools/testing/selftests/bpf/prog_tests/xdp.c  |  2 +-
>  .../bpf/prog_tests/xdp_adjust_tail.c          |  2 +-
>  .../selftests/bpf/prog_tests/xdp_noinline.c   |  4 +-
>  tools/testing/selftests/bpf/test_progs.c      | 55 ++++++++-----------
>  tools/testing/selftests/bpf/test_progs.h      | 26 +++++++--
>  22 files changed, 75 insertions(+), 74 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> index fb5840a62548..f57e0c625de3 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> @@ -49,7 +49,7 @@ void test_bpf_obj_id(void)
>                  * to load.
>                  */
>                 if (err)
> -                       error_cnt++;
> +                       test__fail();
>                 assert(!err);
>
>                 /* Insert a magic value to the map */
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> index 1a1eae356f81..217988243077 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> @@ -28,8 +28,6 @@ static int check_load(const char *file, enum bpf_prog_type type)
>         attr.prog_flags = BPF_F_TEST_RND_HI32;
>         err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
>         bpf_object__close(obj);
> -       if (err)
> -               error_cnt++;
>         return err;
>  }
>
> @@ -105,12 +103,8 @@ void test_bpf_verif_scale(void)
>                         continue;
>
>                 err = check_load(test->file, test->attach_type);
> -               if (test->fails) { /* expected to fail */
> -                       if (err)
> -                               error_cnt--;
> -                       else
> -                               error_cnt++;
> -               }
> +               if (err && !test->fails)
> +                       test__fail();
>         }
>
>         if (env.verifier_stats)
> diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> index 6892b88ae065..e9d882c05ded 100644
> --- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> +++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> @@ -453,7 +453,7 @@ void test_flow_dissector(void)
>         err = bpf_flow_load(&obj, "./bpf_flow.o", "flow_dissector",
>                             "jmp_table", "last_dissection", &prog_fd, &keys_fd);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> index 3d59b3c841fe..afc60f62e2a8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> @@ -137,7 +137,7 @@ void test_get_stack_raw_tp(void)
>
>         goto close_prog_noerr;
>  close_prog:
> -       error_cnt++;
> +       test__fail();
>  close_prog_noerr:
>         if (!IS_ERR_OR_NULL(link))
>                 bpf_link__destroy(link);
> diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
> index d011079fb0bf..db13bfee6bb9 100644
> --- a/tools/testing/selftests/bpf/prog_tests/global_data.c
> +++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
> @@ -8,7 +8,7 @@ static void test_global_data_number(struct bpf_object *obj, __u32 duration)
>
>         map_fd = bpf_find_map(__func__, obj, "result_number");
>         if (map_fd < 0) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> @@ -45,7 +45,7 @@ static void test_global_data_string(struct bpf_object *obj, __u32 duration)
>
>         map_fd = bpf_find_map(__func__, obj, "result_string");
>         if (map_fd < 0) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> @@ -82,7 +82,7 @@ static void test_global_data_struct(struct bpf_object *obj, __u32 duration)
>
>         map_fd = bpf_find_map(__func__, obj, "result_struct");
>         if (map_fd < 0) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> @@ -113,13 +113,13 @@ static void test_global_data_rdonly(struct bpf_object *obj, __u32 duration)
>
>         map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
>         if (!map || !bpf_map__is_internal(map)) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
>         map_fd = bpf_map__fd(map);
>         if (map_fd < 0) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> index 20ddca830e68..724bb40de1f8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> +++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> @@ -31,7 +31,7 @@ static void test_l4lb(const char *file)
>
>         err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> @@ -73,7 +73,7 @@ static void test_l4lb(const char *file)
>                 pkts += stats[i].pkts;
>         }
>         if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
> -               error_cnt++;
> +               test__fail();
>                 printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
>         }
>  out:
> diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> index ee99368c595c..12123ff1f31f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> @@ -10,12 +10,12 @@ static void *parallel_map_access(void *arg)
>                 err = bpf_map_lookup_elem_flags(map_fd, &key, vars, BPF_F_LOCK);
>                 if (err) {
>                         printf("lookup failed\n");
> -                       error_cnt++;
> +                       test__fail();
>                         goto out;
>                 }
>                 if (vars[0] != 0) {
>                         printf("lookup #%d var[0]=%d\n", i, vars[0]);
> -                       error_cnt++;
> +                       test__fail();
>                         goto out;
>                 }
>                 rnd = vars[1];
> @@ -24,7 +24,7 @@ static void *parallel_map_access(void *arg)
>                                 continue;
>                         printf("lookup #%d var[1]=%d var[%d]=%d\n",
>                                i, rnd, j, vars[j]);
> -                       error_cnt++;
> +                       test__fail();
>                         goto out;
>                 }
>         }
> @@ -69,7 +69,7 @@ void test_map_lock(void)
>                        ret == (void *)&map_fd[i - 4]);
>         goto close_prog_noerr;
>  close_prog:
> -       error_cnt++;
> +       test__fail();
>  close_prog_noerr:
>         bpf_object__close(obj);
>  }
> diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
> index 4ecfd721a044..9ef4e4ffb379 100644
> --- a/tools/testing/selftests/bpf/prog_tests/pkt_access.c
> +++ b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
> @@ -10,7 +10,7 @@ void test_pkt_access(void)
>
>         err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
> index ac0d43435806..c354b9d21f4f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
> +++ b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
> @@ -10,7 +10,7 @@ void test_pkt_md_access(void)
>
>         err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
> index e60cd5ff1f55..48a8cd144bd1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
> +++ b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
> @@ -28,7 +28,7 @@ static void test_queue_stack_map_by_type(int type)
>
>         err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> @@ -44,7 +44,7 @@ static void test_queue_stack_map_by_type(int type)
>         for (i = 0; i < MAP_SIZE; i++) {
>                 err = bpf_map_update_elem(map_in_fd, NULL, &vals[i], 0);
>                 if (err) {
> -                       error_cnt++;
> +                       test__fail();
>                         goto out;
>                 }
>         }
> diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> index 4a4f428d1a78..f6987e3dd28c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> +++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> @@ -11,7 +11,7 @@ void test_reference_tracking(void)
>
>         obj = bpf_object__open(file);
>         if (IS_ERR(obj)) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
> index 114ebe6a438e..e843336713e8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
> +++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
> @@ -23,7 +23,7 @@ void test_spinlock(void)
>                        ret == (void *)&prog_fd);
>         goto close_prog_noerr;
>  close_prog:
> -       error_cnt++;
> +       test__fail();
>  close_prog_noerr:
>         bpf_object__close(obj);
>  }
> diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
> index fc539335c5b3..9dba1cc3da60 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
> @@ -70,7 +70,7 @@ void test_stacktrace_map(void)
>
>         goto disable_pmu_noerr;
>  disable_pmu:
> -       error_cnt++;
> +       test__fail();
>  disable_pmu_noerr:
>         bpf_link__destroy(link);
>  close_prog:
> diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
> index fbfa8e76cf63..4e7cf2e663f7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
> @@ -60,7 +60,7 @@ void test_stacktrace_map_raw_tp(void)
>
>         goto close_prog_noerr;
>  close_prog:
> -       error_cnt++;
> +       test__fail();
>  close_prog_noerr:
>         if (!IS_ERR_OR_NULL(link))
>                 bpf_link__destroy(link);
> diff --git a/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c b/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
> index 958a3d88de99..d9ad1aa8a026 100644
> --- a/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
> @@ -72,7 +72,7 @@ void test_task_fd_query_rawtp(void)
>
>         goto close_prog_noerr;
>  close_prog:
> -       error_cnt++;
> +       test__fail();
>  close_prog_noerr:
>         bpf_object__close(obj);
>  }
> diff --git a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
> index f9b70e81682b..76209f2386c8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
> @@ -68,7 +68,7 @@ static void test_task_fd_query_tp_core(const char *probe_name,
>  close_pmu:
>         close(pmu_fd);
>  close_prog:
> -       error_cnt++;
> +       test__fail();
>  close_prog_noerr:
>         bpf_object__close(obj);
>  }
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_estats.c b/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
> index bb8759d69099..e241e5d7c71f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
> @@ -11,7 +11,7 @@ void test_tcp_estats(void)
>         err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
>         CHECK(err, "", "err %d errno %d\n", err, errno);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp.c b/tools/testing/selftests/bpf/prog_tests/xdp.c
> index a74167289545..7c9f89fa1d02 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp.c
> @@ -17,7 +17,7 @@ void test_xdp(void)
>
>         err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> index 922aa0a19764..a479a3303c3b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> @@ -11,7 +11,7 @@ void test_xdp_adjust_tail(void)
>
>         err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> index 15f7c272edb0..10bef9d5ab81 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> @@ -32,7 +32,7 @@ void test_xdp_noinline(void)
>
>         err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
>         if (err) {
> -               error_cnt++;
> +               test__fail();
>                 return;
>         }
>
> @@ -74,7 +74,7 @@ void test_xdp_noinline(void)
>                 pkts += stats[i].pkts;
>         }
>         if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
> -               error_cnt++;
> +               test__fail();
>                 printf("test_xdp_noinline:FAIL:stats %lld %lld\n",
>                        bytes, pkts);
>         }
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 1993f2ce0d23..ad90e45768ce 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -8,24 +8,6 @@
>
>  /* defined in test_progs.h */
>  struct test_env env;
> -int error_cnt, pass_cnt;
> -
> -struct prog_test_def {
> -       const char *test_name;
> -       int test_num;
> -       void (*run_test)(void);
> -       bool force_log;
> -       int pass_cnt;
> -       int error_cnt;
> -       bool tested;
> -
> -       const char *subtest_name;
> -       int subtest_num;
> -
> -       /* store counts before subtest started */
> -       int old_pass_cnt;
> -       int old_error_cnt;
> -};
>
>  static bool should_run(struct test_selector *sel, int num, const char *name)
>  {
> @@ -74,14 +56,14 @@ static const char *test_status_string(bool success)
>  void test__end_subtest()
>  {
>         struct prog_test_def *test = env.test;
> -       int sub_error_cnt = error_cnt - test->old_error_cnt;
> +       int sub_fail_cnt = test->fail_cnt - test->old_fail_cnt;
>
> -       if (sub_error_cnt)
> -               env.fail_cnt++;
> +       if (sub_fail_cnt)
> +               test->fail_cnt++;
>         else
>                 env.sub_succ_cnt++;
>
> -       dump_test_log(test, sub_error_cnt);
> +       dump_test_log(test, sub_fail_cnt);
>
>         fprintf(env.stdout, "#%3d/%-3d %4s %s:%s\n",
>                 test->test_num, test->subtest_num,
> @@ -111,8 +93,8 @@ bool test__start_subtest(const char *name)
>                 return false;
>
>         test->subtest_name = name;
> -       env.test->old_pass_cnt = pass_cnt;
> -       env.test->old_error_cnt = error_cnt;
> +       env.test->old_succ_cnt = env.test->succ_cnt;
> +       env.test->old_fail_cnt = env.test->fail_cnt;
>
>         return true;
>  }
> @@ -126,6 +108,19 @@ void test__skip(void)
>         env.skip_cnt++;
>  }
>
> +void __test__fail(const char *file, int line)
> +{
> +       if (env.test->subtest_name)
> +               fprintf(stderr, "%s:%s failed at %s:%d, errno=%d\n",

Nit: let's keep <test>/<subtest> convention here as well?

Failure doesn't always set errno, this will be quite confusing
sometimes. Especially for higher-level libbpf APIs, which don't set
errno at all.
If test wants to log additional information, let it do it explicitly.

Also, _CHECK already logs error message, so this is going to be
double-verbose for typical case. I'd say let's drop these error
messages and instead slightly extend _CHECK one with line number (it
already logs func name).

> +                       env.test->test_name, env.test->subtest_name,
> +                       file, line, errno);
> +       else
> +               fprintf(stderr, "%s failed at %s:%d, errno=%d\n",
> +                       env.test->test_name, file, line, errno);
> +
> +       env.test->fail_cnt++;
> +}
> +
>  struct ipv4_packet pkt_v4 = {
>         .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
>         .iph.ihl = 5,
> @@ -150,7 +145,7 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name)
>         map = bpf_object__find_map_by_name(obj, name);
>         if (!map) {
>                 printf("%s:FAIL:map '%s' not found\n", test, name);
> -               error_cnt++;
> +               test__fail();
>                 return -1;
>         }
>         return bpf_map__fd(map);
> @@ -509,8 +504,6 @@ int main(int argc, char **argv)
>         stdio_hijack();
>         for (i = 0; i < prog_test_cnt; i++) {
>                 struct prog_test_def *test = &prog_test_defs[i];
> -               int old_pass_cnt = pass_cnt;
> -               int old_error_cnt = error_cnt;
>
>                 env.test = test;
>                 test->test_num = i + 1;
> @@ -525,14 +518,12 @@ int main(int argc, char **argv)
>                         test__end_subtest();
>
>                 test->tested = true;
> -               test->pass_cnt = pass_cnt - old_pass_cnt;
> -               test->error_cnt = error_cnt - old_error_cnt;
> -               if (test->error_cnt)
> +               if (test->fail_cnt)
>                         env.fail_cnt++;
>                 else
>                         env.succ_cnt++;
>
> -               dump_test_log(test, test->error_cnt);
> +               dump_test_log(test, test->fail_cnt);
>
>                 fprintf(env.stdout, "#%3d     %4s %s\n",
>                         test->test_num,
> @@ -546,5 +537,5 @@ int main(int argc, char **argv)
>         free(env.test_selector.num_set);
>         free(env.subtest_selector.num_set);
>
> -       return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
> +       return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
>  }
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 9defd35cb6c0..7b05921784a4 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -38,7 +38,23 @@ typedef __u16 __sum16;
>  #include "trace_helpers.h"
>  #include "flow_dissector_load.h"
>
> -struct prog_test_def;
> +struct prog_test_def {
> +       const char *test_name;
> +       int test_num;
> +       void (*run_test)(void);
> +       bool force_log;
> +       bool tested;
> +
> +       const char *subtest_name;
> +       int subtest_num;
> +
> +       int succ_cnt;
> +       int fail_cnt;

So I'm neutral on this rename, I even considered it myself initially.
But keep in mind, that succ/fail in env means number of tests, while
test->succ/fail means number of assertions. We don't report total
number of failed checks anymore, so it doesn't matter, but if we ever
want to keep track of that at env level, it will be very confusing and
inconvenient.

> +
> +       /* store counts before subtest started */
> +       int old_succ_cnt;
> +       int old_fail_cnt;
> +};

Did you move it here just to access env.test->succ_cnt in _CHECK()?
Maybe add test__success() counterpart to test__fail() instead?

>
>  struct test_selector {
>         const char *name;
> @@ -67,13 +83,13 @@ struct test_env {
>         int skip_cnt; /* skipped tests */
>  };
>
> -extern int error_cnt;
> -extern int pass_cnt;
>  extern struct test_env env;
>
>  extern void test__force_log();
>  extern bool test__start_subtest(const char *name);
>  extern void test__skip(void);
> +#define test__fail() __test__fail(__FILE__, __LINE__)
> +extern void __test__fail(const char *file, int line);

Given my comment above about too verbose logging, I'd say let's keep
this simple and have just

extern void test__fail()

And let _CHECK log file:line.

>
>  #define MAGIC_BYTES 123
>
> @@ -96,11 +112,11 @@ extern struct ipv6_packet pkt_v6;
>  #define _CHECK(condition, tag, duration, format...) ({                 \
>         int __ret = !!(condition);                                      \
>         if (__ret) {                                                    \
> -               error_cnt++;                                            \
> +               test__fail();                                           \
>                 printf("%s:FAIL:%s ", __func__, tag);                   \
>                 printf(format);                                         \
>         } else {                                                        \
> -               pass_cnt++;                                             \
> +               env.test->succ_cnt++;                                   \
>                 printf("%s:PASS:%s %d nsec\n",                          \
>                        __func__, tag, duration);                        \
>         }                                                               \
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 4/4] selftests/bpf: test_progs: remove asserts from subtests
  2019-08-14 16:47 ` [PATCH bpf-next 4/4] selftests/bpf: test_progs: remove asserts from subtests Stanislav Fomichev
@ 2019-08-14 19:52   ` Andrii Nakryiko
  0 siblings, 0 replies; 17+ messages in thread
From: Andrii Nakryiko @ 2019-08-14 19:52 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, David S. Miller, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko

On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Otherwise they can bring the whole process down.
>
> Cc: Andrii Nakryiko <andriin@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---

This is probably why you added all that extra logging in __test__fail(), right?

So had a low-priority TODO item to add another CHECK()-like macro that
would only report failure (but won't bump/log success). Seems like
this is something that would be useful for these asserts?

What do you think about either QCHECK() (for "quiet" check) or surely
we can also do ASSERT (but it's less obvious that it won't log success
and it's also not obvious that it won't actually terminate test
immediately).

Then inside that QCHECK() you can log file:line number, similar to
CHECK(), but only for failure case.

Thoughts?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip
  2019-08-14 19:30     ` Andrii Nakryiko
@ 2019-08-14 19:53       ` Stanislav Fomichev
  2019-08-14 20:01         ` Andrii Nakryiko
  0 siblings, 1 reply; 17+ messages in thread
From: Stanislav Fomichev @ 2019-08-14 19:53 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Stanislav Fomichev, Networking, bpf, David S. Miller,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On 08/14, Andrii Nakryiko wrote:
> On Wed, Aug 14, 2019 at 12:22 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > Export test__skip() to indicate skipped tests and use it in
> > > test_send_signal_nmi().
> > >
> > > Cc: Andrii Nakryiko <andriin@fb.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> >
> > For completeness, we should probably also support test__skip_subtest()
> > eventually, but it's fine until we don't have a use case.
> 
> Hm.. so I think we don't need separate test__skip_subtest().
> test__skip() should skip either test or sub-test, depending on which
> context we are running in. So maybe just make sure this is handled
> correctly?
Do we care if it's a test or a subtest skip? My motivation was to
have a counter that can be examined to make sure we have a full test
coverage, so when people run the tests they can be sure that nothing
is skipped due to missing config or something else.

Let me know if you see a value in highlighting test vs subtest skip.

Other related question is: should we do verbose output in case
of a skip? Right now we don't do it.

> >
> > Acked-by: Andrii Nakryiko <andriin@fb.com>
> >
> > >  tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 +
> > >  tools/testing/selftests/bpf/test_progs.c             | 9 +++++++--
> > >  tools/testing/selftests/bpf/test_progs.h             | 2 ++
> > >  3 files changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > index 1575f0a1f586..40c2c5efdd3e 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void)
> > >                 if (errno == ENOENT) {
> > >                         printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
> > >                                __func__);
> > > +                       test__skip();
> > >                         return 0;
> > >                 }
> > >                 /* Let the test fail with a more informative message */
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > index 1a7a2a0c0a11..1993f2ce0d23 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -121,6 +121,11 @@ void test__force_log() {
> > >         env.test->force_log = true;
> > >  }
> > >
> > > +void test__skip(void)
> > > +{
> > > +       env.skip_cnt++;
> > > +}
> > > +
> > >  struct ipv4_packet pkt_v4 = {
> > >         .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> > >         .iph.ihl = 5,
> > > @@ -535,8 +540,8 @@ int main(int argc, char **argv)
> > >                         test->test_name);
> > >         }
> > >         stdio_restore();
> > > -       printf("Summary: %d/%d PASSED, %d FAILED\n",
> > > -              env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
> > > +       printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> 
> So because some sub-tests might be skipped, while others will be
> running, let's keep output consistent with SUCCESS and use
> <test>/<subtests> format for SKIPPED as well?
> 
> > > +              env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
> > >
> > >         free(env.test_selector.num_set);
> > >         free(env.subtest_selector.num_set);
> > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > > index 37d427f5a1e5..9defd35cb6c0 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.h
> > > +++ b/tools/testing/selftests/bpf/test_progs.h
> > > @@ -64,6 +64,7 @@ struct test_env {
> > >         int succ_cnt; /* successful tests */
> > >         int sub_succ_cnt; /* successful sub-tests */
> > >         int fail_cnt; /* total failed tests + sub-tests */
> > > +       int skip_cnt; /* skipped tests */
> > >  };
> > >
> > >  extern int error_cnt;
> > > @@ -72,6 +73,7 @@ extern struct test_env env;
> > >
> > >  extern void test__force_log();
> > >  extern bool test__start_subtest(const char *name);
> > > +extern void test__skip(void);
> > >
> > >  #define MAGIC_BYTES 123
> > >
> > > --
> > > 2.23.0.rc1.153.gdeed80330f-goog
> > >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip
  2019-08-14 19:53       ` Stanislav Fomichev
@ 2019-08-14 20:01         ` Andrii Nakryiko
  2019-08-16  5:16           ` Alexei Starovoitov
  0 siblings, 1 reply; 17+ messages in thread
From: Andrii Nakryiko @ 2019-08-14 20:01 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Stanislav Fomichev, Networking, bpf, David S. Miller,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Wed, Aug 14, 2019 at 12:53 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 08/14, Andrii Nakryiko wrote:
> > On Wed, Aug 14, 2019 at 12:22 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
> > > >
> > > > Export test__skip() to indicate skipped tests and use it in
> > > > test_send_signal_nmi().
> > > >
> > > > Cc: Andrii Nakryiko <andriin@fb.com>
> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > ---
> > >
> > > For completeness, we should probably also support test__skip_subtest()
> > > eventually, but it's fine until we don't have a use case.
> >
> > Hm.. so I think we don't need separate test__skip_subtest().
> > test__skip() should skip either test or sub-test, depending on which
> > context we are running in. So maybe just make sure this is handled
> > correctly?
> Do we care if it's a test or a subtest skip? My motivation was to
> have a counter that can be examined to make sure we have a full test
> coverage, so when people run the tests they can be sure that nothing
> is skipped due to missing config or something else.

I think we do. We might convert, e.g., test_btf to be one big test
with lots of sub-tests. Some of those might be legitimately skipped.
Having just "1 test skipped" message is not helpful, when there are
170 sub-tests that were not.

>
> Let me know if you see a value in highlighting test vs subtest skip.
>
> Other related question is: should we do verbose output in case
> of a skip? Right now we don't do it.

It might be useful, I guess, especially if it's not too common. But
Alexei is way more picky about stuff like that, so I'd defer to him. I
have no problem with a clean "SKIPPED: <test>/<subtest> (maybe some
reason for skipping here)" message.

>
> > >
> > > Acked-by: Andrii Nakryiko <andriin@fb.com>
> > >
> > > >  tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 +
> > > >  tools/testing/selftests/bpf/test_progs.c             | 9 +++++++--
> > > >  tools/testing/selftests/bpf/test_progs.h             | 2 ++
> > > >  3 files changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > > index 1575f0a1f586..40c2c5efdd3e 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > > @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void)
> > > >                 if (errno == ENOENT) {
> > > >                         printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
> > > >                                __func__);
> > > > +                       test__skip();
> > > >                         return 0;
> > > >                 }
> > > >                 /* Let the test fail with a more informative message */
> > > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > > index 1a7a2a0c0a11..1993f2ce0d23 100644
> > > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > > @@ -121,6 +121,11 @@ void test__force_log() {
> > > >         env.test->force_log = true;
> > > >  }
> > > >
> > > > +void test__skip(void)
> > > > +{
> > > > +       env.skip_cnt++;
> > > > +}
> > > > +
> > > >  struct ipv4_packet pkt_v4 = {
> > > >         .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> > > >         .iph.ihl = 5,
> > > > @@ -535,8 +540,8 @@ int main(int argc, char **argv)
> > > >                         test->test_name);
> > > >         }
> > > >         stdio_restore();
> > > > -       printf("Summary: %d/%d PASSED, %d FAILED\n",
> > > > -              env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
> > > > +       printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> >
> > So because some sub-tests might be skipped, while others will be
> > running, let's keep output consistent with SUCCESS and use
> > <test>/<subtests> format for SKIPPED as well?
> >
> > > > +              env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
> > > >
> > > >         free(env.test_selector.num_set);
> > > >         free(env.subtest_selector.num_set);
> > > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > > > index 37d427f5a1e5..9defd35cb6c0 100644
> > > > --- a/tools/testing/selftests/bpf/test_progs.h
> > > > +++ b/tools/testing/selftests/bpf/test_progs.h
> > > > @@ -64,6 +64,7 @@ struct test_env {
> > > >         int succ_cnt; /* successful tests */
> > > >         int sub_succ_cnt; /* successful sub-tests */
> > > >         int fail_cnt; /* total failed tests + sub-tests */
> > > > +       int skip_cnt; /* skipped tests */
> > > >  };
> > > >
> > > >  extern int error_cnt;
> > > > @@ -72,6 +73,7 @@ extern struct test_env env;
> > > >
> > > >  extern void test__force_log();
> > > >  extern bool test__start_subtest(const char *name);
> > > > +extern void test__skip(void);
> > > >
> > > >  #define MAGIC_BYTES 123
> > > >
> > > > --
> > > > 2.23.0.rc1.153.gdeed80330f-goog
> > > >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 3/4] selftests/bpf: test_progs: remove global fail/success counts
  2019-08-14 19:47   ` Andrii Nakryiko
@ 2019-08-14 20:07     ` Stanislav Fomichev
  2019-08-14 20:22       ` Andrii Nakryiko
  0 siblings, 1 reply; 17+ messages in thread
From: Stanislav Fomichev @ 2019-08-14 20:07 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Stanislav Fomichev, Networking, bpf, David S. Miller,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On 08/14, Andrii Nakryiko wrote:
> On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > Now that we have a global per-test/per-environment state, there
> > is no longer the need to have global fail/success counters
> > (and there is no need to save/get the diff before/after the
> > test).
> >
> > Cc: Andrii Nakryiko <andriin@fb.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  .../selftests/bpf/prog_tests/bpf_obj_id.c     |  2 +-
> >  .../bpf/prog_tests/bpf_verif_scale.c          | 10 +---
> >  .../selftests/bpf/prog_tests/flow_dissector.c |  2 +-
> >  .../bpf/prog_tests/get_stack_raw_tp.c         |  2 +-
> >  .../selftests/bpf/prog_tests/global_data.c    | 10 ++--
> >  .../selftests/bpf/prog_tests/l4lb_all.c       |  4 +-
> >  .../selftests/bpf/prog_tests/map_lock.c       |  8 +--
> >  .../selftests/bpf/prog_tests/pkt_access.c     |  2 +-
> >  .../selftests/bpf/prog_tests/pkt_md_access.c  |  2 +-
> >  .../bpf/prog_tests/queue_stack_map.c          |  4 +-
> >  .../bpf/prog_tests/reference_tracking.c       |  2 +-
> >  .../selftests/bpf/prog_tests/spinlock.c       |  2 +-
> >  .../selftests/bpf/prog_tests/stacktrace_map.c |  2 +-
> >  .../bpf/prog_tests/stacktrace_map_raw_tp.c    |  2 +-
> >  .../bpf/prog_tests/task_fd_query_rawtp.c      |  2 +-
> >  .../bpf/prog_tests/task_fd_query_tp.c         |  2 +-
> >  .../selftests/bpf/prog_tests/tcp_estats.c     |  2 +-
> >  tools/testing/selftests/bpf/prog_tests/xdp.c  |  2 +-
> >  .../bpf/prog_tests/xdp_adjust_tail.c          |  2 +-
> >  .../selftests/bpf/prog_tests/xdp_noinline.c   |  4 +-
> >  tools/testing/selftests/bpf/test_progs.c      | 55 ++++++++-----------
> >  tools/testing/selftests/bpf/test_progs.h      | 26 +++++++--
> >  22 files changed, 75 insertions(+), 74 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> > index fb5840a62548..f57e0c625de3 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> > @@ -49,7 +49,7 @@ void test_bpf_obj_id(void)
> >                  * to load.
> >                  */
> >                 if (err)
> > -                       error_cnt++;
> > +                       test__fail();
> >                 assert(!err);
> >
> >                 /* Insert a magic value to the map */
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> > index 1a1eae356f81..217988243077 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> > @@ -28,8 +28,6 @@ static int check_load(const char *file, enum bpf_prog_type type)
> >         attr.prog_flags = BPF_F_TEST_RND_HI32;
> >         err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
> >         bpf_object__close(obj);
> > -       if (err)
> > -               error_cnt++;
> >         return err;
> >  }
> >
> > @@ -105,12 +103,8 @@ void test_bpf_verif_scale(void)
> >                         continue;
> >
> >                 err = check_load(test->file, test->attach_type);
> > -               if (test->fails) { /* expected to fail */
> > -                       if (err)
> > -                               error_cnt--;
> > -                       else
> > -                               error_cnt++;
> > -               }
> > +               if (err && !test->fails)
> > +                       test__fail();
> >         }
> >
> >         if (env.verifier_stats)
> > diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> > index 6892b88ae065..e9d882c05ded 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> > @@ -453,7 +453,7 @@ void test_flow_dissector(void)
> >         err = bpf_flow_load(&obj, "./bpf_flow.o", "flow_dissector",
> >                             "jmp_table", "last_dissection", &prog_fd, &keys_fd);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> > index 3d59b3c841fe..afc60f62e2a8 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> > @@ -137,7 +137,7 @@ void test_get_stack_raw_tp(void)
> >
> >         goto close_prog_noerr;
> >  close_prog:
> > -       error_cnt++;
> > +       test__fail();
> >  close_prog_noerr:
> >         if (!IS_ERR_OR_NULL(link))
> >                 bpf_link__destroy(link);
> > diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
> > index d011079fb0bf..db13bfee6bb9 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/global_data.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
> > @@ -8,7 +8,7 @@ static void test_global_data_number(struct bpf_object *obj, __u32 duration)
> >
> >         map_fd = bpf_find_map(__func__, obj, "result_number");
> >         if (map_fd < 0) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > @@ -45,7 +45,7 @@ static void test_global_data_string(struct bpf_object *obj, __u32 duration)
> >
> >         map_fd = bpf_find_map(__func__, obj, "result_string");
> >         if (map_fd < 0) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > @@ -82,7 +82,7 @@ static void test_global_data_struct(struct bpf_object *obj, __u32 duration)
> >
> >         map_fd = bpf_find_map(__func__, obj, "result_struct");
> >         if (map_fd < 0) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > @@ -113,13 +113,13 @@ static void test_global_data_rdonly(struct bpf_object *obj, __u32 duration)
> >
> >         map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
> >         if (!map || !bpf_map__is_internal(map)) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> >         map_fd = bpf_map__fd(map);
> >         if (map_fd < 0) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> > index 20ddca830e68..724bb40de1f8 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> > @@ -31,7 +31,7 @@ static void test_l4lb(const char *file)
> >
> >         err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > @@ -73,7 +73,7 @@ static void test_l4lb(const char *file)
> >                 pkts += stats[i].pkts;
> >         }
> >         if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
> > -               error_cnt++;
> > +               test__fail();
> >                 printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> >         }
> >  out:
> > diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > index ee99368c595c..12123ff1f31f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > @@ -10,12 +10,12 @@ static void *parallel_map_access(void *arg)
> >                 err = bpf_map_lookup_elem_flags(map_fd, &key, vars, BPF_F_LOCK);
> >                 if (err) {
> >                         printf("lookup failed\n");
> > -                       error_cnt++;
> > +                       test__fail();
> >                         goto out;
> >                 }
> >                 if (vars[0] != 0) {
> >                         printf("lookup #%d var[0]=%d\n", i, vars[0]);
> > -                       error_cnt++;
> > +                       test__fail();
> >                         goto out;
> >                 }
> >                 rnd = vars[1];
> > @@ -24,7 +24,7 @@ static void *parallel_map_access(void *arg)
> >                                 continue;
> >                         printf("lookup #%d var[1]=%d var[%d]=%d\n",
> >                                i, rnd, j, vars[j]);
> > -                       error_cnt++;
> > +                       test__fail();
> >                         goto out;
> >                 }
> >         }
> > @@ -69,7 +69,7 @@ void test_map_lock(void)
> >                        ret == (void *)&map_fd[i - 4]);
> >         goto close_prog_noerr;
> >  close_prog:
> > -       error_cnt++;
> > +       test__fail();
> >  close_prog_noerr:
> >         bpf_object__close(obj);
> >  }
> > diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
> > index 4ecfd721a044..9ef4e4ffb379 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/pkt_access.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
> > @@ -10,7 +10,7 @@ void test_pkt_access(void)
> >
> >         err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
> > index ac0d43435806..c354b9d21f4f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
> > @@ -10,7 +10,7 @@ void test_pkt_md_access(void)
> >
> >         err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
> > index e60cd5ff1f55..48a8cd144bd1 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
> > @@ -28,7 +28,7 @@ static void test_queue_stack_map_by_type(int type)
> >
> >         err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > @@ -44,7 +44,7 @@ static void test_queue_stack_map_by_type(int type)
> >         for (i = 0; i < MAP_SIZE; i++) {
> >                 err = bpf_map_update_elem(map_in_fd, NULL, &vals[i], 0);
> >                 if (err) {
> > -                       error_cnt++;
> > +                       test__fail();
> >                         goto out;
> >                 }
> >         }
> > diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> > index 4a4f428d1a78..f6987e3dd28c 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> > @@ -11,7 +11,7 @@ void test_reference_tracking(void)
> >
> >         obj = bpf_object__open(file);
> >         if (IS_ERR(obj)) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
> > index 114ebe6a438e..e843336713e8 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
> > @@ -23,7 +23,7 @@ void test_spinlock(void)
> >                        ret == (void *)&prog_fd);
> >         goto close_prog_noerr;
> >  close_prog:
> > -       error_cnt++;
> > +       test__fail();
> >  close_prog_noerr:
> >         bpf_object__close(obj);
> >  }
> > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
> > index fc539335c5b3..9dba1cc3da60 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
> > @@ -70,7 +70,7 @@ void test_stacktrace_map(void)
> >
> >         goto disable_pmu_noerr;
> >  disable_pmu:
> > -       error_cnt++;
> > +       test__fail();
> >  disable_pmu_noerr:
> >         bpf_link__destroy(link);
> >  close_prog:
> > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
> > index fbfa8e76cf63..4e7cf2e663f7 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
> > @@ -60,7 +60,7 @@ void test_stacktrace_map_raw_tp(void)
> >
> >         goto close_prog_noerr;
> >  close_prog:
> > -       error_cnt++;
> > +       test__fail();
> >  close_prog_noerr:
> >         if (!IS_ERR_OR_NULL(link))
> >                 bpf_link__destroy(link);
> > diff --git a/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c b/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
> > index 958a3d88de99..d9ad1aa8a026 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/task_fd_query_rawtp.c
> > @@ -72,7 +72,7 @@ void test_task_fd_query_rawtp(void)
> >
> >         goto close_prog_noerr;
> >  close_prog:
> > -       error_cnt++;
> > +       test__fail();
> >  close_prog_noerr:
> >         bpf_object__close(obj);
> >  }
> > diff --git a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
> > index f9b70e81682b..76209f2386c8 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
> > @@ -68,7 +68,7 @@ static void test_task_fd_query_tp_core(const char *probe_name,
> >  close_pmu:
> >         close(pmu_fd);
> >  close_prog:
> > -       error_cnt++;
> > +       test__fail();
> >  close_prog_noerr:
> >         bpf_object__close(obj);
> >  }
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_estats.c b/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
> > index bb8759d69099..e241e5d7c71f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tcp_estats.c
> > @@ -11,7 +11,7 @@ void test_tcp_estats(void)
> >         err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
> >         CHECK(err, "", "err %d errno %d\n", err, errno);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp.c b/tools/testing/selftests/bpf/prog_tests/xdp.c
> > index a74167289545..7c9f89fa1d02 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/xdp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/xdp.c
> > @@ -17,7 +17,7 @@ void test_xdp(void)
> >
> >         err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> > index 922aa0a19764..a479a3303c3b 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> > @@ -11,7 +11,7 @@ void test_xdp_adjust_tail(void)
> >
> >         err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> > index 15f7c272edb0..10bef9d5ab81 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> > @@ -32,7 +32,7 @@ void test_xdp_noinline(void)
> >
> >         err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
> >         if (err) {
> > -               error_cnt++;
> > +               test__fail();
> >                 return;
> >         }
> >
> > @@ -74,7 +74,7 @@ void test_xdp_noinline(void)
> >                 pkts += stats[i].pkts;
> >         }
> >         if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
> > -               error_cnt++;
> > +               test__fail();
> >                 printf("test_xdp_noinline:FAIL:stats %lld %lld\n",
> >                        bytes, pkts);
> >         }
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 1993f2ce0d23..ad90e45768ce 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -8,24 +8,6 @@
> >
> >  /* defined in test_progs.h */
> >  struct test_env env;
> > -int error_cnt, pass_cnt;
> > -
> > -struct prog_test_def {
> > -       const char *test_name;
> > -       int test_num;
> > -       void (*run_test)(void);
> > -       bool force_log;
> > -       int pass_cnt;
> > -       int error_cnt;
> > -       bool tested;
> > -
> > -       const char *subtest_name;
> > -       int subtest_num;
> > -
> > -       /* store counts before subtest started */
> > -       int old_pass_cnt;
> > -       int old_error_cnt;
> > -};
> >
> >  static bool should_run(struct test_selector *sel, int num, const char *name)
> >  {
> > @@ -74,14 +56,14 @@ static const char *test_status_string(bool success)
> >  void test__end_subtest()
> >  {
> >         struct prog_test_def *test = env.test;
> > -       int sub_error_cnt = error_cnt - test->old_error_cnt;
> > +       int sub_fail_cnt = test->fail_cnt - test->old_fail_cnt;
> >
> > -       if (sub_error_cnt)
> > -               env.fail_cnt++;
> > +       if (sub_fail_cnt)
> > +               test->fail_cnt++;
> >         else
> >                 env.sub_succ_cnt++;
> >
> > -       dump_test_log(test, sub_error_cnt);
> > +       dump_test_log(test, sub_fail_cnt);
> >
> >         fprintf(env.stdout, "#%3d/%-3d %4s %s:%s\n",
> >                 test->test_num, test->subtest_num,
> > @@ -111,8 +93,8 @@ bool test__start_subtest(const char *name)
> >                 return false;
> >
> >         test->subtest_name = name;
> > -       env.test->old_pass_cnt = pass_cnt;
> > -       env.test->old_error_cnt = error_cnt;
> > +       env.test->old_succ_cnt = env.test->succ_cnt;
> > +       env.test->old_fail_cnt = env.test->fail_cnt;
> >
> >         return true;
> >  }
> > @@ -126,6 +108,19 @@ void test__skip(void)
> >         env.skip_cnt++;
> >  }
> >
> > +void __test__fail(const char *file, int line)
> > +{
> > +       if (env.test->subtest_name)
> > +               fprintf(stderr, "%s:%s failed at %s:%d, errno=%d\n",
> 
> Nit: let's keep <test>/<subtest> convention here as well?
> 
> Failure doesn't always set errno, this will be quite confusing
> sometimes. Especially for higher-level libbpf APIs, which don't set
> errno at all.
> If test wants to log additional information, let it do it explicitly.
SG. Maybe we can adapt log_err from cgroup_helpers.h for error reporting
once I move sockopt tests into test_progs.

> Also, _CHECK already logs error message, so this is going to be
> double-verbose for typical case. I'd say let's drop these error
> messages and instead slightly extend _CHECK one with line number (it
> already logs func name).
Not everything goes through the _CHECK macro unfortunately, see
all the cases where I did s/error_cnt++/test__fail/. How about I
remove the error message from _CHECK and leave it in __test_fail?

> > +                       env.test->test_name, env.test->subtest_name,
> > +                       file, line, errno);
> > +       else
> > +               fprintf(stderr, "%s failed at %s:%d, errno=%d\n",
> > +                       env.test->test_name, file, line, errno);
> > +
> > +       env.test->fail_cnt++;
> > +}
> > +
> >  struct ipv4_packet pkt_v4 = {
> >         .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> >         .iph.ihl = 5,
> > @@ -150,7 +145,7 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name)
> >         map = bpf_object__find_map_by_name(obj, name);
> >         if (!map) {
> >                 printf("%s:FAIL:map '%s' not found\n", test, name);
> > -               error_cnt++;
> > +               test__fail();
> >                 return -1;
> >         }
> >         return bpf_map__fd(map);
> > @@ -509,8 +504,6 @@ int main(int argc, char **argv)
> >         stdio_hijack();
> >         for (i = 0; i < prog_test_cnt; i++) {
> >                 struct prog_test_def *test = &prog_test_defs[i];
> > -               int old_pass_cnt = pass_cnt;
> > -               int old_error_cnt = error_cnt;
> >
> >                 env.test = test;
> >                 test->test_num = i + 1;
> > @@ -525,14 +518,12 @@ int main(int argc, char **argv)
> >                         test__end_subtest();
> >
> >                 test->tested = true;
> > -               test->pass_cnt = pass_cnt - old_pass_cnt;
> > -               test->error_cnt = error_cnt - old_error_cnt;
> > -               if (test->error_cnt)
> > +               if (test->fail_cnt)
> >                         env.fail_cnt++;
> >                 else
> >                         env.succ_cnt++;
> >
> > -               dump_test_log(test, test->error_cnt);
> > +               dump_test_log(test, test->fail_cnt);
> >
> >                 fprintf(env.stdout, "#%3d     %4s %s\n",
> >                         test->test_num,
> > @@ -546,5 +537,5 @@ int main(int argc, char **argv)
> >         free(env.test_selector.num_set);
> >         free(env.subtest_selector.num_set);
> >
> > -       return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
> > +       return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
> >  }
> > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > index 9defd35cb6c0..7b05921784a4 100644
> > --- a/tools/testing/selftests/bpf/test_progs.h
> > +++ b/tools/testing/selftests/bpf/test_progs.h
> > @@ -38,7 +38,23 @@ typedef __u16 __sum16;
> >  #include "trace_helpers.h"
> >  #include "flow_dissector_load.h"
> >
> > -struct prog_test_def;
> > +struct prog_test_def {
> > +       const char *test_name;
> > +       int test_num;
> > +       void (*run_test)(void);
> > +       bool force_log;
> > +       bool tested;
> > +
> > +       const char *subtest_name;
> > +       int subtest_num;
> > +
> > +       int succ_cnt;
> > +       int fail_cnt;
> 
> So I'm neutral on this rename, I even considered it myself initially.
> But keep in mind, that succ/fail in env means number of tests, while
> test->succ/fail means number of assertions. We don't report total
> number of failed checks anymore, so it doesn't matter, but if we ever
> want to keep track of that at env level, it will be very confusing and
> inconvenient.
Point taken, I didn't think about it, let me undo the rename. I'll
try to add a comment instead to highlight the difference.

> > +
> > +       /* store counts before subtest started */
> > +       int old_succ_cnt;
> > +       int old_fail_cnt;
> > +};
> 
> Did you move it here just to access env.test->succ_cnt in _CHECK()?
> Maybe add test__success() counterpart to test__fail() instead?
Yeah, good point, will do.

> >
> >  struct test_selector {
> >         const char *name;
> > @@ -67,13 +83,13 @@ struct test_env {
> >         int skip_cnt; /* skipped tests */
> >  };
> >
> > -extern int error_cnt;
> > -extern int pass_cnt;
> >  extern struct test_env env;
> >
> >  extern void test__force_log();
> >  extern bool test__start_subtest(const char *name);
> >  extern void test__skip(void);
> > +#define test__fail() __test__fail(__FILE__, __LINE__)
> > +extern void __test__fail(const char *file, int line);
> 
> Given my comment above about too verbose logging, I'd say let's keep
> this simple and have just
> 
> extern void test__fail()
> 
> And let _CHECK log file:line.
See above about test__fail without _CHECK. Maybe we should do QCHECK
as you suggested in the other email.

So those lonely:

	if (err) {
		error_cnt++;
		return;
	}

checks can instead be converted to:

	if (QCHECK(err))
		return;

Let me play with it a bit and see how it goes.

> >
> >  #define MAGIC_BYTES 123
> >
> > @@ -96,11 +112,11 @@ extern struct ipv6_packet pkt_v6;
> >  #define _CHECK(condition, tag, duration, format...) ({                 \
> >         int __ret = !!(condition);                                      \
> >         if (__ret) {                                                    \
> > -               error_cnt++;                                            \
> > +               test__fail();                                           \
> >                 printf("%s:FAIL:%s ", __func__, tag);                   \
> >                 printf(format);                                         \
> >         } else {                                                        \
> > -               pass_cnt++;                                             \
> > +               env.test->succ_cnt++;                                   \
> >                 printf("%s:PASS:%s %d nsec\n",                          \
> >                        __func__, tag, duration);                        \
> >         }                                                               \
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 3/4] selftests/bpf: test_progs: remove global fail/success counts
  2019-08-14 20:07     ` Stanislav Fomichev
@ 2019-08-14 20:22       ` Andrii Nakryiko
  0 siblings, 0 replies; 17+ messages in thread
From: Andrii Nakryiko @ 2019-08-14 20:22 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Stanislav Fomichev, Networking, bpf, David S. Miller,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Wed, Aug 14, 2019 at 1:07 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 08/14, Andrii Nakryiko wrote:
> > On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > Now that we have a global per-test/per-environment state, there
> > > is no longer the need to have global fail/success counters
> > > (and there is no need to save/get the diff before/after the
> > > test).
> > >
> > > Cc: Andrii Nakryiko <andriin@fb.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---

[...]

> > > +void __test__fail(const char *file, int line)
> > > +{
> > > +       if (env.test->subtest_name)
> > > +               fprintf(stderr, "%s:%s failed at %s:%d, errno=%d\n",
> >
> > Nit: let's keep <test>/<subtest> convention here as well?
> >
> > Failure doesn't always set errno, this will be quite confusing
> > sometimes. Especially for higher-level libbpf APIs, which don't set
> > errno at all.
> > If test wants to log additional information, let it do it explicitly.
> SG. Maybe we can adapt log_err from cgroup_helpers.h for error reporting
> once I move sockopt tests into test_progs.
>
> > Also, _CHECK already logs error message, so this is going to be
> > double-verbose for typical case. I'd say let's drop these error
> > messages and instead slightly extend _CHECK one with line number (it
> > already logs func name).
> Not everything goes through the _CHECK macro unfortunately, see

Well, it should (at least eventually). If existing macro doesn't cover
typical use case, we can add one that does cover.

> all the cases where I did s/error_cnt++/test__fail/. How about I
> remove the error message from _CHECK and leave it in __test_fail?

I'd keep test__fail() and test__success() as a low-level building
block. And move all the logging into corresponding high-level macro.
This still gives flexibility to do one-off crazy tests, if necessary,
while having consistent approach for everything else.

>
> > > +                       env.test->test_name, env.test->subtest_name,
> > > +                       file, line, errno);
> > > +       else
> > > +               fprintf(stderr, "%s failed at %s:%d, errno=%d\n",
> > > +                       env.test->test_name, file, line, errno);
> > > +
> > > +       env.test->fail_cnt++;
> > > +}
> > > +
> > >  struct ipv4_packet pkt_v4 = {
> > >         .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> > >         .iph.ihl = 5,
> > > @@ -150,7 +145,7 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name)
> > >         map = bpf_object__find_map_by_name(obj, name);
> > >         if (!map) {
> > >                 printf("%s:FAIL:map '%s' not found\n", test, name);
> > > -               error_cnt++;
> > > +               test__fail();
> > >                 return -1;
> > >         }
> > >         return bpf_map__fd(map);
> > > @@ -509,8 +504,6 @@ int main(int argc, char **argv)
> > >         stdio_hijack();
> > >         for (i = 0; i < prog_test_cnt; i++) {
> > >                 struct prog_test_def *test = &prog_test_defs[i];
> > > -               int old_pass_cnt = pass_cnt;
> > > -               int old_error_cnt = error_cnt;
> > >
> > >                 env.test = test;
> > >                 test->test_num = i + 1;
> > > @@ -525,14 +518,12 @@ int main(int argc, char **argv)
> > >                         test__end_subtest();
> > >
> > >                 test->tested = true;
> > > -               test->pass_cnt = pass_cnt - old_pass_cnt;
> > > -               test->error_cnt = error_cnt - old_error_cnt;
> > > -               if (test->error_cnt)
> > > +               if (test->fail_cnt)
> > >                         env.fail_cnt++;
> > >                 else
> > >                         env.succ_cnt++;
> > >
> > > -               dump_test_log(test, test->error_cnt);
> > > +               dump_test_log(test, test->fail_cnt);
> > >
> > >                 fprintf(env.stdout, "#%3d     %4s %s\n",
> > >                         test->test_num,
> > > @@ -546,5 +537,5 @@ int main(int argc, char **argv)
> > >         free(env.test_selector.num_set);
> > >         free(env.subtest_selector.num_set);
> > >
> > > -       return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
> > > +       return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
> > >  }
> > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > > index 9defd35cb6c0..7b05921784a4 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.h
> > > +++ b/tools/testing/selftests/bpf/test_progs.h
> > > @@ -38,7 +38,23 @@ typedef __u16 __sum16;
> > >  #include "trace_helpers.h"
> > >  #include "flow_dissector_load.h"
> > >
> > > -struct prog_test_def;
> > > +struct prog_test_def {
> > > +       const char *test_name;
> > > +       int test_num;
> > > +       void (*run_test)(void);
> > > +       bool force_log;
> > > +       bool tested;
> > > +
> > > +       const char *subtest_name;
> > > +       int subtest_num;
> > > +
> > > +       int succ_cnt;
> > > +       int fail_cnt;
> >
> > So I'm neutral on this rename, I even considered it myself initially.
> > But keep in mind, that succ/fail in env means number of tests, while
> > test->succ/fail means number of assertions. We don't report total
> > number of failed checks anymore, so it doesn't matter, but if we ever
> > want to keep track of that at env level, it will be very confusing and
> > inconvenient.
> Point taken, I didn't think about it, let me undo the rename. I'll
> try to add a comment instead to highlight the difference.
>
> > > +
> > > +       /* store counts before subtest started */
> > > +       int old_succ_cnt;
> > > +       int old_fail_cnt;
> > > +};
> >
> > Did you move it here just to access env.test->succ_cnt in _CHECK()?
> > Maybe add test__success() counterpart to test__fail() instead?
> Yeah, good point, will do.
>
> > >
> > >  struct test_selector {
> > >         const char *name;
> > > @@ -67,13 +83,13 @@ struct test_env {
> > >         int skip_cnt; /* skipped tests */
> > >  };
> > >
> > > -extern int error_cnt;
> > > -extern int pass_cnt;
> > >  extern struct test_env env;
> > >
> > >  extern void test__force_log();
> > >  extern bool test__start_subtest(const char *name);
> > >  extern void test__skip(void);
> > > +#define test__fail() __test__fail(__FILE__, __LINE__)
> > > +extern void __test__fail(const char *file, int line);
> >
> > Given my comment above about too verbose logging, I'd say let's keep
> > this simple and have just
> >
> > extern void test__fail()
> >
> > And let _CHECK log file:line.
> See above about test__fail without _CHECK. Maybe we should do QCHECK
> as you suggested in the other email.
>
> So those lonely:
>
>         if (err) {
>                 error_cnt++;
>                 return;
>         }
>
> checks can instead be converted to:
>
>         if (QCHECK(err))
>                 return;
>
> Let me play with it a bit and see how it goes.

Yeah, give it a go. Try keeping file:line logging in macro, where it's
more natural, IMO.

>
> > >
> > >  #define MAGIC_BYTES 123
> > >
> > > @@ -96,11 +112,11 @@ extern struct ipv6_packet pkt_v6;
> > >  #define _CHECK(condition, tag, duration, format...) ({                 \
> > >         int __ret = !!(condition);                                      \
> > >         if (__ret) {                                                    \
> > > -               error_cnt++;                                            \
> > > +               test__fail();                                           \
> > >                 printf("%s:FAIL:%s ", __func__, tag);                   \
> > >                 printf(format);                                         \
> > >         } else {                                                        \
> > > -               pass_cnt++;                                             \
> > > +               env.test->succ_cnt++;                                   \
> > >                 printf("%s:PASS:%s %d nsec\n",                          \
> > >                        __func__, tag, duration);                        \
> > >         }                                                               \
> > > --
> > > 2.23.0.rc1.153.gdeed80330f-goog
> > >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip
  2019-08-14 20:01         ` Andrii Nakryiko
@ 2019-08-16  5:16           ` Alexei Starovoitov
  2019-08-16  5:23             ` Andrii Nakryiko
  0 siblings, 1 reply; 17+ messages in thread
From: Alexei Starovoitov @ 2019-08-16  5:16 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Stanislav Fomichev, Stanislav Fomichev, Networking, bpf,
	David S. Miller, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko

On Wed, Aug 14, 2019 at 1:01 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> >
> > Let me know if you see a value in highlighting test vs subtest skip.
> >
> > Other related question is: should we do verbose output in case
> > of a skip? Right now we don't do it.
>
> It might be useful, I guess, especially if it's not too common. But
> Alexei is way more picky about stuff like that, so I'd defer to him. I
> have no problem with a clean "SKIPPED: <test>/<subtest> (maybe some
> reason for skipping here)" message.

Since test_progs prints single number for FAILED tests then single number
for SKIPPED tests is fine as well.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip
  2019-08-16  5:16           ` Alexei Starovoitov
@ 2019-08-16  5:23             ` Andrii Nakryiko
  0 siblings, 0 replies; 17+ messages in thread
From: Andrii Nakryiko @ 2019-08-16  5:23 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Stanislav Fomichev, Stanislav Fomichev, Networking, bpf,
	David S. Miller, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko

On Thu, Aug 15, 2019 at 10:16 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Aug 14, 2019 at 1:01 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > >
> > > Let me know if you see a value in highlighting test vs subtest skip.
> > >
> > > Other related question is: should we do verbose output in case
> > > of a skip? Right now we don't do it.
> >
> > It might be useful, I guess, especially if it's not too common. But
> > Alexei is way more picky about stuff like that, so I'd defer to him. I
> > have no problem with a clean "SKIPPED: <test>/<subtest> (maybe some
> > reason for skipping here)" message.
>
> Since test_progs prints single number for FAILED tests then single number
> for SKIPPED tests is fine as well.

I'm fine with single number, but it should count number of subtests
skipped, if there are subtests within test, same as for FAILED.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-08-16  5:23 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14 16:47 [PATCH bpf-next 0/4] selftests/bpf: test_progs: misc fixes Stanislav Fomichev
2019-08-14 16:47 ` [PATCH bpf-next 1/4] selftests/bpf: test_progs: change formatting of the condenced output Stanislav Fomichev
2019-08-14 17:00   ` Alexei Starovoitov
2019-08-14 17:07     ` Stanislav Fomichev
2019-08-14 16:47 ` [PATCH bpf-next 2/4] selftests/bpf: test_progs: test__skip Stanislav Fomichev
2019-08-14 19:22   ` Andrii Nakryiko
2019-08-14 19:30     ` Andrii Nakryiko
2019-08-14 19:53       ` Stanislav Fomichev
2019-08-14 20:01         ` Andrii Nakryiko
2019-08-16  5:16           ` Alexei Starovoitov
2019-08-16  5:23             ` Andrii Nakryiko
2019-08-14 16:47 ` [PATCH bpf-next 3/4] selftests/bpf: test_progs: remove global fail/success counts Stanislav Fomichev
2019-08-14 19:47   ` Andrii Nakryiko
2019-08-14 20:07     ` Stanislav Fomichev
2019-08-14 20:22       ` Andrii Nakryiko
2019-08-14 16:47 ` [PATCH bpf-next 4/4] selftests/bpf: test_progs: remove asserts from subtests Stanislav Fomichev
2019-08-14 19:52   ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).