Unit test of argument parser

Question

I'm new to testing and just finished writing (what I hope to be) tests for an existing argument parser in C. Please have a look.

First the Unit Under Test:

args.h

#ifndef ARGS_H
#define ARGS_H
#include <argp.h>
struct args
{
    int count, silent, verbose;
    char *output_file;
};
error_t parse_opt(int, char*, struct argp_state*);
int parse_args(int, char**);
#endif /* ARGS_H */

args.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <argp.h>
#include <errno.h>
#include <libeternity/args.h>
#include <config.h>

/* argp requires these to be global */
const char *argp_program_version = PACKAGE_VERSION;
const char *argp_program_bug_address = PACKAGE_BUGREPORT;

error_t parse_opt(int key, char *arg, struct argp_state *state)
{
    struct args *args = state->input;

    switch (key)
    {
        case 'q': case 's':
            args->silent = 1;
            break;

        case 'v':
            args->verbose = 1;
            break;

        case 'o':
            args->output_file = arg;
            break;

        case ARGP_KEY_ARG:
            if (args->count-- < 0)
                break;
            break;

        case ARGP_KEY_END:
            if (state->arg_num < 1)
            {
                argp_usage(state);
                return 0;
            }
            break;

        default:
            return ARGP_ERR_UNKNOWN;
            break;
    }
    return 0;
}

int parse_args(int argc, char **argv)
{
    int rv;
    const char* doc = "Eternity";
    char args_doc[] = "file1.jpg file2.png file3.bmp ...";

    struct argp_option options[] = {
        {"verbose",'v', 0, 0, "Produce verbose output", 0},
        {"quiet",  'q', 0, 0, "Don't produce any output", 0},
        {"silent", 's', 0, OPTION_ALIAS, NULL, 0},
        {"output", 'o', "FILE", 0, "Output to FILE instead of std output", 0},
        {0}
    };

    struct argp argp = {options, parse_opt, args_doc, doc, NULL, 0, NULL};
    struct args args;
    args.count = argc;
    args.silent = args.verbose = 0;
    args.output_file = "-";

    rv = argp_parse(&argp, argc, argv, ARGP_NO_EXIT, NULL, &args);

    if (rv > 0)
    {
        errno = rv;
        perror("argp_parse");
        return -1;
    }

    if (rv < 0)
        return -1;

    return 0;
}

And the test itself (compiled with -Wl,--wrap=argp_parse,--wrap=argp_usage for mocking purposes):

test_args.c

#include <stdarg.h>
#include <stddef.h>
#include <setjmp.h>
#include <cmocka.h>
#include <argp.h>
#include <libeternity/args.h>

int __wrap_argp_parse(const struct argp *argp,
        int argc, char **argv, unsigned flags,
        int *arg_index, void *input)
{
    check_expected(argc);
    check_expected(argv);
    return mock_type(int);
}

void __wrap_argp_usage(struct argp_state *state)
{
    assert_non_null(state);
}

void parseArgs_argpSucceeds_return0(void **state)
{
    int rv, argc = 2;
    char *argv[argc];
    argv[0] = "eternity";
    argv[1] = "--help";

    expect_value(__wrap_argp_parse, argc, 2);
    expect_memory(__wrap_argp_parse, argv, &argv, sizeof(argv));
    will_return(__wrap_argp_parse, 0);

    rv = parse_args(argc, argv);

    assert_int_equal(rv, 0);
}

void parseArgs_argpFails_returnNeg1(void **state)
{
    int rv, argc = 2;
    char *argv[argc];
    argv[0] = "eternity";
    argv[1] = "--help";

    expect_value(__wrap_argp_parse, argc, 2);
    expect_memory(__wrap_argp_parse, argv, &argv, sizeof(argv));
    will_return(__wrap_argp_parse, -1);

    rv = parse_args(argc, argv);

    assert_int_equal(rv, -1);
}

void parseOpt_qEncountered_setSilentFlag(void **state)
{
    int rv;
    struct argp_state argp_state;
    struct args args;
    args.silent = 0;
    struct args *argsp = &args;
    argp_state.input = (void *) argsp;

    rv = parse_opt((int) 'q', 0, &argp_state);
    argsp = (struct args*) argp_state.input;

    assert_int_equal(argsp->silent, 1);
    assert_int_equal(rv, 0);
}

void parseOpt_sEncountered_setSilentFlag(void **state)
{
    int rv;
    struct argp_state argp_state;
    struct args args;
    args.silent = 0;
    struct args *argsp = &args;
    argp_state.input = (void *) argsp;

    rv = parse_opt((int) 's', 0, &argp_state);
    argsp = (struct args*) argp_state.input;

    assert_int_equal(argsp->silent, 1);
    assert_int_equal(rv, 0);
}

void parseOpt_vEncountered_setVerboseFlag(void **state)
{
    int rv;
    struct argp_state argp_state;
    struct args args;
    args.verbose = 0;
    struct args *argsp = &args;
    argp_state.input = (void *) argsp;

    rv = parse_opt((int) 'v', 0, &argp_state);
    argsp = (struct args*) argp_state.input;

    assert_int_equal(argsp->verbose, 1);
    assert_int_equal(rv, 0);
}

void parseOpt_oEncountered_setOutputFile(void **state)
{
    int rv;
    struct argp_state argp_state;
    struct args args;
    args.output_file = NULL;
    struct args *argsp = &args;
    argp_state.input = (void *) argsp;

    rv = parse_opt((int) 'o', "/tmp/libeternity.out", &argp_state);
    argsp = (struct args*) argp_state.input;

    assert_non_null(argsp->output_file);
    assert_int_equal(rv, 0);
}

void parseOpt_keyArgEncountered_decrementArgCount(void **state)
{
    int rv;
    struct argp_state argp_state;
    struct args args;
    args.count = 4;
    struct args *argsp = &args;
    argp_state.input = (void *) argsp;

    rv = parse_opt((int) ARGP_KEY_ARG, 0, &argp_state);
    argsp = (struct args*) argp_state.input;

    assert_int_equal(argsp->count, 3);
    assert_int_equal(rv, 0);
}

int main(void)
{
    const struct CMUnitTest tests[] =
    {
        cmocka_unit_test(parseArgs_argpSucceeds_return0),
        cmocka_unit_test(parseArgs_argpFails_returnNeg1),
        cmocka_unit_test(parseOpt_qEncountered_setSilentFlag),
        cmocka_unit_test(parseOpt_sEncountered_setSilentFlag),
        cmocka_unit_test(parseOpt_vEncountered_setVerboseFlag),
        cmocka_unit_test(parseOpt_oEncountered_setOutputFile),
        cmocka_unit_test(parseOpt_keyArgEncountered_decrementArgCount),
    };

    return cmocka_run_group_tests(tests, NULL, NULL);
}

Am I approaching this right? How is my code coverage, and how can it be improved?

Adriano Repetti · Accepted Answer · 2017-08-21 09:00:24Z

In args structure you're using int to store boolean flags. C supports boolean values from C99 (and I'd suggest to put one field on each line):

struct args
{
    int count;
    _Bool silent;
    _Bool verbose;
    char *output_file;
};

It's not about efficiency but about clarity. Now include <stdbool.h> and replace _Bool with bool and you will also get true and false constants.

I see that you have two fields for the same thing: silent and verbose. Does it make any sense to have them both set to true? If not then you should change it to an enum (I'm simplifying little bit assuming that in your case verbose and quiet are specular):

enum verbosity
{
    quiet,
    normal,
    verbose
};

In this way you can later support the fancy -qq (or --really-quiet) and -vv variants and your code will be as simple as:

if (opts->verbosity >= verbose) { }

In parse_opt you have:

if (args->count-- < 0)
    break;
break;

Which may simply be:

--args->count;
break;

And:

return ARGP_ERR_UNKNOWN;
break;

Which may be:

return ARGP_ERR_UNKNOWN;

I think you're testing the wrong function. parse_args() should be (IMO) the only exported function with this prototype:

error_t parse_args(int, char**, args*);

And parse_opt() should be an implementation detail private to args.c.

You initialize args.output_file to "-" but in parseOpt_oEncountered_setOutputFile() you just assert it's not null. It works because you're not calling parse_args() which is, IMO, what you should really test. Why? Because that's the interface and parse_opt() is just an implementation detail. I suppose you do not want to write the same tests twice (once to check parse_opts() works as expected and once to check parse_args() works as expected), currently you do not check parse_args() in depth.

Writing tests parse_args() if you forget to assign a value for output_file in parse_opts() then it will go unnoticed in your tests. Here there is also another problem: you assign a literal string to that field but there is no clue for its users that the value cannot be changed when it's not null. It's not something that may happen for "-" but it's an unwanted vulnerability. Fortunately it's pretty easy to solve both problems in out shot changing initialization value:

args.silent = args.verbose = false;
args.output_file = NULL;

It has also the advantage to use NULL as magic value for the undefined case (otherwise you have to strcmp() with "-" (which should be then #defineed somewhere).

Toby Speight · Accepted Answer · 2017-08-21 09:15:37Z

Reduce duplication

If you're doing TDD, you'll know the mantra Red, Green, Refactor. The refactor (or reduce duplication) step applies to the tests as much as it does to the production code.

What can we do to reduce the amount of duplication within the tests? If we're able to change the signature of parse_args to accept an array of pointers to constant strings, then we might be able to use literals in the tests, like this:

const char *const argv[] = { "eternity", "--help", NULL };
int argc = sizeof argv / sizeof argv[0] - 1;

I haven't used cmocka myself, so won't be more specific, but just take the general advice - look for opportunities to make your tests simpler.

Some of the boiler-plate can be compacted a little, by initialising variables, reducing the nead for temporaries, and using implicit conversion between void* and other pointers. For example:

void parseOpt_sEncountered_setSilentFlag(void **)
{
    struct argp_state argp_state;
    struct args args;
    argp_state.input = &args;
    args.silent = 0;

    assert_int_equal(0, parse_opt('s', 0, &argp_state));

    struct args *argsp = argp_state.input;
    assert_int_equal(argsp->silent, 1);
}

Test for combinations of options

Is -q cumulative? What about -v? And what does it mean to have both -q and -v? They might cancel each other out, or (as in svn log) may control different aspects of the program.

Test the non-effects, too

Consider parseOpt_sEncountered_setSilentFlag:

assert_int_equal(argsp->silent, 1);
assert_int_equal(rv, 0);

We check that the silent value is set, but not whether (e.g.) verbose has been unintentionally changed. We could make the test more thorough by checking that all of argsp matches our expectations:

struct args expected = args;
expected->silent = 1;
assert_true(expected == args);

(You might want to write your own assertion function for comparing struct args that does a memberwise comparison and lets you know which member fails. You'll want to test the function itself, of course!)

Stack Exchange Network

Unit test of argument parser

args.h

args.c

test_args.c

2 Answers 2

Reduce duplication

Test for combinations of options

Test the non-effects, too

You must log in to answer this question.

Hot Network Questions

Unit test of argument parser

args.h

args.c

test_args.c

2 Answers 2

Reduce duplication

Test for combinations of options

Test the non-effects, too

You must log in to answer this question.

Related

Hot Network Questions