Subject: Re: [PATCH 3/4] t/unit-tests: adapt priority queue test to use clar test framework

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 16 Jan 2025 14:13:02 +0100

On Thu, Jan 16, 2025 at 11:49:10AM +0100, Seyi Kuforiji wrote:
> diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
> deleted file mode 100644
> index a053635000..0000000000
> --- a/t/unit-tests/t-prio-queue.c
> +++ /dev/null

Hm. A bit surprising that Git decides to not render this as a rename, as
most of `test_prio_queue()` is unchanged.

> diff --git a/t/unit-tests/u-prio-queue.c b/t/unit-tests/u-prio-queue.c
> new file mode 100644
> index 0000000000..d36a565e6f
> --- /dev/null
> +++ b/t/unit-tests/u-prio-queue.c
> @@ -0,0 +1,94 @@
> +#include "unit-test.h"
> +#include "prio-queue.h"
> +
> +static int intcmp(const void *va, const void *vb, void *data UNUSED)
> +{
> +	const int *a = va, *b = vb;
> +	return *a - *b;
> +}
> +
> +
> +#define MISSING  -1
> +#define DUMP	 -2
> +#define STACK	 -3
> +#define GET	 -4
> +#define REVERSE  -5
> +
> +static int show(int *v)
> +{
> +	return v ? *v : MISSING;
> +}
> +
> +static void test_prio_queue(int *input, size_t input_size,
> +			    int *result, size_t result_size)
> +{
> +	struct prio_queue pq = { intcmp };
> +	size_t j = 0;

This is a `size_t` now, which is different compared to before. Might be
worthwhile to point out why you did this in the commit message.

> +	for (size_t i = 0; i < input_size; i++) {
> +		void *peek, *get;
> +		switch(input[i]) {
> +		case GET:
> +			peek = prio_queue_peek(&pq);
> +			get = prio_queue_get(&pq);
> +			cl_assert(peek == get);
> +			cl_assert(j < result_size);
> +			cl_assert_equal_i(result[j], show(get));
> +			j++;
> +			break;
> +		case DUMP:
> +			while ((peek = prio_queue_peek(&pq))) {
> +				get = prio_queue_get(&pq);
> +				cl_assert(peek == get);
> +				cl_assert((size_t)j < result_size);

This here is the reason, to avoid -Wsign-compare. But the cast here
isn't necessary now that you've adapted `j` to be a `size_t` anyway.

> +				cl_assert_equal_i(result[j], show(get));
> +				j++;
> +			}
> +			break;
> +		case STACK:
> +			pq.compare = NULL;
> +			break;
> +		case REVERSE:
> +			prio_queue_reverse(&pq);
> +			break;
> +		default:
> +			prio_queue_put(&pq, &input[i]);
> +			break;
> +		}
> +	}
> +	cl_assert_equal_i(j, result_size);
> +	clear_prio_queue(&pq);
> +}
> +
> +#define TEST_INPUT(input, result) \
> +	test_prio_queue(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result))
> +
> +void test_prio_queue__basic(void)
> +{
> +	TEST_INPUT(((int []){ 2, 6, 3, 10, 9, 5, 7, 4, 5, 8, 1, DUMP }),
> +		   ((int []){ 1, 2, 3, 4, 5, 5, 6, 7, 8, 9, 10 }));
> +}
> +
> +void test_prio_queue__mixed(void)
> +{
> +	TEST_INPUT(((int []){ 6, 2, 4, GET, 5, 3, GET, GET, 1, DUMP }),
> +		   ((int []){ 2, 3, 4, 1, 5, 6 }));
> +}
> +
> +void test_prio_queue__empty(void)
> +{
> +	TEST_INPUT(((int []){ 1, 2, GET, GET, GET, 1, 2, GET, GET, GET }),
> +		   ((int []){ 1, 2, MISSING, 1, 2, MISSING }));
> +}
> +
> +void test_prio_queue__stack(void)
> +{
> +	TEST_INPUT(((int []){ STACK, 8, 1, 5, 4, 6, 2, 3, DUMP }),
> +		   ((int []){ 3, 2, 6, 4, 5, 1, 8 }));
> +}
> +
> +void test_prio_queue__reverse_stack(void)
> +{
> +	TEST_INPUT(((int []){ STACK, 1, 2, 3, 4, 5, 6, REVERSE, DUMP }),
> +		   ((int []){ 1, 2, 3, 4, 5, 6 }));
> +}

All of these look like failthful conversions to me.

Patrick