Skip to content

t/unit-tests: convert mem-pool to use clar test framework #1873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Seyi007
Copy link
Contributor

@Seyi007 Seyi007 commented Jan 13, 2025

Adapt the mem-pool test functions to clar framework by using clar assertions where necessary. Following the consensus to convert the unit-tests scripts found in the t/unit-tests folder to clar driven by Patrick Steinhardt.

Mentored-by: Patrick Steinhardt ps@pks.im
Signed-off-by: Seyi Kuforiji kuforiji98@gmail.com

@@ -14,6 +14,7 @@ do
suite_name=$(basename "$suite")
suite_name=${suite_name%.c}
suite_name=${suite_name#u-}
suite_name=$(echo "$suite_name" | sed 's/-/_/g')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine as-is, but I think it's more customary in our codebase to use tr(1) instead. E.g. echo "$suite_name" | tr '-' '_').

@@ -14,6 +14,7 @@ do
suite_name=$(basename "$suite")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit should be ordered before the first one as it is a mandatory prerequisite. The Git project aims for "bisectability", which means that every single commit in your patch series should always compile and test alright. With the current ordering that is not the case.

#include "unit-test.h"
#include "mem-pool.h"

static void t_calloc_100(struct mem_pool *pool)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the testcases do anything with their local pool. So how about we make this take as parameter the block_alloc size and have the function here handle the setup and teardown of the pool?

Adapt script to translate dashes (`-`) in test suite filenames to
underscores (`_`) to ensure proper extraction of test function names.
`generate-clar-decls.sh` does not pick up dashes in filenames such as
`u-mem-pool.c`, which prevents the scripts from being run. This will be
used by subsequent commits which follows the same construct.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
@Seyi007 Seyi007 force-pushed the convert-2-clar-phase2 branch from d31535b to 6cdf8a8 Compare January 14, 2025 13:26
Adapt the mem-pool test script to use clar framework by using clar
assertions where necessary. Following the consensus to convert the
unit-tests scripts found in the t/unit-tests folder to clar driven by
Patrick Steinhardt. Test functions are created as a standalone to test
different test cases.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
@Seyi007 Seyi007 force-pushed the convert-2-clar-phase2 branch 2 times, most recently from ca6348f to ae4d0f4 Compare January 15, 2025 09:31
while ((peek = prio_queue_peek(&pq))) {
get = prio_queue_get(&pq);
cl_assert(peek == get);
cl_assert((size_t)j < result_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the cast you're talking about? If so, you can avoid the cast by converting the variable declaration of j to be a size_t directly. It's the right thing to do anyway, I'd say.

Convert the prio-queue test script to clar framework by using clar
assertions where necessary. Following the consensus to convert the
unit-tests scripts found in the t/unit-tests folder to clar driven by
Patrick Steinhardt. Test functions are created as a standalone to test
different cases.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
@Seyi007 Seyi007 force-pushed the convert-2-clar-phase2 branch 2 times, most recently from 51274ad to 7f9a133 Compare January 16, 2025 08:52
{
TEST(t_tree_search(), "tree_search works");
TEST(t_infix_walk(), "infix_walk works");
t_tree_search();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit curious. I would've expected you to inline t_tree_search() here.

return test_done();
void test_reftable_tree__infix_walk(void)
{
t_infix_walk();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I think this should be inlined.

Adapts reftable tree test script to clar framework by using clar
assertions where necessary. Following the consensus to convert the
unit-tests scripts found in the t/unit-tests folder to clar driven by
Patrick Steinhardt.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
@Seyi007 Seyi007 force-pushed the convert-2-clar-phase2 branch from 7f9a133 to 905ab91 Compare January 16, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants