add dlck_args_files tests

[osalyk]

Steps for the author:

• Commit message follows the guidelines.
• Appropriate Features or Test-tag pragmas were used.
• Appropriate Functional Test Stages were run.
• At least two positive code reviews including at least one code owner from each category referenced in the PR.
• Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

• Gatekeeper requested (daos-gatekeeper added as a reviewer).

---

This change is 

[github-actions]

Errors are component not formatted correctly,Ticket number prefix incorrect,PR title is malformatted. See https://daosio.atlassian.net/wiki/spaces/DC/pages/11133911069/Commit+Comments,Unable to load ticket data
https://daosio.atlassian.net/browse/add

[janekmi]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @osalyk)

---

a discussion (no related file):
A few missing testcases I would like to see:

• dlck_args_files_free():
  • maybe a testcase with an empty file list just in case
• args_files_parser():
  • ARGP_KEY_INIT -> sadly we can't wrap args_files_init() I see two options here:
    • either just check the effects or
    • move around the code to make is more testable.
  • ARGP_KEY_END. The same with this one. We can't mock args_files_check(). Give it a thought.
  • ARGP_KEY_SUCCESS
  • ARGP_KEY_FINI.
  • KEY_FILES and parse_file() fails.
  • arbitrary not supported key.

---

src/utils/dlck/tests/SConscript line 59 at r1 (raw file):

def build_dlck_args_files_ut(venv):
    """Build args_files.c test"""

There is no such file as args_files.c. 😉

Suggestion:

dlck_args_files.c

---

src/utils/dlck/tests/SConscript line 62 at r1 (raw file):

    venv.require('cmocka')
    libs = ['cmocka', 'gurt', 'daos_common_pmem', 'uuid']
    srcs = ['dlck_args_files_ut.c', '../dlck_args_files.c', '../dlck_args_parse.c']

I believe we should write a mock for the parse_file() function and test functions in the src/utils/dlck/dlck_args_parse.c file in a separate unit test. It will be a lot easier.

Suggestion:

srcs = ['dlck_args_files_ut.c', '../dlck_args_files.c']

---

src/utils/dlck/tests/dlck_args_files_ut.c line 27 at r1 (raw file):

	D_INIT_LIST_HEAD(&file->link);
	return file;
}

I believe this helper is unnecessary. When you need a struct dlck_file * structure allocated you can allocate on the spot. And I believe the contents of this structure is irrelevant. Let's see below.

struct dlck_file *file;
D_ALLOC_PTR(file);

---

src/utils/dlck/tests/dlck_args_files_ut.c line 33 at r1 (raw file):

setup(void **state)
{
	struct dlck_args_files *args = malloc(sizeof(*args));

I believe we tend not to use malloc()/calloc() directly. There are mocros for these purposes.

Suggestion:

	struct dlck_args_files *args;
	D_ALLOC_PTR(args);

---

src/utils/dlck/tests/dlck_args_files_ut.c line 45 at r1 (raw file):

	struct dlck_args_files *args = *state;
	dlck_args_files_free(args);
	free(args);

.

Suggestion:

D_FREE(args);

---

src/utils/dlck/tests/dlck_args_files_ut.c line 48 at r1 (raw file):

	return 0;
}

Nit-pick. I like to point out where tests start.

Code snippet:

/** tests */

---

src/utils/dlck/tests/dlck_args_files_ut.c line 50 at r1 (raw file):

static void
test_args_files_init_should_initialize_list(void **state)

I think we can drop all these args_files infixes.

Suggestion:

test_init_should_initialize_list

---

src/utils/dlck/tests/dlck_args_files_ut.c line 62 at r1 (raw file):

	struct argp_state       argp_state = {0};

	int                     rc = args_files_check(&argp_state, args);

argp_files_check() calls argp_failure(). I think we should test whether it is called and how it is called. Please see an example below.

That said, I read the argp.h header again I started to doubted whether we should call argp_failure() or argp_error(), thoughts?

Code snippet:

// venv.AppendUnique(LINKFLAGS=['-Wl,--wrap=argp_failure'])

void
__wrap_argp_failure(const struct argp_state *__restrict __state, int __status, int __errnum, const char *__restrict __fmt, ...)
{
	assert_ptr_equal(__state, MOCK_STATE);
	int errnum = mock_type(int);
	assert_int_equal(__status, errnum);
	assert_int_equal(__errnum, errnum);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 62 at r1 (raw file):

	struct argp_state       argp_state = {0};

	int                     rc = args_files_check(&argp_state, args);

Suggestion:

	// #define MOCK_STATE 0x30C4
	expect_value(__wrap_argp_failure, EINVAL);
	int rc = args_files_check(MOCK_STATE, args);

---

src/utils/dlck/tests/dlck_args_files_ut.c line 63 at r1 (raw file):

	int                     rc = args_files_check(&argp_state, args);
	assert_int_not_equal(rc, 0);

Suggestion:

assert_int_equal(rc, EINVAL);

---

src/utils/dlck/tests/dlck_args_files_ut.c line 79 at r1 (raw file):

	assert_int_equal(rc, 0);
	assert_false(d_list_empty(&args->list));
	;

🤡

---

src/utils/dlck/tests/dlck_args_files_ut.c line 83 at r1 (raw file):

	struct dlck_file *file = d_list_entry(args->list.next, struct dlck_file, link);
	assert_non_null(file);
	assert_memory_equal(file->po_uuid, expected, 16);

IMHO we should define a global Dlck_file which will be returned when the parse_file() mock succeeds. Here we should check whether this mocked global is in place.

struct dlck_file File;

int
parse_file(const char *arg, struct argp_state *state, struct dlck_file **file_ptr)
{
    /** ... */
    if (ret == 0) {
        *file_ptr = &File;
    }
    return ret;
}

Suggestion:

assert_ptr_equal(file, &File);

---

src/utils/dlck/tests/dlck_args_files_ut.c line 95 at r1 (raw file):

	dlck_args_files_free(args);
	assert_true(d_list_empty(&args->list));

1. I think UUIDs here are irrelevant.
2. I think we can leverage test_malloc()/test_free(). These are cmocka utilities which will track allocated blocks and make sure they are freed in the end.

// venv.AppendUnique(LINKFLAGS=['-Wl,--wrap=d_free'])
void
__wrap_d_free(void *ptr)
{
    test_free(ptr);
}

Ref: https://api.cmocka.org/group__cmocka__alloc.html

Suggestion:

	struct dlck_args_files *args = *state;
	struct dlck_file       *file = test_malloc(sizeof(*file));
	
	d_list_add_tail(&file->link, &args->list);

	dlck_args_files_free(args);
	assert_true(d_list_empty(&args->list));

---

src/utils/dlck/tests/dlck_args_files_ut.c line 110 at r1 (raw file):

	    cmocka_unit_test_setup_teardown(test_dlck_args_files_free_should_cleanup_list, setup,
					    teardown),
	};

1. I think it is more neat to keep this list outside of the main() function.
2. I think I prefer the initialization you already applied in dlck_d_vector_ut.c e.g.

    {"DVEC100: empty", empty_vector, setup, teardown},

Code quote:

	const struct CMUnitTest tests[] = {
	    cmocka_unit_test_setup_teardown(test_args_files_init_should_initialize_list, setup,
					    teardown),
	    cmocka_unit_test_setup_teardown(test_args_files_check_should_fail_if_no_files, setup,
					    teardown),
	    cmocka_unit_test_setup_teardown(test_args_files_parser_should_add_file_to_list, setup,
					    teardown),
	    cmocka_unit_test_setup_teardown(test_dlck_args_files_free_should_cleanup_list, setup,
					    teardown),
	};

---

src/utils/dlck/dlck_args_files.c line 19 at r1 (raw file):

#else
#define DLCK_STATIC static
#endif

It is already defined in src/include/daos_srv/dlck.h. I understand dlck.h is not included in src/utils/dlck/dlck_args_files.c and I would probably prefer not to.

How about proceed like this:

1. move this bit to a new file called src/include/daos_srv/dlck_debug.h,
2. include this in src/include/daos_srv/dlck.h and
3. include it everywhere the full blown dlck.h is unnecessary but having to common DLCK_UT_BUILD definition would be nice.

Code quote:

#ifdef DLCK_UT_BUILD
#define DLCK_STATIC
#else
#define DLCK_STATIC static
#endif

---

src/utils/dlck/dlck_args_files.c line 21 at r1 (raw file):

#endif

DLCK_STATIC struct argp_option args_files_options[] = {

I believe this is unnecessary.

Suggestion:

static struct argp_option args_files_options[] = {

---

src/utils/dlck/dlck_args_files.c line 43 at r1 (raw file):

}

DLCK_STATIC error_t

IMHO you should access this one through struct argp argp_file which is available already. With the added benefit of verifying what you test is actually in use. 😉

Suggestion:

static error_t

---

src/utils/dlck/dlck_args.h line 212 at r1 (raw file):

int
args_files_check(struct argp_state *state, struct dlck_args_files *args);
#endif

I don't like this bit. This is only for a single unit test's use and it clutters this header for everyone. I recommend moving the necessary forward declaration to the top of the unit test.

Code quote:

#ifdef DLCK_UT_BUILD
int
args_files_parser(int key, char *arg, struct argp_state *state);
void
args_files_init(struct dlck_args_files *args);
int
args_files_check(struct argp_state *state, struct dlck_args_files *args);
#endif

[osalyk]

Reviewable status: 0 of 7 files reviewed, 18 unresolved discussions (waiting on @janekmi)

---

src/utils/dlck/dlck_args.h line 212 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I don't like this bit. This is only for a single unit test's use and it clutters this header for everyone. I recommend moving the necessary forward declaration to the top of the unit test.

Done.

---

src/utils/dlck/dlck_args_files.c line 19 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is already defined in src/include/daos_srv/dlck.h. I understand dlck.h is not included in src/utils/dlck/dlck_args_files.c and I would probably prefer not to.

How about proceed like this:

1. move this bit to a new file called src/include/daos_srv/dlck_debug.h,
2. include this in src/include/daos_srv/dlck.h and
3. include it everywhere the full blown dlck.h is unnecessary but having to common DLCK_UT_BUILD definition would be nice.

Done.

---

src/utils/dlck/dlck_args_files.c line 21 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe this is unnecessary.

Done.

---

src/utils/dlck/dlck_args_files.c line 43 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO you should access this one through struct argp argp_file which is available already. With the added benefit of verifying what you test is actually in use. 😉

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 27 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe this helper is unnecessary. When you need a struct dlck_file * structure allocated you can allocate on the spot. And I believe the contents of this structure is irrelevant. Let's see below.

struct dlck_file *file;
D_ALLOC_PTR(file);

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 33 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe we tend not to use malloc()/calloc() directly. There are mocros for these purposes.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 45 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 50 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think we can drop all these args_files infixes.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 62 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

argp_files_check() calls argp_failure(). I think we should test whether it is called and how it is called. Please see an example below.

That said, I read the argp.h header again I started to doubted whether we should call argp_failure() or argp_error(), thoughts?

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 79 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

🤡

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 95 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. I think UUIDs here are irrelevant.
2. I think we can leverage test_malloc()/test_free(). These are cmocka utilities which will track allocated blocks and make sure they are freed in the end.

// venv.AppendUnique(LINKFLAGS=['-Wl,--wrap=d_free'])
void
__wrap_d_free(void *ptr)
{
    test_free(ptr);
}

Ref: https://api.cmocka.org/group__cmocka__alloc.html

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 110 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. I think it is more neat to keep this list outside of the main() function.
2. I think I prefer the initialization you already applied in dlck_d_vector_ut.c e.g.

    {"DVEC100: empty", empty_vector, setup, teardown},

Done.

---

src/utils/dlck/tests/SConscript line 59 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

There is no such file as args_files.c. 😉

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 62 at r1 (raw file):

	struct argp_state       argp_state = {0};

	int                     rc = args_files_check(&argp_state, args);

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 63 at r1 (raw file):

	int                     rc = args_files_check(&argp_state, args);
	assert_int_not_equal(rc, 0);

Done.

[janekmi]

@janekmi reviewed 6 of 7 files at r2, all commit messages.
Reviewable status: 6 of 7 files reviewed, 10 unresolved discussions (waiting on @osalyk)

---

src/include/daos_srv/dlck_debug.h line 16 at r2 (raw file):

#endif

#endif /** __DLCK_DEBUG__ */

I have no idea where this DAOS bit come from in the dlck.h header. xD
IMHO unnecessary.

Suggestion:

#ifndef __DLCK_DEBUG_H__
#define __DLCK_DEBUG_H__

#ifdef DLCK_UT_BUILD
#define DLCK_STATIC
#else
#define DLCK_STATIC static
#endif

#endif /** __DLCK_DEBUG_H__ */

---

src/utils/dlck/dlck_args.c line 33 at r2 (raw file):

/** glue everything together */

?

---

src/utils/dlck/dlck_args.h line 131 at r2 (raw file):

#define DLCK_PRINTF(ctrl, fmt, ...) (void)ctrl->print.dp_printf(fmt, __VA_ARGS__)

extern struct argp argp_file;

?

---

src/utils/dlck/tests/SConscript line 69 at r2 (raw file):

        '../dlck_args.c',
        '../dlck_args_common.c',
        '../dlck_args_engine.c'

I believe you do not need these four. But I guess I am little bit to early for the party. 😉

---

src/utils/dlck/tests/dlck_args_files_ut.c line 52 at r2 (raw file):

	check_expected(status);
	check_expected(errnum);
	check_expected(fmt);

😍

Code quote:

check_expected

---

src/utils/dlck/tests/dlck_args_files_ut.c line 70 at r2 (raw file):

	struct argp_state       argp_state = {0};

	expect_any(__wrap_argp_failure, state);

I guess you meant something else. 😉

IMHO existence of this variable is pointless. What you want to check is whether a pointer has been passed. In fact it can be any value you want. Ergo a mock. e.g. MOCK_ARGP_STATE. And since it will always be this value you can simplify the test. Just replace check_expected_ptr() with assert_ptr_equal(state, MOCK_ARGP_STATE).

Suggestion:

argp_state

---

src/utils/dlck/tests/dlck_args_files_ut.c line 73 at r2 (raw file):

	expect_value(__wrap_argp_failure, status, EINVAL);
	expect_value(__wrap_argp_failure, errnum, EINVAL);
	expect_string(__wrap_argp_failure, fmt, "No file chosen");

I am not sure about this one. It makes effectively a copy of all error messages we expect. I guess we have to brainstorm this one offline.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 122 at r2 (raw file):

main(void)
{
	const char *test_name = "args_files_tests";

Suggestion:

dlck_args_files.c tests

[janekmi]

@janekmi reviewed 1 of 7 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @osalyk)

[osalyk]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @janekmi)

---

src/include/daos_srv/dlck_debug.h line 16 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I have no idea where this DAOS bit come from in the dlck.h header. xD
IMHO unnecessary.

Done.

---

src/utils/dlck/dlck_args.h line 131 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Done.

---

src/utils/dlck/dlck_args.c line 33 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 83 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO we should define a global Dlck_file which will be returned when the parse_file() mock succeeds. Here we should check whether this mocked global is in place.

struct dlck_file File;

int
parse_file(const char *arg, struct argp_state *state, struct dlck_file **file_ptr)
{
    /** ... */
    if (ret == 0) {
        *file_ptr = &File;
    }
    return ret;
}

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 70 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I guess you meant something else. 😉

IMHO existence of this variable is pointless. What you want to check is whether a pointer has been passed. In fact it can be any value you want. Ergo a mock. e.g. MOCK_ARGP_STATE. And since it will always be this value you can simplify the test. Just replace check_expected_ptr() with assert_ptr_equal(state, MOCK_ARGP_STATE).

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 73 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I am not sure about this one. It makes effectively a copy of all error messages we expect. I guess we have to brainstorm this one offline.

Let's just check if fmt is empty, shall we?

---

src/utils/dlck/tests/SConscript line 69 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe you do not need these four. But I guess I am little bit to early for the party. 😉

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 122 at r2 (raw file):

main(void)
{
	const char *test_name = "args_files_tests";

Done.

[osalyk]

Reviewable status: 2 of 7 files reviewed, 10 unresolved discussions (waiting on @janekmi)

---

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

A few missing testcases I would like to see:

• dlck_args_files_free():
  • maybe a testcase with an empty file list just in case
• args_files_parser():
  • ARGP_KEY_INIT -> sadly we can't wrap args_files_init() I see two options here:
    • either just check the effects or
    • move around the code to make is more testable.
  • ARGP_KEY_END. The same with this one. We can't mock args_files_check(). Give it a thought.
  • ARGP_KEY_SUCCESS
  • ARGP_KEY_FINI.
  • KEY_FILES and parse_file() fails.
  • arbitrary not supported key.

Done.

[janekmi]

@janekmi reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @osalyk)

---

a discussion (no related file):

Previously, osalyk (Oksana Sałyk) wrote…

Done.

Please revisit this list with two hints in mind:

1. You have to call argp_file.parser() directly.
2. You cannot call argp_parse().

---

src/utils/dlck/tests/SConscript line 61 at r3 (raw file):

    """Build dlck_args_files.c test"""
    venv.require('cmocka')
    venv.AppendUnique(LINKFLAGS=['-Wl,--wrap=argp_failure', '-Wl,--wrap=parse_file'])

I believe you do not need a wrap for this. You can just provide an alternative implementation.

Suggestion:

venv.AppendUnique(LINKFLAGS=['-Wl,--wrap=argp_failure'])

---

src/utils/dlck/tests/SConscript line 61 at r3 (raw file):

    """Build dlck_args_files.c test"""
    venv.require('cmocka')
    venv.AppendUnique(LINKFLAGS=['-Wl,--wrap=argp_failure', '-Wl,--wrap=parse_file'])

I would like to test replacing these two with cmocka's test_calloc() and test_free() so cmocka will track for us whether all allocated bits are actually fried. It is essential for tests like test_files_free_empty_list() to work.

Code snippet:

venv.AppendUnique(LINKFLAGS=['-Wl,--wrap=d_calloc'])
venv.AppendUnique(LINKFLAGS=['-Wl,--wrap=d_free'])

---

src/utils/dlck/tests/dlck_args_files_ut.c line 21 at r3 (raw file):

args_files_init(struct dlck_args_files *args);
int
args_files_check(struct argp_state *state, struct dlck_args_files *args);

I think it fits here better.

Suggestion:

void
args_files_init(struct dlck_args_files *args);
int
args_files_check(struct argp_state *state, struct dlck_args_files *args);
extern struct argp argp_file;

---

src/utils/dlck/tests/dlck_args_files_ut.c line 25 at r3 (raw file):

/** mocks */

#define MOCK_ARGP_STATE ((void *)0xDEADBEEF)

💀 🐮 so typical. :D

---

src/utils/dlck/tests/dlck_args_files_ut.c line 25 at r3 (raw file):

/** mocks */

#define MOCK_ARGP_STATE ((void *)0xDEADBEEF)

I just realized it would be necessary for some tests. Sorry.

Suggestion:

struct argp_state Argp_state;
#define MOCK_ARGP_STATE (&Argp_state)

---

src/utils/dlck/tests/dlck_args_files_ut.c line 27 at r3 (raw file):

#define MOCK_ARGP_STATE ((void *)0xDEADBEEF)
struct dlck_file   File;
extern struct argp argp_file;

Please see the comment above.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 28 at r3 (raw file):

struct dlck_file   File;
extern struct argp argp_file;

Two mocks I would like to check usefulness of for our testing purposes. Please see a comment in SConscript for more details.

Code snippet:

void *
__wrap_d_calloc(size_t count, size_t eltsize)
{
        return test_calloc(count, eltsize);
}

void
__wrap_d_free(void *ptr)
{
        test_free(ptr);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 31 at r3 (raw file):

void
__wrap_argp_failure(struct argp_state *state, int status, int errnum, const char *fmt, ...)
{

Missing check.

Code snippet:

assert_ptr_equal(state, MOCK_ARGP_STATE);

---

src/utils/dlck/tests/dlck_args_files_ut.c line 38 at r3 (raw file):

int
__wrap_parse_file(const char *arg, struct argp_state *state, struct dlck_file **file_ptr)

Please try without it.

Suggestion:

parse_file

---

src/utils/dlck/tests/dlck_args_files_ut.c line 43 at r3 (raw file):

	uuid_parse("12345678-1234-1234-1234-123456789abc", File.po_uuid);
	*file_ptr = &File;
	return (int)mock();

We do not test file parsing not try to mimic it. Just basic argument testing and return value control.

Suggestion:

	check_ptr_equal(arg, MOCK_ARG);
	assert_ptr_equal(state, MOCK_ARGP_STATE);
	assert_non_null(file_ptr);
	int rc = mock_type(int);
	if (rc == DER_SUCCESS) {
		memset(&File, 0, sizeof(File));
		*file_ptr = &File;
    }
	return rc;

---

src/utils/dlck/tests/dlck_args_files_ut.c line 92 at r3 (raw file):

	dlck_args_files_free(args);
	D_FREE(args);
}

1. I have no idea how cmocka manages to run this test when its type is not correct. So, please fix it.
2. I realized this test is basically an empty one assuming you will call setup() and teardown().
3. Maybe this one should be the first one assuming we agree it is effectively an empty one.

Suggestion:

static int
test_files_free_empty_list(void **unused)
{
    /** the work is done by setup and teardown functions */
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 99 at r3 (raw file):

	struct dlck_args_files *args       = *state;
	struct argp_state      *argp_state = MOCK_ARGP_STATE;
	assert_ptr_equal(argp_state, MOCK_ARGP_STATE);

💀 🐮 🟰 💀 🐮 🟰 💀 🐮

I am not sure what you did expect. 🤣
Unnecessary.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 125 at r3 (raw file):

	/** cleanup */
	D_FREE(args);

1. We are not testing argp per se. 🙃 So, no argp_parse() calls. Use the argp_file.parser() directly.
2. No argp means no argv is necessary.
3. IMHO clean-up is not necessary either. Since no allocation took place. Just linking.
4. We need another test where will_return(parse_file, EINVAL);.

Suggestion:

	struct dlck_args_files *args = *state;

	memset(&Argp_state, 0 sizeof(Argp_state));
	argp_state.input = args;

	will_return(parse_file, 0);

	int rc = argp_file.parser(KEY_FILES, MOCK_ARG, &argp_state);
	assert_int_equal(rc, 0);

	assert_false(d_list_empty(&args->list));
	struct dlck_file *file = d_list_entry(args->list.next, struct dlck_file, link);
	assert_ptr_equal(file, &File);

---

src/utils/dlck/tests/dlck_args_files_ut.c line 139 at r3 (raw file):

	dlck_args_files_free(args);
	assert_true(d_list_empty(&args->list));

1. It is already done in the teardown. Let's keep it symmetrical.
2. It is a test for *_free() exactly the same as test_files_free_empty_list(). Let's keep it next to each other.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 203 at r3 (raw file):

	assert_int_equal(rc, EINVAL);
	assert_true(d_list_empty(&args.list));
}

It is a test for parse_file(). It would be covered by a different unit test.

[osalyk]

Reviewable status: 5 of 7 files reviewed, 15 unresolved discussions (waiting on @janekmi)

---

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Please revisit this list with two hints in mind:

1. You have to call argp_file.parser() directly.
2. You cannot call argp_parse().

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 21 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think it fits here better.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 25 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I just realized it would be necessary for some tests. Sorry.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 27 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please see the comment above.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 28 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Two mocks I would like to check usefulness of for our testing purposes. Please see a comment in SConscript for more details.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 31 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Missing check.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 38 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please try without it.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 43 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We do not test file parsing not try to mimic it. Just basic argument testing and return value control.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 92 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. I have no idea how cmocka manages to run this test when its type is not correct. So, please fix it.
2. I realized this test is basically an empty one assuming you will call setup() and teardown().
3. Maybe this one should be the first one assuming we agree it is effectively an empty one.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 99 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

💀 🐮 🟰 💀 🐮 🟰 💀 🐮

I am not sure what you did expect. 🤣
Unnecessary.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 125 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. We are not testing argp per se. 🙃 So, no argp_parse() calls. Use the argp_file.parser() directly.
2. No argp means no argv is necessary.
3. IMHO clean-up is not necessary either. Since no allocation took place. Just linking.
4. We need another test where will_return(parse_file, EINVAL);.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 139 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. It is already done in the teardown. Let's keep it symmetrical.
2. It is a test for *_free() exactly the same as test_files_free_empty_list(). Let's keep it next to each other.

Without this call (dlck_args_files_free()), the file object will remain in memory, leading to a leak.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 203 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is a test for parse_file(). It would be covered by a different unit test.

Done.

---

src/utils/dlck/tests/SConscript line 61 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe you do not need a wrap for this. You can just provide an alternative implementation.

Done.

---

src/utils/dlck/tests/SConscript line 61 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I would like to test replacing these two with cmocka's test_calloc() and test_free() so cmocka will track for us whether all allocated bits are actually fried. It is essential for tests like test_files_free_empty_list() to work.

Done.

[janekmi]

@janekmi reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @osalyk)

---

a discussion (no related file):

Previously, osalyk (Oksana Sałyk) wrote…

Done.

ARGP_KEY_* tests are still missing. These are the one which change the state of the parser.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 30 at r4 (raw file):

struct dlck_file File;
static char      mock_arg_str[] = "mock_arg_value";
#define MOCK_ARG mock_arg_str

💀 🐮 returns. 😄

Suggestion:

#define MOCK_ARG ((void *)0xDEADBEEF)

---

src/utils/dlck/tests/dlck_args_files_ut.c line 30 at r4 (raw file):

struct dlck_file File;
static char      mock_arg_str[] = "mock_arg_value";
#define MOCK_ARG mock_arg_str

I think this will solve all our problems. 😁 Please see more below.

Code snippet:

struct dlck_args_files Args;

---

src/utils/dlck/tests/dlck_args_files_ut.c line 78 at r4 (raw file):

	args_files_init(args);
	*state = args;
	return 0;

I think this is what we need for setup. Since it does not allocate nothing new no teardown is necessary.

Suggestion:

	struct dlck_args_files args;
	args_files_init(&args);
	Argp_state.input = &args;

---

src/utils/dlck/tests/dlck_args_files_ut.c line 91 at r4 (raw file):

	}
	return 0;
}

I think it is unnecessary in this case. Setup does not allocate nothing. If test needs to allocate a struct dlck_file * it has to also clean up afterwards by calling dlck_args_files_free().

---

src/utils/dlck/tests/dlck_args_files_ut.c line 107 at r4 (raw file):

	assert_true(d_list_empty(&args->list));
}

These two are basically the same. Let's fuse them together.

Suggestion:

static void
test_files_free_empty_list(void **unused)
{
	assert_true(d_list_empty(&Args.list));
	dlck_args_files_free(&Args);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 119 at r4 (raw file):

	int rc = args_files_check(MOCK_ARGP_STATE, args);
	assert_int_equal(rc, EINVAL);
}

This is should a test look like when args are not provided via state but is global.

• state -> unused.

Suggestion:

static void
test_check_should_fail_if_no_files(void **unused)
{
	expect_value(__wrap_argp_failure, status, EINVAL);
	expect_value(__wrap_argp_failure, errnum, EINVAL);

	int rc = args_files_check(MOCK_ARGP_STATE, &Args);
	assert_int_equal(rc, EINVAL);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 149 at r4 (raw file):

	struct dlck_file *file = d_list_entry(args->list.next, struct dlck_file, link);
	assert_ptr_equal(file, &File);
}

• linking Argp_state.input = &Args is already done in setup().

Suggestion:

static void
test_parser_should_add_file_to_list(void **state)
{
	will_return(parse_file, 0);

	int rc = argp_file.parser(KEY_FILES, MOCK_ARG, &Argp_state);
	assert_int_equal(rc, 0);

	assert_false(d_list_empty(&Args.list));

	struct dlck_file *file = d_list_entry(Args.list.next, struct dlck_file, link);
	assert_ptr_equal(file, &File);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 190 at r4 (raw file):

	assert_int_equal(rc, EINVAL);
	assert_true(d_list_empty(&args.list));
}

This is exactly the same as test_key_files_parse_file_fails().

---

src/utils/dlck/tests/dlck_args_files_ut.c line 204 at r4 (raw file):

	assert_int_equal(rc, 0);
	assert_false(d_list_empty(&args.list));
}

This is exactly the same as test_parser_should_add_file_to_list().

---

src/utils/dlck/tests/dlck_args_files_ut.c line 241 at r4 (raw file):

    {"DARG109: parser fail", test_key_files_parse_file_fails, setup, teardown},
    {"DARG110: unknown key", test_parser_unknown_key_should_return_zero, setup, teardown},
};

1. No teardown().
2. Please keep tests in order e.g.:

• free
• check
• parser

3. Please also reorder the respective test functions in the same order.

Suggestion:

static const struct CMUnitTest tests_all[] = {
    {"DARG100: free empty", test_files_free_empty_list, setup, NULL},
    {"DARG101: free list", test_free_should_cleanup_list, setup, NULL},
    {"DARG102: free list multiple", test_free_should_cleanup_multiple_files, setup, NULL},
    {"DARG103: check fail", test_check_should_fail_if_no_files, setup, NULL},
    {"DARG104: check success", test_check_should_succeed_with_file, setup, NULL},
    {"DARG105: parser add", test_parser_should_add_file_to_list, setup, NULL},
    {"DARG106: parser fail", test_key_files_parse_file_fails, NULL, NULL},
    {"DARG117: unknown key", test_parser_unknown_key_should_return_zero, setup, NULL},
};

[osalyk]

Reviewable status: 6 of 7 files reviewed, 10 unresolved discussions (waiting on @janekmi)

---

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

ARGP_KEY_* tests are still missing. These are the one which change the state of the parser.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 30 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

💀 🐮 returns. 😄

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 30 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think this will solve all our problems. 😁 Please see more below.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 78 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think this is what we need for setup. Since it does not allocate nothing new no teardown is necessary.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 91 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think it is unnecessary in this case. Setup does not allocate nothing. If test needs to allocate a struct dlck_file * it has to also clean up afterwards by calling dlck_args_files_free().

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 107 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

These two are basically the same. Let's fuse them together.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 119 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This is should a test look like when args are not provided via state but is global.

• state -> unused.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 149 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

• linking Argp_state.input = &Args is already done in setup().

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 190 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This is exactly the same as test_key_files_parse_file_fails().

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 204 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This is exactly the same as test_parser_should_add_file_to_list().

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 241 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. No teardown().
2. Please keep tests in order e.g.:

• free
• check
• parser

3. Please also reorder the respective test functions in the same order.

Done.

[janekmi]

@janekmi reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @osalyk)

---

a discussion (no related file):
I realized in my comments I did not applied consistently in descriptions and function names either conditions or results.
e.g. often "no files" means "fail". Whereas success is not called "with files".

I am not sure what I prefer. What would be better for someone who would read these in the future? 👽
I think the rule should be:

1. If there is only one possible way of achieving the result (either success of failure). Call it simply either success or failure.
2. If there is more than one way of achieving the result:

• Make sure the tested condition is in the name and description.
• The result may be there as well.

In the end, I will leave it to you to decide. 😉

---

src/utils/dlck/tests/dlck_args_files_ut.c line 24 at r5 (raw file):

extern struct argp argp_file;

/** mocks */

Suggestion:

/** mocks & globals */

---

src/utils/dlck/tests/dlck_args_files_ut.c line 48 at r5 (raw file):

	assert_non_null(file_ptr);
	int rc = mock_type(int);
	if (rc == DER_SUCCESS) {

parse_file() does not use DER_* error codes. We ought to use 0 here.

Suggestion:

if (rc == 0) {

---

src/utils/dlck/tests/dlck_args_files_ut.c line 68 at r5 (raw file):

/* Setup for dlck_args_files */
static int

1. Of course, it is for dlck_args_files. No need to point it out.
2. A newline makes it a "section" instead of just a function comment.
3. We can also explicitly state we did not forgot about the teardown.

Suggestion:

/** setup (no teardown necessary) */

static int

---

src/utils/dlck/tests/dlck_args_files_ut.c line 75 at r5 (raw file):

	*state           = &Args;
	return 0;
}

They are globals. No need to pass them around.

Suggestion:

setup(void **unused)
{
	args_files_init(&Args);
	Argp_state.input = &Args;
	return 0;
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 80 at r5 (raw file):

static void
test_check_should_fail_if_no_files(void **unused)

Suggestion:

test_check_no_files_fail

---

src/utils/dlck/tests/dlck_args_files_ut.c line 98 at r5 (raw file):

	int rc = args_files_check(MOCK_ARGP_STATE, args);
	assert_int_equal(rc, 0);
}

1. Function name.
2. unused argument.

Suggestion:

static void
test_check_success(void **unused)
{
	d_list_add_tail(&File.link, &Args.list);

	int rc = args_files_check(MOCK_ARGP_STATE, &Args);
	assert_int_equal(rc, 0);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 101 at r5 (raw file):

static void
test_parser_key_init(void **state)

Suggestion:

test_parser_KEY_INIT(void **unused)

---

src/utils/dlck/tests/dlck_args_files_ut.c line 118 at r5 (raw file):

	D_FREE(dummy);
}

I think we can simplify this one a lot. Sorry to break this intricate construction.

Suggestion:

{
	memset(&Args, 0xf, sizeof(Args));

	int rc = argp_file.parser(ARGP_KEY_INIT, NULL, &Argp_state);
	assert_int_equal(rc, 0);

	assert_true(d_list_empty(&Args.list));
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 121 at r5 (raw file):

static void
test_parser_end_without_files_triggers_failure(void **unused)

Suggestion:

test_parser_KEY_END_without_files_fails

---

src/utils/dlck/tests/dlck_args_files_ut.c line 132 at r5 (raw file):

static void
test_parser_end_with_files_returns_zero(void **state)

Suggestion:

test_parser_KEY_END_with_files_success

---

src/utils/dlck/tests/dlck_args_files_ut.c line 146 at r5 (raw file):

	D_FREE(f);
}

A simplified version. We can just use globals. This is what are they for, aren't they? 😉

Suggestion:

static void
test_parser_end_with_files_returns_zero(void **unused)
{
	d_list_add_tail(&File.link, &Args.list);
	int rc = argp_file.parser(ARGP_KEY_END, NULL, &Argp_state);
	assert_int_equal(rc, 0);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 175 at r5 (raw file):

	D_FREE(d1);
	D_FREE(d2);
}

1. I very very much like this test. ❤️
2. Again, we should use globals.
3. Let's make two tests instead of one. 😉

Suggestion:

static void
helper_KEY_is_noop(int key)
{
	d_list_add_tail(&File.link, &Args.list);
	int rc = argp_file.parser(key, NULL, &Argp_state);
	assert_int_equal(rc, 0);
	assert_false(d_list_empty(&Args.list));	
}

static void
test_parser_KEY_SUCCESS_is_noop(void **unused)
{
	helper_KEY_is_noop(ARGP_KEY_SUCCESS);
}

static void
test_parser_KEY_FINI_is_noop(void **unused)
{
	helper_KEY_is_noop(ARGP_KEY_FINI);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 178 at r5 (raw file):

static void
test_parser_should_add_file_to_list(void **state)

Suggestion:

test_KEY_FILE_success(void **unused)

---

src/utils/dlck/tests/dlck_args_files_ut.c line 192 at r5 (raw file):

static void
test_key_files_parse_file_fails(void **state)

Suggestion:

test_KEY_FILE_fail(void **unused)

---

src/utils/dlck/tests/dlck_args_files_ut.c line 202 at r5 (raw file):

	assert_int_equal(rc, EINVAL);
	assert_true(d_list_empty(&args.list));
}

Globals. Globals. Globals.

Suggestion:

{
	will_return(parse_file, EINVAL);

	int rc = argp_file.parser(KEY_FILES, MOCK_ARG, &Argp_state);
	assert_int_equal(rc, EINVAL);
	assert_true(d_list_empty(&Args.list));
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 205 at r5 (raw file):

static void
test_parser_unknown_key_should_return_zero(void **state)

Suggestion:

test_KEY_UKNOWN_fail(void **unused)

---

src/utils/dlck/tests/dlck_args_files_ut.c line 212 at r5 (raw file):

	int rc = argp_file.parser(9999, NULL, &Argp_state);
	assert_int_equal(rc, ARGP_ERR_UNKNOWN);
}

#define MOCK_KEY_UKNOWN 9999

Suggestion:

{
	int rc = argp_file.parser(MOCK_KEY_UKNOWN, NULL, &Argp_state);
	assert_int_equal(rc, ARGP_ERR_UNKNOWN);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 219 at r5 (raw file):

	assert_true(d_list_empty(&Args.list));
	dlck_args_files_free(&Args);
}

Just as a sanity check.

Suggestion:

{
	assert_true(d_list_empty(&Args.list));
	dlck_args_files_free(&Args);
	assert_true(d_list_empty(&Args.list));
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 247 at r5 (raw file):

	dlck_args_files_free(args);
	assert_true(d_list_empty(&args->list));
}

1. Reuse.
2. Globals.

Suggestion:

static void
helper_files_free(int file_num)
{
	for (int i = 0; i < file_num; i++) {
		struct dlck_file *file;
		D_ALLOC_PTR(file);
		d_list_add_tail(&file->link, &Args.list);
	}

	dlck_args_files_free(&Args);
	assert_true(d_list_empty(&Args.list));
}

static void
test_files_free_one_file(void **unused)
{
	helper_files_free(1);
}

static void
test_files_free_three_files(void **unused)
{
	helper_files_free(3);
}

---

src/utils/dlck/tests/dlck_args_files_ut.c line 250 at r5 (raw file):

static const struct CMUnitTest tests_all[] = {
    {"DARG100: check fail", test_check_should_fail_if_no_files, setup, NULL},

Please apply to all. I think this identifier has to be as unique as possible to when a certain test fails in CI it will be easy to grep the actual test in the code.

Suggestion:

ARGFILES100

---

src/utils/dlck/tests/dlck_args_files_ut.c line 250 at r5 (raw file):

static const struct CMUnitTest tests_all[] = {
    {"DARG100: check fail", test_check_should_fail_if_no_files, setup, NULL},

I would say what is important is the condition. No that it should fail.

Suggestion:

check no files

---

src/utils/dlck/tests/dlck_args_files_ut.c line 261 at r5 (raw file):

    {"DARG109: free empty", test_files_free_empty_list, setup, NULL},
    {"DARG110: free list", test_free_should_cleanup_list, setup, NULL},
    {"DARG111: free list multiple", test_free_should_cleanup_multiple_files, setup, NULL},

Regarding just test descriptions.

Suggestion:

    {"DARG100: args_files_check() no files", test_check_should_fail_if_no_files, setup, NULL},
    {"DARG101: args_files_check() success", test_check_should_succeed_with_file, setup, NULL},
    {"DARG102: parser ARGP_KEY_INIT", test_parser_key_init, setup, NULL},
    {"DARG103: parser ARGP_KEY_END no files", test_parser_end_without_files_triggers_failure, setup, NULL},
    {"DARG104: parser ARGP_KEY_END success", test_parser_end_with_files_returns_zero, setup, NULL},
    {"DARG105: parser ARGP_KEY_SUCCESS noop", test_parser_success_and_fini_are_noop, setup, NULL},
    {"DARG105: parser ARGP_KEY_FINI noop", test_parser_success_and_fini_are_noop, setup, NULL},
    {"DARG106: KEY_FILES success", test_parser_should_add_file_to_list, setup, NULL},
    {"DARG107: KEY_FILES fail", test_key_files_parse_file_fails, setup, NULL},
    {"DARG108: unknown key", test_parser_unknown_key_should_return_zero, setup, NULL},
    {"DARG109: dlck_args_files_free() no files", test_files_free_empty_list, setup, NULL},
    {"DARG110: dlck_args_files_free() one file", test_free_should_cleanup_list, setup, NULL},
    {"DARG111: dlck_args_files_free() multiple files", test_free_should_cleanup_multiple_files, setup, NULL},

[osalyk]

Reviewable status: 5 of 7 files reviewed, 22 unresolved discussions (waiting on @janekmi)

---

src/utils/dlck/tests/dlck_args_files_ut.c line 48 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

parse_file() does not use DER_* error codes. We ought to use 0 here.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 68 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. Of course, it is for dlck_args_files. No need to point it out.
2. A newline makes it a "section" instead of just a function comment.
3. We can also explicitly state we did not forgot about the teardown.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 75 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

They are globals. No need to pass them around.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 98 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. Function name.
2. unused argument.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 118 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think we can simplify this one a lot. Sorry to break this intricate construction.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 146 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

A simplified version. We can just use globals. This is what are they for, aren't they? 😉

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 175 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. I very very much like this test. ❤️
2. Again, we should use globals.
3. Let's make two tests instead of one. 😉

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 202 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Globals. Globals. Globals.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 212 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

#define MOCK_KEY_UKNOWN 9999

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 219 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Just as a sanity check.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 247 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1. Reuse.
2. Globals.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 250 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please apply to all. I think this identifier has to be as unique as possible to when a certain test fails in CI it will be easy to grep the actual test in the code.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 250 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I would say what is important is the condition. No that it should fail.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 261 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Regarding just test descriptions.

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 24 at r5 (raw file):

extern struct argp argp_file;

/** mocks */

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 80 at r5 (raw file):

static void
test_check_should_fail_if_no_files(void **unused)

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 101 at r5 (raw file):

static void
test_parser_key_init(void **state)

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 121 at r5 (raw file):

static void
test_parser_end_without_files_triggers_failure(void **unused)

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 132 at r5 (raw file):

static void
test_parser_end_with_files_returns_zero(void **state)

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 178 at r5 (raw file):

static void
test_parser_should_add_file_to_list(void **state)

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 192 at r5 (raw file):

static void
test_key_files_parse_file_fails(void **state)

Done.

---

src/utils/dlck/tests/dlck_args_files_ut.c line 205 at r5 (raw file):

static void
test_parser_unknown_key_should_return_zero(void **state)

Done.

[janekmi]

@janekmi reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @osalyk)