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); } }