From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sat, 20 Jan 2024 14:23:09 GMT
Subject: Re: [PATCH v3] tests: move t0009-prio-queue.sh to the new unit testing framework
Message-ID: <611378dc-dfe7-446b-803d-ee933b50fdbf@gmail.com>
In-Reply-To: <pull.1642.v3.git.1705502304219.gitgitgadget@gmail.com>

On 17/01/2024 14:38, Chandra Pratap via GitGitGadget wrote:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
> 
> t/t0009-prio-queue.sh along with t/helper/test-prio-queue.c unit
> tests Git's implementation of a priority queue. Migrate the
> test over to the new unit testing framework to simplify debugging
> and reduce test run-time. Refactor the required logic and add
> a new test case in addition to porting over the original ones in
> shell.

Thanks for working on this, it is good to see people exploring the unit 
test framework. I agree with the points that Junio and Peff have already 
made, I've got a couple of additional comments below.

 > [...]
> +static int test_prio_queue(int *input, int *result)

This function does not need to return a value.

> +{
> +	struct prio_queue pq = { intcmp };
> +	int i = 0;
> +
> +	while (*input) {

I agree with Junio that a for() loop would be better here, but I think 
that rather than testing for '0' we should just pass the length of the 
input array to this function.

> +		int *val = input++;
> +		void *peek, *get;
> +		switch(*val) {
> +			case GET:
> +				peek = prio_queue_peek(&pq);
> +				get = prio_queue_get(&pq);
> +				if (peek != get)
> +					BUG("peek and get results don't match");

If possible avoid calling BUG() in unit test code as it aborts the whole 
test program, not just the current test. You can use check() and return 
early instead

	if (!check(peek, !=, get))
		return;

> +				result[i++] = show(get);

I agree with Peff that it would be safer just to use check_int() here. 
If you switch to a for loop with an index you can also print the current 
index with

	test_msg("index %d", i);

Which will help diagnose which item in the queue is failing.

Alternatively we could grow result dynamically using ALLOC_GROW() and 
add a function to compare two arrays of integers. Such a function should 
take the two lengths, one for each array.

Best Wishes

Phillip