Hello all,
More unit testing excitement! I think there are some valuable changes
we could make to the `test/testutil` API, so I wanted to share some
ideas with the community. I introduced many of the issues that I want
to focus on, so I don't really feel obligated to be polite here :). I
just wanted to give that disclaimer in case it feels like I'm being
overly harsh.
I already touched on one of testutil's problems in an earlier email: a
lack of support for standalone self tests. Recall that a self test is
something that runs in the simulator when you type `newt test
<test-name>`. In my earlier email, I expressed that these tests should
automatically execute `sysinit()` before running.
Now I want to discuss a second problem that I see with this library: an
abundance of configurable callbacks that makes the API too complicated.
I think we can remove about half of the callbacks without losing any
functionality. The execution flow of a test suite is described in a
comment in `testutil.h`:
TEST_SUITE
tu_suite_init -> ts_suite_init_cb
tu_suite_pre_test -> ts_case_pre_test_cb
tu_case_init -> tc_case_init_cb
tu_case_pre_test -> tc_case_pre_test_cb
TEST_CASE
tu_case_post_test -> tc_case_post_test_cb
tu_case_pass/tu_case_fail -> ts_case_{pass,fail}_cb
tu_case_complete
tu_suite_post_test -> ts_case_post_test_cb
tu_suite_complete -> ts_suite_complete_cb
That is a lot of callbacks for a suite with one test case! Let's go
through them one by one.
tu_suite_init:
Description: Gets called once per suite, before the suite runs.
Proposal: Remove.
Rationale: If a suite needs to do any setup, it can do so directly
at the top of its block.
tu_suite_pre_test:
Description: Gets called at the start of each case in the suite.
Proposal: Keep.
Rationale: This callback is the very reason why we group test cases
into suites. Similar test cases need to start from the
same state.
tu_case_init:
Description: Same as tu_suite_pre_test.
Proposal: Remove.
Rationale: Redundant.
tu_case_pre_test:
Description: Gets called once at the start of the next test case.
Proposal: Remove.
Rationale: A test case is just a block of C code. If it needs to
perform some setup, it can do it directly at the start of
the test.
tu_case_post_test:
Description: Gets called at the end of the current / next test case.
Proposal: Keep.
Rationale: A test case may terminate prematurely due to a failed
`TEST_ASSERT_FATAL()`. We need a callback to perform
clean up because the test case may not reach the end of
its block.
tu_case_pass / tu_case_fail:
Description: One of these gets called whenever a test case
completes.
Proposal: Keep.
Rationale: We need some way to report results.
tu_suite_post_test:
Description: Gets called at the end of each case in the suite.
Proposal: Remove.
Rationale: Test cases can already clean up individually using
`tu_case_post_test`. I can see how common clean up code
might be useful, but I don't think this is needed often
enough to justify the added API complexity. For
reference, this function is only used in one place
(NFFS), and it is used unnecessarily - the calls can be
removed with no effect on the tests.
tu_suite_complete:
Description: Gets called at the end of each test suite.
Proposal: Remove.
Rationale: If a test suite needs to perform any cleanup (unlikely),
it can do so directly at the end of its code block.
Unlike a test case, a test suite never terminates early,
so the code at the end of a suite always gets executed.
If we make the proposed removals, the execution flow looks like this:
TEST_SUITE
TEST_CASE
tu_suite_pre_test_cb
<test-body>
tu_case_pass/tu_case_fail
tu_case_post_test_cb
I think that looks better!
I also wanted to talk about how I think these callbacks should get
configured:
tu_case_pass / tu_case_fail:
Configured at the start of the application.
tu_suite_pre_test_cb:
Always configured at the top of a test suite body.
tu_case_post_test_cb:
Always configured at the top of a test case body.
Finally, I've added an example test suite at the bottom of this
email. This is a totally ridiculous set of tests that verify the
machine's (or compiler's) ability to do arithmetic. Ridiculous, but it
is just intended to demonstrate the proposed API changes.
I plan to submit a PR with the proposed changes soon. So feel free to
comment here, or wait for the PR. In any case, all comments are
welcome.
Thanks,
Chris
------
TEST_SUITE(arithmetic_suite)
{
/* Configure a "setup" function that runs at the start of every
* case in this suite.
*/
tu_suite_set_pre_test_cb(arithmetic_pre_test, NULL);
/* Run each test case in sequence. */
test_case_add();
test_case_div();
/* If suite needs to clean up (unlikely), it does it here
* directly.
*/
}
TEST_CASE_SELF(test_case_add)
{
/* Configure a cleanup function for this test case. */
tu_case_set_post_test_cb(adder_uninit, NULL);
/* Initialize the "adder". */
adder_init();
/* Verify adder behavior. */
TEST_ASSERT(0 + 0 == 0);
TEST_ASSERT(100 + 20 == 120);
TEST_ASSERT(UINT_MAX + 1 == 0);
}
/**
* Helper function for division test. Performs a single integer
* division and checks the result.
*/
static void
test_div_int(int num, int den, int expected)
{
int answer;
TEST_ASSERT_FATAL(den != 0, "bug in test: 0 denominator");
answer = num / den;
TEST_ASSERT(answer == expected,
"%d / %d, have=%d want=%d",
num, den, answer, expected);
}
TEST_CASE_SELF(test_case_div)
{
static const struct {
int num;
int den;
int expected;
} table[] = {
{ 0, 100, 0 },
{ 1, 2, 0 },
{ 53, 8, 6 },
};
int i;
for (i = 0; i < sizeof table / sizeof table[0]; i++) {
test_div_int(table[i].num, table[i].den, table[i].expected);
}
}
