From: Bruce Richardson <bruce.richardson@intel.com>
To: Ciara Power <ciara.power@intel.com>
Cc: kevin.laatz@intel.com, thomas@monjalon.net,
ferruh.yigit@intel.com, arybchenko@solarflare.com, dev@dpdk.org,
keith.wiles@intel.com,
Louise Kilheeney <louise.kilheeney@intel.com>
Subject: Re: [dpdk-dev] [PATCH v5 2/3] test/test_telemetry_data: add unit tests for data to JSON
Date: Fri, 17 Jul 2020 12:53:18 +0100 [thread overview]
Message-ID: <20200717115318.GA708@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20200715122935.1822-3-ciara.power@intel.com>
On Wed, Jul 15, 2020 at 01:29:34PM +0100, Ciara Power wrote:
> From: Louise Kilheeney <louise.kilheeney@intel.com>
>
> This patch adds tests for verifying telemetry data structures are
> converted to JSON as expected. Both flat and recursive data structures
> are tested, for all possible value types.
>
> Signed-off-by: Louise Kilheeney <louise.kilheeney@intel.com>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
Great to see some unit tests included. However, I think the documenting of
what is happening is missing in comments and in the commit log. I think you
need to explain in the commit log the general pattern used for the tests,
which seems to be having a different response copied to the user each time
the callback is called, and verifing that the output got is what was
built-up internally.
This should also be explained via comments in the code too, as I call out
below.
> ---
> app/test/Makefile | 1 +
> app/test/meson.build | 5 +-
> app/test/test_telemetry_data.c | 359 +++++++++++++++++++++++++++++++++
> 3 files changed, 363 insertions(+), 2 deletions(-)
> create mode 100644 app/test/test_telemetry_data.c
>
> diff --git a/app/test/Makefile b/app/test/Makefile
> index f4065271e..1cb64089c 100644
> --- a/app/test/Makefile
> +++ b/app/test/Makefile
> @@ -145,6 +145,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_LPM) += test_lpm6.c
> SRCS-$(CONFIG_RTE_LIBRTE_LPM) += test_lpm6_perf.c
>
> SRCS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += test_telemetry_json.c
> +SRCS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += test_telemetry_data.c
>
> SRCS-y += test_debug.c
> SRCS-y += test_errno.c
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 786a21397..4a72fe5b6 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -170,6 +170,7 @@ test_deps = ['acl',
> 'ring',
> 'security',
> 'stack',
> + 'telemetry',
> 'timer'
> ]
>
> @@ -345,8 +346,8 @@ if dpdk_conf.has('RTE_LIBRTE_SKELETON_EVENTDEV_PMD')
> test_deps += 'pmd_skeleton_event'
> endif
> if dpdk_conf.has('RTE_LIBRTE_TELEMETRY')
> - test_sources += 'test_telemetry_json.c'
> - fast_tests += [['telemetry_json_autotest', true]]
> + test_sources += ['test_telemetry_json.c', 'test_telemetry_data.c']
> + fast_tests += [['telemetry_json_autotest', true], ['telemetry_data_autotest', true]]
> endif
>
> # The following linkages of drivers are required because
> diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
> new file mode 100644
> index 000000000..670af9134
> --- /dev/null
> +++ b/app/test/test_telemetry_data.c
> @@ -0,0 +1,359 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020 Intel Corporation
> + */
> +
> +#include <glob.h>
> +#include <string.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <unistd.h>
> +#include <limits.h>
> +
> +#include <rte_eal.h>
> +#include <rte_common.h>
> +#include <rte_telemetry.h>
> +#include <rte_string_fns.h>
> +
> +#include "test.h"
> +#include "telemetry_data.h"
> +
> +#define TELEMETRY_VERSION "v2"
> +#define REQUEST_CMD "/test"
> +#define BUF_SIZE 1024
> +#define TEST_OUTPUT(exp) test_output(__func__, exp)
> +
> +static struct rte_tel_data response_data;
> +static int sock;
> +
> +static int
> +test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
> + struct rte_tel_data *d)
> +{
> + *d = response_data;
> + return 0;
> +}
This function needs a comment explaining what it does and why.
> +
> +static int
> +test_output(const char *func_name, const char *expected)
> +{
> + int bytes;
> + char buf[BUF_SIZE * 16];
> + if (write(sock, REQUEST_CMD, strlen(REQUEST_CMD)) < 0) {
> + printf("%s: Error with socket write - %s\n", __func__,
> + strerror(errno));
> + return -1;
> + }
> + bytes = read(sock, buf, sizeof(buf));
> + if (bytes < 0) {
> + printf("%s: Error with socket read - %s\n", __func__,
> + strerror(errno));
> + return -1;
> + }
> + buf[bytes] = '\0';
> + printf("%s: buf = '%s', expected = '%s'\n", func_name, buf, expected);
> + return strncmp(expected, buf, sizeof(buf));
> +}
> +
This one too, needs comment explaining how it works etc.
<snip>
> +static int
> +test_telemetry_data(void)
> +{
> + sock = connect_to_socket();
> + if (sock <= 0)
> + return -1;
> +
> + rte_telemetry_register_cmd(REQUEST_CMD, test_cb, "Test");
> + if (test_case_array_string() != 0
> + || test_case_array_int() != 0
> + || test_case_array_u64() != 0
> + || test_case_add_dict_int() != 0
> + || test_case_add_dict_u64() != 0
> + || test_case_add_dict_string() != 0
> + || test_dict_with_array_int_values() != 0
> + || test_dict_with_array_u64_values() != 0
> + || test_dict_with_array_string_values() != 0
> + || test_array_with_array_int_values() != 0
> + || test_array_with_array_u64_values() != 0
> + || test_array_with_array_string_values() != 0) {
> + close(sock);
> + return -1;
> + }
I'm not sure I like this way of calling the test cases, and the indentation
of the lines is definitely wrong. The simplest option here is to have a
single if statement for each test and a goto for the "error" leg:
if (test_case_array_string() != 0)
goto error;
if (test_case_array_int() != 0)
goto error;
...
However, if you want to avoid the multiple if statements, a better option
than merging as above would be to put the functions in an array and loop
over them as (rough code, completely untested!):
typedef int (*test_case)(void);
test_case test_cases[] = {test_case_array_string,
test_case_array_int, ... };
...
for (i = 0; i < RTE_DIM(test_cases); i++)
if (test_cases[i]() != 0) {
...
}
> +
> + close(sock);
> + return 0;
> +}
> +
> +REGISTER_TEST_COMMAND(telemetry_data_autotest, test_telemetry_data);
> --
Regards,
/Bruce
next prev parent reply other threads:[~2020-07-17 11:53 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-12 10:53 [dpdk-dev] [RFC 0/2] add basic ethdev stats with data object recursion Ciara Power
2020-06-12 10:53 ` [dpdk-dev] [RFC 1/2] telemetry: support some recursive data objects Ciara Power
2020-06-12 12:58 ` Bruce Richardson
2020-06-12 13:07 ` Bruce Richardson
2020-06-12 13:14 ` Bruce Richardson
2020-06-12 10:53 ` [dpdk-dev] [RFC 2/2] ethdev: add basic stats for telemetry Ciara Power
2020-06-12 13:10 ` Bruce Richardson
2020-06-24 13:48 ` [dpdk-dev] [PATCH v2 0/2] add basic ethdev stats with data object recursion Ciara Power
2020-06-24 13:48 ` [dpdk-dev] [PATCH v2 1/2] telemetry: support array values in data objects Ciara Power
2020-06-24 15:09 ` Bruce Richardson
2020-06-24 15:18 ` Bruce Richardson
2020-06-24 13:48 ` [dpdk-dev] [PATCH v2 2/2] ethdev: add basic stats for telemetry Ciara Power
2020-06-24 15:19 ` Bruce Richardson
2020-07-02 10:19 ` [dpdk-dev] [PATCH v3 0/2] add basic ethdev stats with data object recursion Ciara Power
2020-07-02 10:19 ` [dpdk-dev] [PATCH v3 1/2] telemetry: support array values in data objects Ciara Power
2020-07-07 7:15 ` Thomas Monjalon
2020-07-02 10:19 ` [dpdk-dev] [PATCH v3 2/2] ethdev: add common stats for telemetry Ciara Power
2020-07-13 14:23 ` [dpdk-dev] [PATCH v4 0/2] add basic ethdev stats with data object recursion Ciara Power
2020-07-13 14:23 ` [dpdk-dev] [PATCH v4 1/2] telemetry: support array values in data objects Ciara Power
2020-07-13 14:23 ` [dpdk-dev] [PATCH v4 2/2] ethdev: add common stats for telemetry Ciara Power
2020-07-15 12:29 ` [dpdk-dev] [PATCH v5 0/3] add basic ethdev stats with data object recursion Ciara Power
2020-07-15 12:29 ` [dpdk-dev] [PATCH v5 1/3] telemetry: support array values in data objects Ciara Power
2020-07-15 12:29 ` [dpdk-dev] [PATCH v5 2/3] test/test_telemetry_data: add unit tests for data to JSON Ciara Power
2020-07-17 11:53 ` Bruce Richardson [this message]
2020-07-15 12:29 ` [dpdk-dev] [PATCH v5 3/3] ethdev: add common stats for telemetry Ciara Power
2020-07-20 11:19 ` [dpdk-dev] [PATCH v6 0/3] add basic ethdev stats with data object recursion Ciara Power
2020-07-20 11:19 ` [dpdk-dev] [PATCH v6 1/3] telemetry: support array values in data objects Ciara Power
2020-07-20 11:19 ` [dpdk-dev] [PATCH v6 2/3] test/test_telemetry_data: add unit tests for data to JSON Ciara Power
2020-07-20 13:08 ` Bruce Richardson
2020-07-20 11:19 ` [dpdk-dev] [PATCH v6 3/3] ethdev: add common stats for telemetry Ciara Power
2020-07-20 14:04 ` [dpdk-dev] [PATCH v7 0/3] add basic ethdev stats with data object recursion Ciara Power
2020-07-20 14:04 ` [dpdk-dev] [PATCH v7 1/3] telemetry: support array values in data objects Ciara Power
2020-07-20 14:04 ` [dpdk-dev] [PATCH v7 2/3] test/test_telemetry_data: add unit tests for data to JSON Ciara Power
2020-07-20 14:32 ` Bruce Richardson
2020-07-20 14:04 ` [dpdk-dev] [PATCH v7 3/3] ethdev: add common stats for telemetry Ciara Power
2020-08-21 12:51 ` [dpdk-dev] [PATCH v8 0/3] add basic ethdev stats with data object recursion Ciara Power
2020-08-21 12:51 ` [dpdk-dev] [PATCH v8 1/3] telemetry: support array values in data objects Ciara Power
2020-08-21 12:51 ` [dpdk-dev] [PATCH v8 2/3] test/test_telemetry_data: add unit tests for data to JSON Ciara Power
2020-08-21 12:51 ` [dpdk-dev] [PATCH v8 3/3] ethdev: add common stats for telemetry Ciara Power
2020-09-23 11:12 ` [dpdk-dev] [PATCH v9 0/3] add basic ethdev stats with data object recursion Ciara Power
2020-09-23 11:12 ` [dpdk-dev] [PATCH v9 1/3] telemetry: support array values in data objects Ciara Power
2020-09-23 11:12 ` [dpdk-dev] [PATCH v9 2/3] test/test_telemetry_data: add unit tests for data to JSON Ciara Power
2020-09-23 11:12 ` [dpdk-dev] [PATCH v9 3/3] ethdev: add common stats for telemetry Ciara Power
2020-10-06 20:56 ` [dpdk-dev] [PATCH v9 0/3] add basic ethdev stats with data object recursion Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200717115318.GA708@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=arybchenko@solarflare.com \
--cc=ciara.power@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=keith.wiles@intel.com \
--cc=kevin.laatz@intel.com \
--cc=louise.kilheeney@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is 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).