1
\$\begingroup\$

I have written a template for a C program implementing a simple test framework.

If the program is run with the first parameter "test", all subsequent parameters will be interpreted as a sequence of test cases (without the test_ prefix) to be executed. If no other parameters are specified after the first parameter "test", all functions named test_* will be executed in the sequence specified in the TEST_CASES macro.

If one of the test cases fails, the program will report this and stop. In no test case fails, All test cases completed successfully will be logged.

Normally I would split the program into a header, general function definitions and test case files, but for SE, I thought it would be easier post the amalgamated code.

What do you think?

/*template.c - program template with a basic test framework*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

/*To add a new test case for function foo(), add a line "X(foo) \" to the 
  TEST_CASES macro and then define the concrete test case function as -
  result_t *test_foo(result_t *result){
// Code to test foo() goes here. If the test is successful, set 
// result->rc = SUCCESS. Messages should be added to result->string using 
// the cat_message function
}
*/
#define TEST_CASES \
  X(init_string) \
X(cat_message) \
X(parse_tc) \
X(get_all_tc)

/*Return codes*/
typedef enum {
  SUCCESS,
  ERR_MALLOC,
  ERR_REALLOC,
  ERR_GENERAL
} rc_t;

/*String object : array of char with allocated size*/
typedef struct {
  size_t size;
  char *str;
} string_t;

/*Test results object : string object with return code*/
typedef struct {
  string_t *string;
  rc_t rc;
} result_t;

/*tc_func_t : test case function signature */
typedef result_t *(*tc_func_t)(result_t *result);

/*Test case execution sequence*/
typedef struct {
  int num_tc; /* number in test cases in the tc_funcs array */
  tc_func_t *tc_funcs; /*pointer to an array of test case functions*/
} tc_seq_t;

/*report_func_t : report results function signature*/
typedef rc_t (*report_func_t)(result_t *result);

/*Function prototypes*/
/*Initialise string object */
string_t *init_string(size_t size);
/*Concatenate a message to the string object*/
string_t *cat_message(const char *message, string_t *string);
/*Count number of test cases in TEST_CASES*/
int count_tc(void);
/*Create a test case set from the command line parameters*/
tc_seq_t *parse_tc(int argc, char **argv);
/*Create a test case set with all test cases specified in the TEST_CASES macro*/
tc_seq_t *get_all_tc(void);
/*Run all test cases in the test case sequence and report results*/
result_t *run_tests(tc_seq_t *tc_seq, report_func_t report_func, 
    result_t *result);
/*Write results to STDOUT*/
rc_t print_stdout(result_t *result);
/*Test case prototypes are generated by the X macro expansion*/
#define X(name) result_t *test_ ## name(result_t *result);
TEST_CASES
#undef X

string_t *init_string(size_t size){
  string_t *string = malloc(sizeof *string);
  /*If an error occurs, return a NULL pointer*/
  if (string == NULL) return NULL;
  string->str = malloc(size);
  if (string->str == NULL){
    free(string);
    return NULL;
  }
  string->size = size;
  *string->str = '\0';
  return string;
}

string_t *cat_message(const char *message, string_t *string){
  /*If the caller does supply the correct parameters, return the string object 
   * unchanged*/
  if (message == NULL || string == NULL || string->str == NULL) return string;
  while (strlen(message) >= string->size - strlen(string->str)){
    /*If the memory allocated to the string object is insufficient, keep 
     * doubling the allocation until it is enough to concancenate the message*/
    string->str = realloc(string->str, string->size * 2);
    if (string->str == NULL) return string; 
    string->size *= 2;
  }
  memcpy(string->str+strlen(string->str), message, strlen(message)+1);
  return string;
}

int count_tc(void){
  int num_tc = 0;
#define X(name) num_tc++;
  TEST_CASES
#undef X
    return num_tc;
}

tc_seq_t *parse_tc(int argc, char **argv){
  /*If the first parameter is not "test", return a NULL pointer*/
  if (argc == 1 || strcmp(argv[1], "test")) return NULL;
  /*if the first and only parameter is "test", return all test cases */
  if (argc == 2) return get_all_tc();
  /*return test cases specified in the parameters after "test"*/
  tc_seq_t *tc_set = malloc(sizeof *tc_set);
  tc_set->tc_funcs = malloc((argc - 2) * sizeof *tc_set->tc_funcs);
  /*If an error occurs, return a NULL pointer*/
  if (tc_set->tc_funcs == NULL) {
    free(tc_set);
    return NULL;
  }
  int tc_ind = 0;
  /*Fill the array testcase->tc_funcs[] with pointers to test functions*/
  for (int i = 2; i < argc; i++){
#define X(name) if (!strcmp(argv[i], #name)) \
    {tc_set->tc_funcs[tc_ind] = test_ ## name; tc_ind++; break;}
    TEST_CASES
#undef X
  }
  tc_set->num_tc = argc - 2;
  return tc_set;
}

tc_seq_t *get_all_tc(void){
  tc_seq_t *tc_set = malloc(sizeof *tc_set);
  if (tc_set == NULL) return NULL;
  tc_set->tc_funcs = malloc(count_tc() * sizeof *tc_set->tc_funcs);
  /*If an error occurs, return a NULL pointer*/
  if (tc_set->tc_funcs == NULL) {
    free(tc_set);
    return NULL;
  }
  tc_set->num_tc = count_tc();
  /*Fill the array testcase->tc_funcs[] with pointers to test functions*/
  int tc = -1;
#define X(name) tc++; tc_set->tc_funcs[tc] = test_ ## name;
  TEST_CASES
#undef X
    return tc_set;
}

result_t *run_tests(tc_seq_t *tc_seq, report_func_t report_func, 
    result_t *result){
  for (int tc_ind = 0; tc_ind < tc_seq->num_tc; tc_ind++) {
    result = tc_seq->tc_funcs[tc_ind](result);
    if (result->rc != SUCCESS) break;
  }
  if (result->rc == SUCCESS) cat_message(
      "All test cases completed successfully\n", result->string);
  free(tc_seq->tc_funcs);
  free(tc_seq);
  report_func(result);
  return result;
}

rc_t print_stdout(result_t *result){
  if (result == NULL) return ERR_GENERAL;
  if (result->string == NULL) return ERR_GENERAL;
  if (result->string->str == NULL) return ERR_GENERAL;
  printf("%s", result->string->str);
  printf("Return code: %d\n", result->rc);
  return SUCCESS;
}

int main(int argc, char **argv){
  string_t *string = init_string(1024);
  if (string == NULL) return ERR_MALLOC;
  string = cat_message("Start log\n", string);
  if (string == NULL) return ERR_REALLOC;
  result_t *result = malloc(sizeof *result);
  if (result == NULL) { 
    free(string->str);
    free(string);
    return ERR_GENERAL;
  } 
  result->string = string;
  result->rc = SUCCESS;
  tc_seq_t *tc_seq = parse_tc(argc, argv);
  if (tc_seq != NULL) result = run_tests(tc_seq, print_stdout, result);
  free(string->str);
  free(string);
  free(result);
}

result_t *test_init_string(result_t *result){
  string_t *string = init_string(10);
  if (string == NULL){
    result->rc = ERR_MALLOC;
    result->string = cat_message("init_string failed\n", result->string);
    return result;
  }
  if (string->size == 10 && !strcmp(string->str, "")){
    result->rc = SUCCESS;
  } else {
    result->rc = ERR_GENERAL;
    result->string = cat_message("init_string failed\n", result->string);
  }
  free(string->str);
  free(string);
  return result;
}

result_t *test_cat_message(result_t *result){
  string_t *string = init_string(10);
  if (string == NULL){
    result->rc = ERR_MALLOC;
    return result;
  }
  string = cat_message("foo", string);
  if (string == NULL){
    result->rc = ERR_REALLOC;
    result->string = cat_message("cat_message failed\n", result->string);
    return result;
  }
  string = cat_message("bar", string);
  if (string == NULL){
    result->string = cat_message("cat_message failed\n", result->string);
    result->rc = ERR_REALLOC;
    return result;
  }
  if (string->size == 10 && !strcmp(string->str, "foobar")){
    result->rc = SUCCESS;
  } else {
    result->string = cat_message("cat_message failed\n", result->string);
    result->rc = ERR_GENERAL;
  }
  free(string->str);
  free(string);
  return result;
}

result_t *test_parse_tc(result_t *result){
  char *argv[] = {"template", "test", "parce_tc", "get_all_tc"};
  tc_seq_t *tc = parse_tc(4, argv);
  if (tc == NULL){
    result->string = cat_message("parse_tc failed\n", result->string);
    result->rc = ERR_GENERAL;
    return result;
  }
  if (tc->num_tc == 2) {
    result->rc = SUCCESS;
  } else {
    result->rc = ERR_GENERAL;
    result->string = cat_message("parse_tc failed\n", result->string);
  }
  free(tc->tc_funcs);
  free(tc);
  return result;
}

result_t *test_get_all_tc(result_t *result){
  tc_seq_t *tc = get_all_tc();
  if (tc == NULL){
    result->rc = ERR_GENERAL;
    result->string = cat_message("get_all_tc failed\n", result->string);
    return result;
  }
  if (tc->num_tc == count_tc()) {
    result->rc = SUCCESS;
  } else {
    result->rc = ERR_GENERAL;
    result->string = cat_message("get_all_tc failed\n", result->string);
  }
  free(tc->tc_funcs);
  free(tc);
  return result;
}
\$\endgroup\$
  • \$\begingroup\$ Something I often do when posting as a single file is to write an empty comment line <!-- --> at each file break. This makes it easy to copy+paste into a file, but still leaves it clear where the boundaries are. \$\endgroup\$ – Toby Speight Feb 13 '18 at 9:37
1
\$\begingroup\$

What do you think?

Well laid out consistently, but a bit tight.
Good basic error handling.
Good uses of sizeof *obj in allocation calculations.
Good used of (void) with such function declarations.
Compiled well without warnings.


Interface unclear

Certainly the comment was meant to be "... caller does not supply ..."

When cat_message() returns, how to determine it it succeeded or failed given:
"If the caller does supply the correct parameters, return the string object * unchanged"?
Recommended to return an error flag, value or bool.

Name space

Functions names init_string(), cat_message(), count_tc() and others share little to hint that they work together. Recommend a common prefix.

Doubling unclear.

In cat_message(), why repetitively call realloc() if code can determine the minimal needs? Just allocate once: maximum of 2x former size and size needed.

Inefficientcy

In cat_message(), code re-calculates strlen(string->str) and strlen(message) incurring a run down each string. Just save the lengths and re-use. Specifically the strlen(string->str) is an issue as code may have called realloc() and code cannot assume the 2nd call to strlen(string->str) returns the same length.


Minor

0 corner

init_string(0) leads to UB due to *string->str = '\0';. Suggest code gracefully handle this corner. Maybe return NULL.

unsigned math wrap-around

Watch out for underflow of unsigned math with size_t. Both below should work for OP, yet the 2nd form is more resilient to unexpected string->size, string->str pairs.

// while (strlen(message) >= string->size - strlen(string->str))
while (strlen(message) + strlen(string->str) >= string->size )

Assumption

Code assumes argc == 0 never occurs and so argv[1] is not guarded.

// if (argc == 1 || strcmp(argv[1], "test")) return NULL;
if (argc <= 1 || strcmp(argv[1], "test")) return NULL;

Later.

Information hiding

Code does not detail what might be the *.h part and implementation part. To this end:

typedef struct {
  size_t size;
  char *str;
} string_t;

and the other 3 typedefs only need to declare the pointer type with typedef struct string_t *string_t; and then declare the functions. The definition of the type can occurs later and not in the public *.h section.

How many rc_t?

Often code needs to assess the range of an enumerated type. Useful to add a _N as the last one.

/*Return codes*/
typedef enum {
  SUCCESS,
  ERR_MALLOC,
  ERR_REALLOC,
  ERR_GENERAL,
  ERR_N    // add
} rc_t;

Non-unique error messages

cat_message("cat_message failed\n", result->string); used 3 times. Maybe cat_message("cat_message failed 1\n", result->string);, etc. to help distinguish reported error.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ thanks @chux 1. yes, I missed the word "not" from the comment at the start of cat_message 2. I agree a shared name prefix would be useful for these functions. 3. In hindsight, I see the repetitive realloc is inefficient - I should realloc at least strlen(string->str) + strlen(message) 4. I assumed the compiler would optimise away the repeated strlen(x) calls but I agree, they should not there in the first place. 5. init_string(0) does not make sense, I will check size > 0. 6. You say watch out for underflow, how could this happen with my while(...) expression? \$\endgroup\$ – user2309803 Feb 13 '18 at 21:50
  • \$\begingroup\$ 7. I thought argv[0] was always the name of the executable so isn't argc always >= 1? 8. I was going put all code above string_t *init_string(size_t size){ in a header file since these declarations are used by the internal interfaces of the program, why is this not advisable? 8. I always put ERR_GENERAL in the last position of the enum declaration. 9. I agree error messages should be unique. \$\endgroup\$ – user2309803 Feb 13 '18 at 21:56
  • \$\begingroup\$ @user2309803 3: Off by 1, realloc at least strlen(string->str) + strlen(message) + 1 4 Compiler cannot optimize away 2nd strlen(string->str) as string->str may have changed. 6. Unexpected string->size, string->str pairs could result from broken code. \$\endgroup\$ – chux - Reinstate Monica Feb 13 '18 at 22:15
  • \$\begingroup\$ @user2309803 7 argc always >= 1 no. It may be 0. C spec: "The value of argc shall be nonnegative." 8. Not advisable as it is not needed for public to see it and potential mess with it (see # 6) or write code that depends on it. 8. I was speaking of enum declarations in general. \$\endgroup\$ – chux - Reinstate Monica Feb 13 '18 at 22:16
  • \$\begingroup\$ you could add a subsection "Potential execution of NULL function pointers", To avoid undefined behaviour, there should be a check that tc_funcs[tc_ind] and report_func are not NULL pointers before execution, agree? \$\endgroup\$ – user2309803 Feb 14 '18 at 14:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.