diff mbox series

[v2,10/13] test/telemetry_data: refactor for maintainability

Message ID 20220725163543.875775-11-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series telemetry JSON escaping and other enhancements |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Bruce Richardson July 25, 2022, 4:35 p.m. UTC 
  To help with the writing and maintaining of test cases in this file we
can make the following changes to it:

- rename non-test-case functions i.e. the infrastructure functions, to
  not start with "test_", so that each sub-test case can be identified
  by starting with that prefix.
- add a comment at the start of the file explaining how tests are to be
  written and managed, so as to keep consistency.
- add a trivial test-case for returning a simple string value to use as
  a reference example for those wanting to add test cases.
- improve the key macro used for validating the output from each
  function, so that the standard json preamble can be skipped for each
  function. This hides more of the infrastructure implementation from
  the user i.e. they don't need to worry what the actual command used is
  called, and also shortens the output strings so we can avoid line
  splitting in most cases.
- add clearing the "response_data" structure to the loop calling each
  test to avoid each test function having to do so individually.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_data.c | 101 ++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 41 deletions(-)

Comments

Power, Ciara Aug. 23, 2022, 12:33 p.m. UTC | #1 
> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Monday 25 July 2022 17:36
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Power, Ciara
> <ciara.power@intel.com>
> Subject: [PATCH v2 10/13] test/telemetry_data: refactor for maintainability
> 
> To help with the writing and maintaining of test cases in this file we can make
> the following changes to it:
> 
> - rename non-test-case functions i.e. the infrastructure functions, to
>   not start with "test_", so that each sub-test case can be identified
>   by starting with that prefix.
> - add a comment at the start of the file explaining how tests are to be
>   written and managed, so as to keep consistency.
> - add a trivial test-case for returning a simple string value to use as
>   a reference example for those wanting to add test cases.
> - improve the key macro used for validating the output from each
>   function, so that the standard json preamble can be skipped for each
>   function. This hides more of the infrastructure implementation from
>   the user i.e. they don't need to worry what the actual command used is
>   called, and also shortens the output strings so we can avoid line
>   splitting in most cases.
> - add clearing the "response_data" structure to the loop calling each
>   test to avoid each test function having to do so individually.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/test_telemetry_data.c | 101 ++++++++++++++++++++-------------
>  1 file changed, 60 insertions(+), 41 deletions(-)
> 
> diff --git a/app/test/test_telemetry_data.c
> b/app/test/test_telemetry_data.c index 73eee293a1..5a85e790d3 100644
> --- a/app/test/test_telemetry_data.c
> +++ b/app/test/test_telemetry_data.c
> @@ -21,18 +21,45 @@
>  #define TELEMETRY_VERSION "v2"
>  #define REQUEST_CMD "/test"
>  #define BUF_SIZE 1024
> -#define TEST_OUTPUT(exp) test_output(__func__, exp)
> +#define CHECK_OUTPUT(exp) check_output(__func__, "{\""
> REQUEST_CMD
> +"\":" exp "}")
> +
> +/*
> + * Runs a series of test cases, checking the output of telemetry for
> +various different types of
> + * responses. On init, a single connection to DPDK telemetry is made,
> +and a single telemetry
> + * callback "/test" is registered. That callback always returns the
> +value of the static global
> + * variable "response_data", so each test case builds up that
> +structure, and then calls the
> + * "check_output" function to ensure the response received over the
> +socket for "/test" matches
> + * that expected for the response_data value populated.
> + *
> + * NOTE:
> + * - each test case function in this file should be added to the "test_cases"
> array in
> + *   test_telemetry_data function at the bottom of the file.
> + * - each test case function should populate the "response_data" global
> variable (below)
> + *   with the appropriate values which would be returned from a simulated
> telemetry function.
> + *   Then the test case function should have "return
> TEST_OUTPUT(<expected_data>);" as it's


[CP] nit: I think this should be CHECK_OUTPUT based on the macro rename above.


> + *   last line. The test infrastructure will then validate that the output when
> returning
> + *   "response_data" structure matches that in "<expected_data>".
> + * - the response_data structure will be zeroed on entry to each test
> function, so each function
> + *   can begin with a call to "rte_tel_data_string/start_array/start_dict" as so
> desired.
> + * - the expected_output for each function can be just the actual json data
> from the
> + *   "response_data" value. The CHECK_OUTPUT macro will include the
> appropriate "{\"/test\": ... }"
> + *   structure around the json output.
> + *
> + *  See test_simple_string(), or test_case_array_int() for a basic examples
> of test cases.
> + */

<snip>
diff mbox series

Patch

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index 73eee293a1..5a85e790d3 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -21,18 +21,45 @@ 
 #define TELEMETRY_VERSION "v2"
 #define REQUEST_CMD "/test"
 #define BUF_SIZE 1024
-#define TEST_OUTPUT(exp) test_output(__func__, exp)
+#define CHECK_OUTPUT(exp) check_output(__func__, "{\"" REQUEST_CMD "\":" exp "}")
+
+/*
+ * Runs a series of test cases, checking the output of telemetry for various different types of
+ * responses. On init, a single connection to DPDK telemetry is made, and a single telemetry
+ * callback "/test" is registered. That callback always returns the value of the static global
+ * variable "response_data", so each test case builds up that structure, and then calls the
+ * "check_output" function to ensure the response received over the socket for "/test" matches
+ * that expected for the response_data value populated.
+ *
+ * NOTE:
+ * - each test case function in this file should be added to the "test_cases" array in
+ *   test_telemetry_data function at the bottom of the file.
+ * - each test case function should populate the "response_data" global variable (below)
+ *   with the appropriate values which would be returned from a simulated telemetry function.
+ *   Then the test case function should have "return TEST_OUTPUT(<expected_data>);" as it's
+ *   last line. The test infrastructure will then validate that the output when returning
+ *   "response_data" structure matches that in "<expected_data>".
+ * - the response_data structure will be zeroed on entry to each test function, so each function
+ *   can begin with a call to "rte_tel_data_string/start_array/start_dict" as so desired.
+ * - the expected_output for each function can be just the actual json data from the
+ *   "response_data" value. The CHECK_OUTPUT macro will include the appropriate "{\"/test\": ... }"
+ *   structure around the json output.
+ *
+ *  See test_simple_string(), or test_case_array_int() for a basic examples of test cases.
+ */
+
 
 static struct rte_tel_data response_data;
 static int sock;
 
+
 /*
  * This function is the callback registered with Telemetry to be used when
  * the /test command is requested. This callback returns the global data built
  * up by the individual test cases.
  */
 static int
-test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
+telemetry_test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
 		struct rte_tel_data *d)
 {
 	*d = response_data;
@@ -46,7 +73,7 @@  test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
  * and is compared to the actual response received from Telemetry.
  */
 static int
-test_output(const char *func_name, const char *expected)
+check_output(const char *func_name, const char *expected)
 {
 	int bytes;
 	char buf[BUF_SIZE * 16];
@@ -66,6 +93,14 @@  test_output(const char *func_name, const char *expected)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_simple_string(void)
+{
+	rte_tel_data_string(&response_data, "Simple string");
+
+	return CHECK_OUTPUT("\"Simple string\"");
+}
+
 static int
 test_dict_with_array_int_values(void)
 {
@@ -77,7 +112,6 @@  test_dict_with_array_int_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_INT_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
@@ -90,8 +124,7 @@  test_dict_with_array_int_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[0,1,2,3,4],"
-			"\"dict_1\":[0,1,2,3,4]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[0,1,2,3,4],\"dict_1\":[0,1,2,3,4]}");
 }
 
 static int
@@ -105,7 +138,6 @@  test_array_with_array_int_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_INT_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	for (i = 0; i < 5; i++) {
@@ -115,18 +147,18 @@  test_array_with_array_int_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[0,1,2,3,4],[0,1,2,3,4]]}");
+	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
 static int
 test_case_array_int(void)
 {
 	int i;
-	memset(&response_data, 0, sizeof(response_data));
+
 	rte_tel_data_start_array(&response_data, RTE_TEL_INT_VAL);
 	for (i = 0; i < 5; i++)
 		rte_tel_data_add_array_int(&response_data, i);
-	return TEST_OUTPUT("{\"/test\":[0,1,2,3,4]}");
+	return CHECK_OUTPUT("[0,1,2,3,4]");
 }
 
 static int
@@ -135,7 +167,6 @@  test_case_add_dict_int(void)
 	int i = 0;
 	char name_of_value[8];
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
@@ -143,14 +174,12 @@  test_case_add_dict_int(void)
 		rte_tel_data_add_dict_int(&response_data, name_of_value, i);
 	}
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,"
-			"\"dict_3\":3,\"dict_4\":4}}");
+	return CHECK_OUTPUT("{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,\"dict_3\":3,\"dict_4\":4}");
 }
 
 static int
 test_case_array_string(void)
 {
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_STRING_VAL);
 	rte_tel_data_add_array_string(&response_data, "aaaa");
 	rte_tel_data_add_array_string(&response_data, "bbbb");
@@ -158,14 +187,12 @@  test_case_array_string(void)
 	rte_tel_data_add_array_string(&response_data, "dddd");
 	rte_tel_data_add_array_string(&response_data, "eeee");
 
-	return TEST_OUTPUT("{\"/test\":[\"aaaa\",\"bbbb\",\"cccc\",\"dddd\","
-			"\"eeee\"]}");
+	return CHECK_OUTPUT("[\"aaaa\",\"bbbb\",\"cccc\",\"dddd\",\"eeee\"]");
 }
 
 static int
 test_case_add_dict_string(void)
 {
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_dict_string(&response_data, "dict_0", "aaaa");
@@ -173,8 +200,7 @@  test_case_add_dict_string(void)
 	rte_tel_data_add_dict_string(&response_data, "dict_2", "cccc");
 	rte_tel_data_add_dict_string(&response_data, "dict_3", "dddd");
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":\"aaaa\",\"dict_1\":"
-			"\"bbbb\",\"dict_2\":\"cccc\",\"dict_3\":\"dddd\"}}");
+	return CHECK_OUTPUT("{\"dict_0\":\"aaaa\",\"dict_1\":\"bbbb\",\"dict_2\":\"cccc\",\"dict_3\":\"dddd\"}");
 }
 
 
@@ -187,7 +213,6 @@  test_dict_with_array_string_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -198,8 +223,7 @@  test_dict_with_array_string_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[\"aaaa\"],\"dict_1\":"
-			"[\"bbbb\"]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[\"aaaa\"],\"dict_1\":[\"bbbb\"]}");
 }
 
 static int
@@ -214,7 +238,6 @@  test_dict_with_dict_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -226,8 +249,7 @@  test_dict_with_dict_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_of_dicts",
 			dict_of_dicts, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_of_dicts\":{\"dict_0\":"
-			"[\"aaaa\"],\"dict_1\":[\"bbbb\"]}}}");
+	return CHECK_OUTPUT("{\"dict_of_dicts\":{\"dict_0\":[\"aaaa\"],\"dict_1\":[\"bbbb\"]}}");
 }
 
 static int
@@ -239,7 +261,6 @@  test_array_with_array_string_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -248,18 +269,18 @@  test_array_with_array_string_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[\"aaaa\"],[\"bbbb\"]]}");
+	return CHECK_OUTPUT("[[\"aaaa\"],[\"bbbb\"]]");
 }
 
 static int
 test_case_array_u64(void)
 {
 	int i;
-	memset(&response_data, 0, sizeof(response_data));
+
 	rte_tel_data_start_array(&response_data, RTE_TEL_U64_VAL);
 	for (i = 0; i < 5; i++)
 		rte_tel_data_add_array_u64(&response_data, i);
-	return TEST_OUTPUT("{\"/test\":[0,1,2,3,4]}");
+	return CHECK_OUTPUT("[0,1,2,3,4]");
 }
 
 static int
@@ -268,15 +289,13 @@  test_case_add_dict_u64(void)
 	int i = 0;
 	char name_of_value[8];
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
 		sprintf(name_of_value, "dict_%d", i);
 		rte_tel_data_add_dict_u64(&response_data, name_of_value, i);
 	}
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,"
-			"\"dict_3\":3,\"dict_4\":4}}");
+	return CHECK_OUTPUT("{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,\"dict_3\":3,\"dict_4\":4}");
 }
 
 static int
@@ -290,7 +309,6 @@  test_dict_with_array_u64_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_U64_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 10; i++) {
@@ -303,8 +321,7 @@  test_dict_with_array_u64_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[0,1,2,3,4,5,6,7,8,9],"
-			"\"dict_1\":[0,1,2,3,4,5,6,7,8,9]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[0,1,2,3,4,5,6,7,8,9],\"dict_1\":[0,1,2,3,4,5,6,7,8,9]}");
 }
 
 static int
@@ -318,7 +335,6 @@  test_array_with_array_u64_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_U64_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	for (i = 0; i < 5; i++) {
@@ -328,7 +344,7 @@  test_array_with_array_u64_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[0,1,2,3,4],[0,1,2,3,4]]}");
+	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
 static int
@@ -369,7 +385,7 @@  connect_to_socket(void)
 }
 
 static int
-test_telemetry_data(void)
+telemetry_data_autotest(void)
 {
 	typedef int (*test_case)(void);
 	unsigned int i = 0;
@@ -378,7 +394,9 @@  test_telemetry_data(void)
 	if (sock <= 0)
 		return -1;
 
-	test_case test_cases[] = {test_case_array_string,
+	test_case test_cases[] = {
+			test_simple_string,
+			test_case_array_string,
 			test_case_array_int, test_case_array_u64,
 			test_case_add_dict_int, test_case_add_dict_u64,
 			test_case_add_dict_string,
@@ -390,8 +408,9 @@  test_telemetry_data(void)
 			test_array_with_array_u64_values,
 			test_array_with_array_string_values };
 
-	rte_telemetry_register_cmd(REQUEST_CMD, test_cb, "Test");
+	rte_telemetry_register_cmd(REQUEST_CMD, telemetry_test_cb, "Test");
 	for (i = 0; i < RTE_DIM(test_cases); i++) {
+		memset(&response_data, 0, sizeof(response_data));
 		if (test_cases[i]() != 0) {
 			close(sock);
 			return -1;
@@ -401,4 +420,4 @@  test_telemetry_data(void)
 	return 0;
 }
 
-REGISTER_TEST_COMMAND(telemetry_data_autotest, test_telemetry_data);
+REGISTER_TEST_COMMAND(telemetry_data_autotest, telemetry_data_autotest);