Re: [dpdk-dev] [PATCH] test: add unit tests for metrics library

[Remy Horton <remy.horton@intel.com>]

'noon,

See inline comments.

On 05/07/2018 08:37, Hari kumar Vemula wrote:
> Unit Testcases are added for metrics library.

[..]

> +/* Test case to validate registering a list of valid  metric names */
> +static int
> +test_metrics_reg_names(void)
> +{
> +	int err = 0;
> +	const char * const mnames[] = {
> +		"mean_bits_in", "mean_bits_out",
> +		"peak_bits_in", "peak_bits_out",
> +		};
> +
> +
> +	/* Success Test: valid array and count size */
> +	err = rte_metrics_reg_names(&mnames[0], ARRAY_SIZE(mnames));
> +	TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);
> +
> +	/* Failure Test: valid array and higher count size than array size*/
> +	err = rte_metrics_reg_names(&mnames[0], ARRAY_SIZE(mnames) + 2);
> +	TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);

rte_metrics_reg_names() relies on cnt_names being truthful about the 
size of *names. If it is overstated there is nothing to prevent an array 
overrun and in most cases a core-dump.

> +
> +	/* Failure Test: valid array and count size lessthan 1*/
> +	err = rte_metrics_reg_names(&mnames[0], 0);
> +	TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);
> +
> +	/* Failure Test: valid array and count exceeds max count  */
> +	err = rte_metrics_reg_names(&mnames[0], INVALID_COUNT);
> +	TEST_ASSERT(err == -ENOMEM, "%s, %d", __func__, __LINE__);

See previous comment.

> +
> +	/* Failure Test: Invalid array and valid count size */
> +	err = rte_metrics_reg_names(NULL, ARRAY_SIZE(mnames) + 2);
> +	TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);
> +
> +	/* Failure Test: Valid array and Invalid count size */
> +	err = rte_metrics_reg_names(&mnames[0], INVAL_METRIC_COUNT);
> +	TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);

Ditto.

> +
> +	return TEST_SUCCESS;
> +}
> +

[..]

> +/* Test case to validate update a list of  metrics  */
> +static int
> +test_metrics_update_values(void)
> +{
> +	int err = 0;
> +	const uint64_t  value[REG_METRIC_COUNT] = {1, 2, 3, 4, 5, 6};
> +
> +	/* Success Test: valid data */
> +	err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
> +			 KEY, &value[0], ARRAY_SIZE(value));
> +	TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);

Updates cannot overlap more than one metric set. A successful test would be:

         /* Success Test: valid data */
         err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 0,
                 &value[0], 1);
         TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);
         err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 1,
                 &value[1], 1);
         TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);
         err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 2,
                 &value[2], 4);
         TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);

I am wondering whether this constraint should be removed.


> +	/* Failed Test: Invalid count size */
> +	err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
> +			 KEY, &value[0], 0);
> +	TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);

Good catch. Will update library code.


> +
> +	/* Success Test: valid data with lower count stats size */
> +	err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
> +			 KEY, &value[0], ARRAY_SIZE(value) - 3);
> +	TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__);

KEY should be 2, which is where the set of size four starts. The set 
starting at KEY (i.e 1) is of size one.


> +
> +	/* Failed Test: Invalid port_id(lower value) and valid data */
> +	err = rte_metrics_update_values(-2, KEY, &value[0], ARRAY_SIZE(value));
> +	TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);
> +
> +	/* Failed Test: Invalid port_id(higher value) and valid data */
> +	err = rte_metrics_update_values(39, 1, &value[0], ARRAY_SIZE(value));
> +	TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);

Testcases Ok.

> +
> +	/* Failed Test: higher count size */
> +	err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
> +			 KEY, &value[0], ARRAY_SIZE(value) + 5);
> +	TEST_ASSERT(err == -ERANGE, "%s, %d", __func__, __LINE__);

Array sizes should not be overstated.

> +
> +	/* Failed Test: Invalid array */
> +	err = rte_metrics_update_values(RTE_METRICS_GLOBAL,
> +			 KEY, NULL, ARRAY_SIZE(value));
> +	TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__);
> +
> +	return TEST_SUCCESS;
> +}


> +
> +/* Test to validate get metric name-key lookup table */
> +static int
> +test_metrics_get_names(void)
> +{
[..]

> +
> +	/* Failure Test: Invalid array list, Correct Count Stats same as
> +	 * memzone stats
> +	 */
> +	err = rte_metrics_get_names(metrics, REG_METRIC_COUNT);
> +	TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);

Overstated array size.

> +
> +	/* Failure Test: Invalid array list, Increased Count Stats */
> +	err = rte_metrics_get_names(metrics, REG_METRIC_COUNT+9);
> +	TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__);

Overstated array size.


> +
> +	return TEST_SUCCESS;
> +}
> +
> +/* Test to validate get list of metric values  */


..Remy