From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id BE7DDA053D; Fri, 17 Jul 2020 13:53:29 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9B2BA1C0D1; Fri, 17 Jul 2020 13:53:29 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 7384E1C031 for ; Fri, 17 Jul 2020 13:53:27 +0200 (CEST) IronPort-SDR: lE+h8SEZoY95yuQ9fZKT2dObKDnaUn3fhs3gq3I/IQhzcU6mhsHo/keZLJt5Qds+iS73N4Q6Xt 9SuYgYPjXFcA== X-IronPort-AV: E=McAfee;i="6000,8403,9684"; a="129136855" X-IronPort-AV: E=Sophos;i="5.75,362,1589266800"; d="scan'208";a="129136855" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Jul 2020 04:53:26 -0700 IronPort-SDR: IEwCdAr50hSwU22mtfphZLcThxMZ2kqZ5KwkbyPhY7Ev1US5dyeN/esi9I+3SpLuqOnfqwQ05B 3CPZ3k+RqxlA== X-IronPort-AV: E=Sophos;i="5.75,362,1589266800"; d="scan'208";a="486941530" Received: from bricha3-mobl.ger.corp.intel.com ([10.213.193.115]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 17 Jul 2020 04:53:24 -0700 Date: Fri, 17 Jul 2020 12:53:18 +0100 From: Bruce Richardson To: Ciara Power Cc: kevin.laatz@intel.com, thomas@monjalon.net, ferruh.yigit@intel.com, arybchenko@solarflare.com, dev@dpdk.org, keith.wiles@intel.com, Louise Kilheeney Message-ID: <20200717115318.GA708@bricha3-MOBL.ger.corp.intel.com> References: <20200612105344.15383-1-ciara.power@intel.com> <20200715122935.1822-1-ciara.power@intel.com> <20200715122935.1822-3-ciara.power@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200715122935.1822-3-ciara.power@intel.com> Subject: Re: [dpdk-dev] [PATCH v5 2/3] test/test_telemetry_data: add unit tests for data to JSON X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jul 15, 2020 at 01:29:34PM +0100, Ciara Power wrote: > From: Louise Kilheeney > > 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 > Signed-off-by: Ciara Power 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 > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#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. > +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