Re: [PATCH 5/5] histograms: Add struct traceeval unit tests

[Ross Zwisler <zwisler@google.com>]

On Fri, Jul 28, 2023 at 03:04:40PM -0400, Stevie Alvarez wrote:
> From: "Stevie Alvarez (Google)" <stevie.6strings@gmail.com>
> 
> Add unit tests for traceeval_init(), traceeval_release(), and
> traceeval_compare() to check edge cases and ensure the interface is
> functional.
> 
> Signed-off-by: Stevie Alvarez (Google) <stevie.6strings@gmail.com>
> ---
>  utest/.gitignore        |   1 +
>  utest/Makefile          |  35 +++++++
>  utest/eval-test.h       |  10 ++
>  utest/eval-utest.c      |  27 +++++
>  utest/traceeval-utest.c | 222 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 295 insertions(+)
>  create mode 100644 utest/.gitignore
>  create mode 100644 utest/Makefile
>  create mode 100644 utest/eval-test.h
>  create mode 100644 utest/eval-utest.c
>  create mode 100644 utest/traceeval-utest.c
> 
> diff --git a/utest/.gitignore b/utest/.gitignore
> new file mode 100644
> index 0000000..ca0ac10
> --- /dev/null
> +++ b/utest/.gitignore
> @@ -0,0 +1 @@
> +eval-utest
> diff --git a/utest/Makefile b/utest/Makefile
> new file mode 100644
> index 0000000..955f91e
> --- /dev/null
> +++ b/utest/Makefile
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: MIT
> +
> +include $(src)/scripts/utils.mk
> +
> +bdir := $(obj)/utest
> +eval_lib := $(obj)/lib/libtraceeval.a
> +
> +TARGETS = $(bdir)/eval-utest
> +
> +OBJS := eval-utest.o
> +OBJS += traceeval-utest.o
> +
> +LIBS += -lcunit				\
> +	-ldl				\
> +	$(eval_lib)
> +
> +OBJS := $(OBJS:%.o=$(bdir)/%.o)
> +
> +$(bdir):
> +	@mkdir -p $(bdir)
> +
> +$(OBJS): | $(bdir)
> +
> +$(bdir)/eval-utest: $(OBJS) $(eval_lib)
> +	$(Q)$(do_app_build)
> +
> +$(bdir)/%.o: %.c
> +	$(Q)$(call do_fpic_compile)
> +
> +-include .*.d
> +
> +test: $(TARGETS)
> +
> +clean:
> +	$(Q)$(call do_clean,$(TARGETS) $(bdir)/*.o $(bdir)/.*.d)
> diff --git a/utest/eval-test.h b/utest/eval-test.h
> new file mode 100644
> index 0000000..f3372e8
> --- /dev/null
> +++ b/utest/eval-test.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2023 Google Inc, Stevie Alvarez <stevie.6strings@gmail.com>
> + */
> +#ifndef _EVAL_UTEST_H_
> +#define _EVAL_UTEST_H_
> +
> +void test_traceeval_lib(void);
> +
> +#endif /* _EVAL_UTEST_H_ */
> diff --git a/utest/eval-utest.c b/utest/eval-utest.c
> new file mode 100644
> index 0000000..771a0c4
> --- /dev/null
> +++ b/utest/eval-utest.c
> @@ -0,0 +1,27 @@
> +
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2023 Google Inc, Stevie Alvarez <stevie.6strings@gmail.com>
> + */
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include <CUnit/CUnit.h>
> +#include <CUnit/Basic.h>
> +
> +#include "eval-test.h"
> +
> +int main(int argc, char **argv)
> +{
> +	if (CU_initialize_registry() != CUE_SUCCESS) {
> +		printf("Test registry cannot be initialized\n");
> +		return EXIT_FAILURE;
> +	}
> +
> +	test_traceeval_lib();
> +
> +	CU_basic_set_mode(CU_BRM_VERBOSE);
> +	CU_basic_run_tests();
> +	CU_cleanup_registry();
> +	return EXIT_SUCCESS;
> +}
> diff --git a/utest/traceeval-utest.c b/utest/traceeval-utest.c
> new file mode 100644
> index 0000000..2bcb4b9
> --- /dev/null
> +++ b/utest/traceeval-utest.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2023 Google Inc, Stevie Alvarez <stevie.6strings@gmail.com>
> + */
> +
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <CUnit/CUnit.h>
> +#include <CUnit/Basic.h>
> +
> +#include <histograms.h>
> +
> +#define TRACEEVAL_SUITE		"traceeval library"
> +#define TRACEEVAL_SUCCESS	0
> +#define TRACEEVAL_FAILURE	-1
> +#define TRACEEVAL_NOT_SAME	1
> +
> +/**
> + * Test traceeval_init(), traceeval_release(), and traceeval_compare() with
> + * NULL values.
> + */
> +void test_eval_null(void)

All functions except test_traceeval_lib() should be static.

> +{
> +	// set up
> +	char *name = "test null";
> +	enum traceeval_data_type type = TRACEEVAL_TYPE_NONE;
> +	const struct traceeval_type test_data[] =  {
> +		{
> +			.type = TRACEEVAL_TYPE_NUMBER,
> +			.name = name
> +		},
> +		{
> +			.type = type,
			.type = TRACEEVAL_TYPE_NONE,

and you can get rid of the 'type' variable.

> +			.name = NULL
> +		}
> +	};
> +
> +	// test init
> +	struct traceeval *result_null = traceeval_init(NULL, NULL);
> +	struct traceeval *result_key = traceeval_init(test_data, NULL);
> +	struct traceeval *result_val = traceeval_init(NULL, test_data);
> +	
> +	// analyze init
> +	CU_ASSERT(!result_null);
> +	CU_ASSERT(result_key != NULL);
> +	CU_ASSERT(!result_val);
> +
> +	// release
> +	traceeval_release(NULL);
> +	traceeval_release(result_key);
> +}
> +
> +/**
> + * Use provided data to test traceeval_init(), traceeval_compare(), and
> + * traceeval_release().
> + */
> +void test_eval_base(const struct traceeval_type *keys1,
> +		const struct traceeval_type *vals1,
> +		const struct traceeval_type *keys2,
> +		const struct traceeval_type *vals2,
> +		bool init_not_null1, bool init_not_null2,
> +		int compare_result)
> +{
> +	struct traceeval *init1;
> +	struct traceeval *init2;
> +	int result;
> +
> +	// test init
> +	init1 = traceeval_init(keys1, vals1);
> +	init2 = traceeval_init(keys2, vals2);
> +
> +	result = init1 != NULL;
> +	if (init_not_null1) {
> +		CU_ASSERT(result);
> +	} else {
> +		CU_ASSERT(!result);
> +	}
> +
> +	result = init2 != NULL;
> +	if (init_not_null2) {
> +		CU_ASSERT(result);
> +	} else {
> +		CU_ASSERT(!result);
> +	}
> +
> +	// test compare
> +	result = traceeval_compare(init1, init2);
> +	
> +	// analyze compare
> +	CU_ASSERT(result == compare_result);
> +	
> +	// release
> +	traceeval_release(init1);
> +	traceeval_release(init2);
> +}
> +
> +/**
> + * Test traceeval_init(), traceeval_release(), and traceeval_compare() with
> + * TRACEEVAL_TYPE_NONE.
> + */
> +void test_eval_none(void)
> +{
> +	// set up
> +	char *name = "test none";
> +	char *name2 = "test none (some)";
> +	const struct traceeval_type test_data_none[] =  {
> +		{
> +			.type = TRACEEVAL_TYPE_NONE,
> +			.name = name
> +		}
> +	};
> +	const struct traceeval_type test_data_some[] =  {
> +		{
> +			.type = TRACEEVAL_TYPE_NUMBER,
> +			.name = name2
> +		},
> +		{
> +			.type = TRACEEVAL_TYPE_NONE,
> +			.name = NULL
> +		}
> +	};
> +
> +	test_eval_base(test_data_some, test_data_none, test_data_some,
> +			test_data_none, true, true, TRACEEVAL_SUCCESS);
> +	test_eval_base(test_data_none, test_data_none, test_data_some,
> +			test_data_none, false, true, TRACEEVAL_FAILURE);
> +	test_eval_base(test_data_none, test_data_none, test_data_none,
> +			test_data_none, false, false, TRACEEVAL_FAILURE);
> +}
> +
> +/**
> + * Test traceeval_init() and traceeval_release() with equivalent values.
> + */
> +void test_eval_same(void)
> +{
> +	// set up
> +	char *name = "test data";
> +	char *name2 = "test done";

        char *name2 = "test none";
        ?

> +	const struct traceeval_type test_data[] =  {
> +		{
> +			.type = TRACEEVAL_TYPE_NUMBER,
> +			.name = name
> +		},
> +		{
> +			.type = TRACEEVAL_TYPE_NONE,
> +			.name = name2
> +		}
> +	};
> +	
> +	test_eval_base(test_data, test_data, test_data, test_data, true, true,
> +			TRACEEVAL_SUCCESS);
> +}
> +
> +/**
> + * Test traceeval_init() and traceeval_release() with non-equivalent values.
> + */
> +void test_eval_not_same(void)
> +{
> +	const struct traceeval_type test_data1[] =  {
> +		{
> +			.type = TRACEEVAL_TYPE_NUMBER,
> +			.name = "test data 1"

nit: maybe just inline all the names in other test functions, like you have in
this one?  Saves some variables & is more readable.

> +		},
> +		{
> +			.type = TRACEEVAL_TYPE_NONE,
> +			.name = NULL
> +		}
> +	};
> +	const struct traceeval_type test_data2[] =  {
> +		{
> +			.type = TRACEEVAL_TYPE_NUMBER,
> +			.name = "test data 2"
> +		},
> +		{
> +			.type = TRACEEVAL_TYPE_NONE,
> +			.name = NULL
> +		}
> +	};
> +
> +	// type 1 key diff
> +	test_eval_base(test_data2, test_data1, test_data1, test_data1, true,
> +			true, TRACEEVAL_NOT_SAME);
> +	// type 1 data diff
> +	test_eval_base(test_data1, test_data2, test_data1, test_data1, true,
> +			true, TRACEEVAL_NOT_SAME);
> +	// type 2 key diff
> +	test_eval_base(test_data1, test_data1, test_data2, test_data1, true,
> +			true, TRACEEVAL_NOT_SAME);
> +	// type 2 data diff
> +	test_eval_base(test_data1, test_data1, test_data1, test_data2, true,
> +			true, TRACEEVAL_NOT_SAME);
> +}
> +
> +/**
> + * Tests the traceeval_init() and traceeval_release() functions.
> + */
> +void test_eval(void)
> +{
> +	test_eval_null();
> +	test_eval_none();
> +	test_eval_same();
> +	test_eval_not_same();
> +}
> +
> +/**
> + * Register tests with CUnit.
> + */
> +void test_traceeval_lib(void)
> +{
> +	CU_pSuite suite = NULL;
> +	
> +	// set up suite
> +	suite = CU_add_suite(TRACEEVAL_SUITE, NULL, NULL);
> +	if (suite == NULL) {
> +		fprintf(stderr, "Suite %s cannot be created\n", TRACEEVAL_SUITE);
> +		return;
> +	}
> +
> +	// add tests to suite
> +	CU_add_test(suite, "Test traceeval alloc and release", test_eval);
> +}
> -- 
> 2.41.0
>