From: Phillip Wood <phillip.wood123@gmail.com>
To: Chandra Pratap via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: Chandra Pratap <chandrapratap3519@gmail.com>,
Chandra Pratap <chandrapratap376@gmail.com>,
Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v3] tests: move t0009-prio-queue.sh to the new unit testing framework
Date: Sat, 20 Jan 2024 14:23:09 +0000 [thread overview]
Message-ID: <611378dc-dfe7-446b-803d-ee933b50fdbf@gmail.com> (raw)
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
next prev parent reply other threads:[~2024-01-20 14:23 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-14 8:10 [PATCH] tests: move t0009-prio-queue.sh to the new unit testing framework Chandra Pratap via GitGitGadget
2024-01-14 8:15 ` Chandra Pratap
2024-01-14 8:18 ` [PATCH v2] " Chandra Pratap via GitGitGadget
2024-01-16 16:36 ` Junio C Hamano
2024-01-17 6:01 ` Josh Steadmon
2024-01-17 14:38 ` [PATCH v3] " Chandra Pratap via GitGitGadget
2024-01-17 21:52 ` Junio C Hamano
2024-01-17 21:58 ` Junio C Hamano
2024-01-17 22:15 ` Junio C Hamano
2024-01-20 2:31 ` Jeff King
2024-01-20 14:23 ` Phillip Wood [this message]
2024-01-21 19:28 ` [PATCH v4] " Chandra Pratap via GitGitGadget
2024-01-22 19:09 ` Junio C Hamano
2024-01-22 22:42 ` Jeff King
2024-01-25 20:02 ` [PATCH v5] " Chandra Pratap via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=611378dc-dfe7-446b-803d-ee933b50fdbf@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=chandrapratap3519@gmail.com \
--cc=chandrapratap376@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).