tests: move t0009-prio-queue.sh to the new unit testing framework #1642
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit 38e7775: |
|
Preview email sent as pull.1642.git.1705215008529.gitgitgadget@gmail.com |
|
Preview email sent as pull.1642.git.1705217183977.gitgitgadget@gmail.com |
|
Preview email sent as pull.1642.git.1705217746789.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.1642.git.1705218013046.gitgitgadget@gmail.com |
|
Preview email sent as pull.1642.git.1705218933873.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.1642.git.1705219312877.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1642.git.1705219829965.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
/submit |
|
Submitted as pull.1642.v2.git.1705220304781.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Chandra Pratap wrote (reply to this): Please ignore the patch above, my e-mail client seems to have
messed up the backslash indentation. I will follow up with the
corrected patch shortly.
On Sun, 14 Jan 2024 at 13:40, Chandra Pratap via GitGitGadget
<gitgitgadget@gmail.com> 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.
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> tests: move t0009-prio-queue.sh to the new unit testing framework
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1642%2FChand-ra%2Fprio-queue-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1642/Chand-ra/prio-queue-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1642
>
> Makefile | 2 +-
> t/helper/test-prio-queue.c | 51 -------------------
> t/helper/test-tool.c | 1 -
> t/helper/test-tool.h | 1 -
> t/t0009-prio-queue.sh | 66 -------------------------
> t/unit-tests/t-prio-queue.c | 99 +++++++++++++++++++++++++++++++++++++
> 6 files changed, 100 insertions(+), 120 deletions(-)
> delete mode 100644 t/helper/test-prio-queue.c
> delete mode 100755 t/t0009-prio-queue.sh
> create mode 100644 t/unit-tests/t-prio-queue.c
>
> diff --git a/Makefile b/Makefile
> index 312d95084c1..d5e48e656b3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -828,7 +828,6 @@ TEST_BUILTINS_OBJS += test-partial-clone.o
> TEST_BUILTINS_OBJS += test-path-utils.o
> TEST_BUILTINS_OBJS += test-pcre2-config.o
> TEST_BUILTINS_OBJS += test-pkt-line.o
> -TEST_BUILTINS_OBJS += test-prio-queue.o
> TEST_BUILTINS_OBJS += test-proc-receive.o
> TEST_BUILTINS_OBJS += test-progress.o
> TEST_BUILTINS_OBJS += test-reach.o
> @@ -1340,6 +1339,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
>
> UNIT_TEST_PROGRAMS += t-basic
> UNIT_TEST_PROGRAMS += t-strbuf
> +UNIT_TEST_PROGRAMS += t-prio-queue
> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
> deleted file mode 100644
> index f0bf255f5f0..00000000000
> --- a/t/helper/test-prio-queue.c
> +++ /dev/null
> @@ -1,51 +0,0 @@
> -#include "test-tool.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;
> -}
> -
> -static void show(int *v)
> -{
> - if (!v)
> - printf("NULL\n");
> - else
> - printf("%d\n", *v);
> - free(v);
> -}
> -
> -int cmd__prio_queue(int argc UNUSED, const char **argv)
> -{
> - struct prio_queue pq = { intcmp };
> -
> - while (*++argv) {
> - if (!strcmp(*argv, "get")) {
> - void *peek = prio_queue_peek(&pq);
> - void *get = prio_queue_get(&pq);
> - if (peek != get)
> - BUG("peek and get results do not match");
> - show(get);
> - } else if (!strcmp(*argv, "dump")) {
> - void *peek;
> - void *get;
> - while ((peek = prio_queue_peek(&pq))) {
> - get = prio_queue_get(&pq);
> - if (peek != get)
> - BUG("peek and get results do not match");
> - show(get);
> - }
> - } else if (!strcmp(*argv, "stack")) {
> - pq.compare = NULL;
> - } else {
> - int *v = xmalloc(sizeof(*v));
> - *v = atoi(*argv);
> - prio_queue_put(&pq, v);
> - }
> - }
> -
> - clear_prio_queue(&pq);
> -
> - return 0;
> -}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 876cd2dc313..5f591b9718f 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -57,7 +57,6 @@ static struct test_cmd cmds[] = {
> { "path-utils", cmd__path_utils },
> { "pcre2-config", cmd__pcre2_config },
> { "pkt-line", cmd__pkt_line },
> - { "prio-queue", cmd__prio_queue },
> { "proc-receive", cmd__proc_receive },
> { "progress", cmd__progress },
> { "reach", cmd__reach },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 70dd4eba119..b921138d8ec 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -50,7 +50,6 @@ int cmd__partial_clone(int argc, const char **argv);
> int cmd__path_utils(int argc, const char **argv);
> int cmd__pcre2_config(int argc, const char **argv);
> int cmd__pkt_line(int argc, const char **argv);
> -int cmd__prio_queue(int argc, const char **argv);
> int cmd__proc_receive(int argc, const char **argv);
> int cmd__progress(int argc, const char **argv);
> int cmd__reach(int argc, const char **argv);
> diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh
> deleted file mode 100755
> index eea99107a48..00000000000
> --- a/t/t0009-prio-queue.sh
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -#!/bin/sh
> -
> -test_description='basic tests for priority queue implementation'
> -
> -TEST_PASSES_SANITIZE_LEAK=true
> -. ./test-lib.sh
> -
> -cat >expect <<'EOF'
> -1
> -2
> -3
> -4
> -5
> -5
> -6
> -7
> -8
> -9
> -10
> -EOF
> -test_expect_success 'basic ordering' '
> - test-tool prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -2
> -3
> -4
> -1
> -5
> -6
> -EOF
> -test_expect_success 'mixed put and get' '
> - test-tool prio-queue 6 2 4 get 5 3 get get 1 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -1
> -2
> -NULL
> -1
> -2
> -NULL
> -EOF
> -test_expect_success 'notice empty queue' '
> - test-tool prio-queue 1 2 get get get 1 2 get get get >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -3
> -2
> -6
> -4
> -5
> -1
> -8
> -EOF
> -test_expect_success 'stack order' '
> - test-tool prio-queue stack 8 1 5 4 6 2 3 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -test_done
> diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
> new file mode 100644
> index 00000000000..98a69790f7e
> --- /dev/null
> +++ b/t/unit-tests/t-prio-queue.c
> @@ -0,0 +1,99 @@
> +#include "test-lib.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;
> +}
> +
> +static int show(int *v)
> +{
> + int ret = -1;
> + if (v)
> + ret = *v;
> + free(v);
> + return ret;
> +}
> +
> +static int test_prio_queue(const char **input, int *result)
> +{
> + struct prio_queue pq = { intcmp };
> + int i = 0;
> +
> + while (*input) {
> + if (!strcmp(*input, "get")) {
> + void *peek = prio_queue_peek(&pq);
> + void *get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + } else if (!strcmp(*input, "dump")) {
> + void *peek;
> + void *get;
> + while ((peek = prio_queue_peek(&pq))) {
> + get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + }
> + } else if (!strcmp(*input, "stack")) {
> + pq.compare = NULL;
> + } else if (!strcmp(*input, "reverse")) {
> + prio_queue_reverse(&pq);
> + } else {
> + int *v = xmalloc(sizeof(*v));
> + *v = atoi(*input);
> + prio_queue_put(&pq, v);
> + }
> + input++;
> + }
> +
> + clear_prio_queue(&pq);
> +
> + return 0;
> +}
> +
> +#define INPUT_SIZE 6
> +
> +#define BASIC_INPUT "1", "2", "3", "4", "5", "5", "dump"
> +#define BASIC_EXPECTED 1, 2, 3, 4, 5, 5
> +
> +#define MIXED_PUT_GET_INPUT "6", "2", "4", "get", "5", "3", "get", "get", "1", "dump"
> +#define MIXED_PUT_GET_EXPECTED 2, 3, 4, 1, 5, 6
> +
> +#define EMPTY_QUEUE_INPUT "1", "2", "get", "get", "get", "1", "2", "get", "get", "get"
> +#define EMPTY_QUEUE_EXPECTED 1, 2, -1, 1, 2, -1
> +
> +#define STACK_INPUT "stack", "1", "5", "4", "6", "2", "3", "dump"
> +#define STACK_EXPECTED 3, 2, 6, 4, 5, 1
> +
> +#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
> +#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
> +
> +#define TEST_INPUT(INPUT, EXPECTED, name) \
> + static void test_##name(void) \
> +{ \
> + const char *input[] = {INPUT}; \
> + int expected[] = {EXPECTED}; \
> + int result[INPUT_SIZE]; \
> + test_prio_queue(input, result); \
> + check(!memcmp(expected, result, sizeof(expected))); \
> +}
> +
> +TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
> +TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
> +TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
> +TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
> +TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + TEST(test_basic(), "prio-queue works for basic input");
> + TEST(test_mixed(), "prio-queue works for mixed put & get commands");
> + TEST(test_empty(), "prio-queue works when queue is empty");
> + TEST(test_stack(), "prio-queue works when used as a LIFO stack");
> + TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
> +
> + return test_done();
> +}
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
> --
> gitgitgadget |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
> new file mode 100644
> index 00000000000..07b112f5894
> --- /dev/null
> +++ b/t/unit-tests/t-prio-queue.c
> @@ -0,0 +1,99 @@
> +#include "test-lib.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;
> +}
This came from the original and looks obviously OK.
> +static int show(int *v)
> +{
> + int ret = -1;
> + if (v)
> + ret = *v;
> + free(v);
> + return ret;
> +}
The original allowed us to differentiate "prio queue did not yield
anything" case and "the integer obtained from the queue happened to
be negative" cases, but with this, -1 is taken.
It is not a huge loss, as the tests can limit themselves to a subset
of "int" (say, positive integers only). But this realization can
lead to further simplfication.
> +static int test_prio_queue(const char **input, int *result)
This takes an array of strings, but it does not have to. Make it
take an array of "int" and then with something like
#define MISSING -1
#define GET -2
#define DUMP -3
#define STACK -4
you can get rid of strcmp(), atoi(), and xmalloc(), like so:
while (*input) {
int val = *input++;
switch (val) {
case GET:
void *peek = prio_queue_peek(&pq);
...
result[i++] = peek ? (*(int*)peek) : MISSING;
break;
... other "commands" here ...
default:
prio_queue_put(&pq, val);
break;
}
}
I inlined the show() in the above illustration, but you can keep it
as a separate small helper function. The point is that it makes it
far easier to follow by using the MISSING symbolic constant either
way.
> +{
> + struct prio_queue pq = { intcmp };
> + int i = 0;
> +
> + while (*input) {
> + if (!strcmp(*input, "get")) {
> + void *peek = prio_queue_peek(&pq);
> + void *get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + } else if (!strcmp(*input, "dump")) {
> + void *peek;
> + void *get;
> + while ((peek = prio_queue_peek(&pq))) {
> + get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + }
> + } else if (!strcmp(*input, "stack")) {
> + pq.compare = NULL;
> + } else if (!strcmp(*input, "reverse")) {
> + prio_queue_reverse(&pq);
> + } else {
> + int *v = xmalloc(sizeof(*v));
> + *v = atoi(*input);
> + prio_queue_put(&pq, v);
> + }
> + input++;
> + }
> +
> + clear_prio_queue(&pq);
> +
> + return 0;
> +}
> +
> +#define INPUT_SIZE 6
You do not need this (see below)
> +#define BASIC_INPUT "1", "2", "3", "4", "5", "5", "dump"
> +#define BASIC_EXPECTED 1, 2, 3, 4, 5, 5
> +
> +#define MIXED_PUT_GET_INPUT "6", "2", "4", "get", "5", "3", "get", "get", "1", "dump"
> +#define MIXED_PUT_GET_EXPECTED 2, 3, 4, 1, 5, 6
> +
> +#define EMPTY_QUEUE_INPUT "1", "2", "get", "get", "get", "1", "2", "get", "get", "get"
> +#define EMPTY_QUEUE_EXPECTED 1, 2, -1, 1, 2, -1
And these input and expectation can become all integers, i.e.
#define EMPTY_QUEUE_INPUT 1, 2, GET, GET, GET, 1, 2, GET, GET, GET
#define EMPTY_QUEUE_EXPECTED 1, 2, MISSING, 1, 2, MISSING
> +
> +#define STACK_INPUT "stack", "1", "5", "4", "6", "2", "3", "dump"
> +#define STACK_EXPECTED 3, 2, 6, 4, 5, 1
> +
> +#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
> +#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
> +
> +#define TEST_INPUT(INPUT, EXPECTED, name) \
> + static void test_##name(void) \
> +{ \
> + const char *input[] = {INPUT}; \
> + int expected[] = {EXPECTED}; \
> + int result[INPUT_SIZE]; \
This can be written more like
int result[ARRAY_SIZE(expected)]
so that you can freely extend each test without having to worry
about increasing the hardcoded limit.
> + test_prio_queue(input, result); \
> + check(!memcmp(expected, result, sizeof(expected))); \
> +}
> +
> +TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
> +TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
> +TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
> +TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
> +TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + TEST(test_basic(), "prio-queue works for basic input");
> + TEST(test_mixed(), "prio-queue works for mixed put & get commands");
> + TEST(test_empty(), "prio-queue works when queue is empty");
> + TEST(test_stack(), "prio-queue works when used as a LIFO stack");
> + TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
> +
> + return test_done();
> +}
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996 |
|
On the Git mailing list, Josh Steadmon wrote (reply to this): On 2024.01.14 08:18, 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.
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> tests: move t0009-prio-queue.sh to the new unit testing framework
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1642%2FChand-ra%2Fprio-queue-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1642/Chand-ra/prio-queue-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1642
>
> Range-diff vs v1:
>
> 1: ca20abe95ea ! 1: 60ac9b3c259 tests: move t0009-prio-queue.sh to the new unit testing framework
> @@ t/unit-tests/t-prio-queue.c (new)
> +#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
> +#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
> +
> -+#define TEST_INPUT(INPUT, EXPECTED, name) \
> ++#define TEST_INPUT(INPUT, EXPECTED, name) \
> + static void test_##name(void) \
> -+{ \
> -+ const char *input[] = {INPUT}; \
> -+ int expected[] = {EXPECTED}; \
> -+ int result[INPUT_SIZE]; \
> -+ test_prio_queue(input, result); \
> ++{ \
> ++ const char *input[] = {INPUT}; \
> ++ int expected[] = {EXPECTED}; \
> ++ int result[INPUT_SIZE]; \
> ++ test_prio_queue(input, result); \
> + check(!memcmp(expected, result, sizeof(expected))); \
> +}
> +
>
>
> Makefile | 2 +-
> t/helper/test-prio-queue.c | 51 -------------------
> t/helper/test-tool.c | 1 -
> t/helper/test-tool.h | 1 -
> t/t0009-prio-queue.sh | 66 -------------------------
> t/unit-tests/t-prio-queue.c | 99 +++++++++++++++++++++++++++++++++++++
> 6 files changed, 100 insertions(+), 120 deletions(-)
> delete mode 100644 t/helper/test-prio-queue.c
> delete mode 100755 t/t0009-prio-queue.sh
> create mode 100644 t/unit-tests/t-prio-queue.c
>
> diff --git a/Makefile b/Makefile
> index 312d95084c1..d5e48e656b3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -828,7 +828,6 @@ TEST_BUILTINS_OBJS += test-partial-clone.o
> TEST_BUILTINS_OBJS += test-path-utils.o
> TEST_BUILTINS_OBJS += test-pcre2-config.o
> TEST_BUILTINS_OBJS += test-pkt-line.o
> -TEST_BUILTINS_OBJS += test-prio-queue.o
> TEST_BUILTINS_OBJS += test-proc-receive.o
> TEST_BUILTINS_OBJS += test-progress.o
> TEST_BUILTINS_OBJS += test-reach.o
> @@ -1340,6 +1339,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
>
> UNIT_TEST_PROGRAMS += t-basic
> UNIT_TEST_PROGRAMS += t-strbuf
> +UNIT_TEST_PROGRAMS += t-prio-queue
> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
> deleted file mode 100644
> index f0bf255f5f0..00000000000
> --- a/t/helper/test-prio-queue.c
> +++ /dev/null
> @@ -1,51 +0,0 @@
> -#include "test-tool.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;
> -}
> -
> -static void show(int *v)
> -{
> - if (!v)
> - printf("NULL\n");
> - else
> - printf("%d\n", *v);
> - free(v);
> -}
> -
> -int cmd__prio_queue(int argc UNUSED, const char **argv)
> -{
> - struct prio_queue pq = { intcmp };
> -
> - while (*++argv) {
> - if (!strcmp(*argv, "get")) {
> - void *peek = prio_queue_peek(&pq);
> - void *get = prio_queue_get(&pq);
> - if (peek != get)
> - BUG("peek and get results do not match");
> - show(get);
> - } else if (!strcmp(*argv, "dump")) {
> - void *peek;
> - void *get;
> - while ((peek = prio_queue_peek(&pq))) {
> - get = prio_queue_get(&pq);
> - if (peek != get)
> - BUG("peek and get results do not match");
> - show(get);
> - }
> - } else if (!strcmp(*argv, "stack")) {
> - pq.compare = NULL;
> - } else {
> - int *v = xmalloc(sizeof(*v));
> - *v = atoi(*argv);
> - prio_queue_put(&pq, v);
> - }
> - }
> -
> - clear_prio_queue(&pq);
> -
> - return 0;
> -}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 876cd2dc313..5f591b9718f 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -57,7 +57,6 @@ static struct test_cmd cmds[] = {
> { "path-utils", cmd__path_utils },
> { "pcre2-config", cmd__pcre2_config },
> { "pkt-line", cmd__pkt_line },
> - { "prio-queue", cmd__prio_queue },
> { "proc-receive", cmd__proc_receive },
> { "progress", cmd__progress },
> { "reach", cmd__reach },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 70dd4eba119..b921138d8ec 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -50,7 +50,6 @@ int cmd__partial_clone(int argc, const char **argv);
> int cmd__path_utils(int argc, const char **argv);
> int cmd__pcre2_config(int argc, const char **argv);
> int cmd__pkt_line(int argc, const char **argv);
> -int cmd__prio_queue(int argc, const char **argv);
> int cmd__proc_receive(int argc, const char **argv);
> int cmd__progress(int argc, const char **argv);
> int cmd__reach(int argc, const char **argv);
> diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh
> deleted file mode 100755
> index eea99107a48..00000000000
> --- a/t/t0009-prio-queue.sh
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -#!/bin/sh
> -
> -test_description='basic tests for priority queue implementation'
> -
> -TEST_PASSES_SANITIZE_LEAK=true
> -. ./test-lib.sh
> -
> -cat >expect <<'EOF'
> -1
> -2
> -3
> -4
> -5
> -5
> -6
> -7
> -8
> -9
> -10
> -EOF
> -test_expect_success 'basic ordering' '
> - test-tool prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -2
> -3
> -4
> -1
> -5
> -6
> -EOF
> -test_expect_success 'mixed put and get' '
> - test-tool prio-queue 6 2 4 get 5 3 get get 1 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -1
> -2
> -NULL
> -1
> -2
> -NULL
> -EOF
> -test_expect_success 'notice empty queue' '
> - test-tool prio-queue 1 2 get get get 1 2 get get get >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -3
> -2
> -6
> -4
> -5
> -1
> -8
> -EOF
> -test_expect_success 'stack order' '
> - test-tool prio-queue stack 8 1 5 4 6 2 3 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -test_done
> diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
> new file mode 100644
> index 00000000000..07b112f5894
> --- /dev/null
> +++ b/t/unit-tests/t-prio-queue.c
> @@ -0,0 +1,99 @@
> +#include "test-lib.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;
> +}
> +
> +static int show(int *v)
> +{
> + int ret = -1;
> + if (v)
> + ret = *v;
> + free(v);
> + return ret;
> +}
> +
> +static int test_prio_queue(const char **input, int *result)
> +{
> + struct prio_queue pq = { intcmp };
> + int i = 0;
> +
> + while (*input) {
> + if (!strcmp(*input, "get")) {
Running the test segfaults at the above strcmp() for me, from the very
first test_basic test case. I'm not sure why, there's not an obvious
problem that I can see. Does it run properly for you?
> + void *peek = prio_queue_peek(&pq);
> + void *get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + } else if (!strcmp(*input, "dump")) {
> + void *peek;
> + void *get;
> + while ((peek = prio_queue_peek(&pq))) {
> + get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + }
> + } else if (!strcmp(*input, "stack")) {
> + pq.compare = NULL;
> + } else if (!strcmp(*input, "reverse")) {
> + prio_queue_reverse(&pq);
> + } else {
> + int *v = xmalloc(sizeof(*v));
> + *v = atoi(*input);
> + prio_queue_put(&pq, v);
> + }
> + input++;
> + }
> +
> + clear_prio_queue(&pq);
> +
> + return 0;
> +}
> +
> +#define INPUT_SIZE 6
> +
> +#define BASIC_INPUT "1", "2", "3", "4", "5", "5", "dump"
> +#define BASIC_EXPECTED 1, 2, 3, 4, 5, 5
> +
> +#define MIXED_PUT_GET_INPUT "6", "2", "4", "get", "5", "3", "get", "get", "1", "dump"
> +#define MIXED_PUT_GET_EXPECTED 2, 3, 4, 1, 5, 6
> +
> +#define EMPTY_QUEUE_INPUT "1", "2", "get", "get", "get", "1", "2", "get", "get", "get"
> +#define EMPTY_QUEUE_EXPECTED 1, 2, -1, 1, 2, -1
> +
> +#define STACK_INPUT "stack", "1", "5", "4", "6", "2", "3", "dump"
> +#define STACK_EXPECTED 3, 2, 6, 4, 5, 1
> +
> +#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
> +#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
> +
> +#define TEST_INPUT(INPUT, EXPECTED, name) \
> + static void test_##name(void) \
> +{ \
> + const char *input[] = {INPUT}; \
> + int expected[] = {EXPECTED}; \
> + int result[INPUT_SIZE]; \
> + test_prio_queue(input, result); \
> + check(!memcmp(expected, result, sizeof(expected))); \
> +}
> +
> +TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
> +TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
> +TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
> +TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
> +TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + TEST(test_basic(), "prio-queue works for basic input");
> + TEST(test_mixed(), "prio-queue works for mixed put & get commands");
> + TEST(test_empty(), "prio-queue works when queue is empty");
> + TEST(test_stack(), "prio-queue works when used as a LIFO stack");
> + TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
> +
> + return test_done();
> +}
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
> --
> gitgitgadget |
|
User |
|
/submit |
|
Submitted as pull.1642.v3.git.1705502304219.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static int test_prio_queue(int *input, int *result)
> +{
> + struct prio_queue pq = { intcmp };
> + int i = 0;
> +
> + while (*input) {
> + int *val = input++;
> + void *peek, *get;
> + switch(*val) {
Ah, this one is a bit tricky. I would have expected that we can
make val a pure integer, but because we need to give the address of
the element to be placed in the queue, not the value to be placed as
an element in the queue, to prio_queue_put(), we would need the
pointer.
I wonder if writing this as a more conventional for(;;) loop would
make it easier to handle, i.e.
int val, i;
for (i = 0; (val = *input); input++) {
switch (val) {
case ...:
...
default:
prio_queue_put(&pq, input);
break;
}
}
> +...
> + default:
> + prio_queue_put(&pq, val);
> + break;
> + }
> + }
> + clear_prio_queue(&pq);
> + return 0;
> +}
> +#define TEST_INPUT(INPUT, EXPECTED, name) \
> + static void test_##name(void) \
> +{ \
> + int input[] = {INPUT}; \
I think I missed this myself in my review the last round, but we
need the sentinel value 0 at the end, i.e.,
int input[] = {INPUT, 0};
because the test_prio_queue() loops "while (*input)". Otherwise
the loop would not terminate.
> + int expected[] = {EXPECTED}; \
> + int result[ARRAY_SIZE(expected)]; \
These arrays are correct.
> + test_prio_queue(input, result); \
> + check(!memcmp(expected, result, sizeof(expected))); \
So is this check.
> +}
> +
> +TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
> +TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
> +TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
> +TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
> +TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + TEST(test_basic(), "prio-queue works for basic input");
> + TEST(test_mixed(), "prio-queue works for mixed put & get commands");
> + TEST(test_empty(), "prio-queue works when queue is empty");
> + TEST(test_stack(), "prio-queue works when used as a LIFO stack");
> + TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
> +
> + return test_done();
> +}
Other than that, looking good. Thanks. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): I forgot to examine the contents of the tests themselves.
> -cat >expect <<'EOF'
> -1
> -2
> -3
> -4
> -5
> -5
> -6
> -7
> -8
> -9
> -10
> -EOF
> -test_expect_success 'basic ordering' '
> - test-tool prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
> - test_cmp expect actual
> -'
This seems to have been lost from the converted test. Your basic
input test feeds an already sorted array of 6 items and dump to see
they are the same already sorted array, which is a lot less
interesting than the above.
> -cat >expect <<'EOF'
> -2
> -3
> -4
> -1
> -5
> -6
> -EOF
> -test_expect_success 'mixed put and get' '
> - test-tool prio-queue 6 2 4 get 5 3 get get 1 dump >actual &&
> - test_cmp expect actual
> -'
This is a faithful conversion.
> -cat >expect <<'EOF'
> -1
> -2
> -NULL
> -1
> -2
> -NULL
> -EOF
> -test_expect_success 'notice empty queue' '
> - test-tool prio-queue 1 2 get get get 1 2 get get get >actual &&
> - test_cmp expect actual
> -'
This too.
> -cat >expect <<'EOF'
> -3
> -2
> -6
> -4
> -5
> -1
> -8
> -EOF
> -test_expect_success 'stack order' '
> - test-tool prio-queue stack 8 1 5 4 6 2 3 dump >actual &&
> - test_cmp expect actual
> -'
This test got truncated in your version, which is not horribly
wrong, but if we claim "move t0009 to unit testing", people would
expect to see a conversion faithful to the original. And with the
use of result[ARRAY_SIZE(expected)], there is no reason to truncate
the original test with this version, no?
Thanks. |
|
This patch series was integrated into seen via git@bf95d09. |
|
This patch series was integrated into seen via git@f57f14d. |
|
There was a status update in the "Cooking" section about the branch Migrate priority queue test to unit testing framework. source: <pull.1642.v4.git.1705865326185.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@50b3a90. |
|
This patch series was integrated into seen via git@2473938. |
|
This patch series was integrated into seen via git@391381e. |
|
This patch series was integrated into seen via git@841c2fa. |
|
This patch series was integrated into seen via git@6e7e7ac. |
|
This patch series was integrated into next via git@38aa655. |
|
This patch series was integrated into seen via git@38aa655. |
|
This patch series was integrated into seen via git@d983427. |
|
There was a status update in the "Cooking" section about the branch Migrate priority queue test to unit testing framework. Will merge to 'master'. source: <pull.1642.v4.git.1705865326185.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@38aa655. |
|
This patch series was integrated into seen via git@4a01344. |
|
There was a status update in the "Cooking" section about the branch Migrate priority queue test to unit testing framework. Will merge to 'master'. source: <pull.1642.v4.git.1705865326185.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@37b0cc6. |
|
This patch series was integrated into seen via git@eaf9251. |
|
There was a status update in the "Cooking" section about the branch Migrate priority queue test to unit testing framework. Will merge to 'master'. source: <pull.1642.v4.git.1705865326185.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@75cab77. |
|
This patch series was integrated into seen via git@0ae17e5. |
|
This patch series was integrated into seen via git@aa5a01e. |
|
This patch series was integrated into seen via git@8c95728. |
|
This patch series was integrated into seen via git@107023e. |
|
This patch series was integrated into master via git@107023e. |
|
This patch series was integrated into next via git@107023e. |
|
Closed via 107023e. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks for the feedbacks! I have updated the patch so that the
resulting tests show something like:
in the case of a failing test.
cc: Chandra Pratap chandrapratap3519@gmail.com
cc: Jeff King peff@peff.net