On Wed, Jul 15, 2020 at 01:29:34PM +0100, Ciara Power wrote: > From: Louise Kilheeney <louise.kilhee...@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.kilhee...@intel.com> > Signed-off-by: Ciara Power <ciara.po...@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