Name of user not set #1002873 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/40538/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40538/1//COMMIT_MSG@20 PS1, Line 20: make unit-tests
After cherry-picking the latest patchset, I am able to build and run the unit tests.
Great, thanks!
https://review.coreboot.org/c/coreboot/+/40538/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40538/2//COMMIT_MSG@9 PS2, Line 9: This commit's purpose is to show basic example of how unit testing may : be applied for coreboot project. It adds test harness for lib/string.c : module.
Show a basic example of how unit testing can be applied for the coreboot project. […]
Done
https://review.coreboot.org/c/coreboot/+/40538/1/tests/include/mocks/assert.... File tests/include/mocks/assert.h:
https://review.coreboot.org/c/coreboot/+/40538/1/tests/include/mocks/assert.... PS1, Line 64: * called.
Sorry, I just wrote these comments in the order I read your code. […]
Done
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/Makefile.inc File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/Makefile.inc@21 PS2, Line 21: string-test-stage:= ramstage
Actually, I'd rather you leave this out so that the reference to others is that you don't need to pr […]
OK.
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@25 PS2, Line 25: /* Since strdup() requires malloc() it may only be invoked during ramstage : * phase. Expect dead_code/assert invocation otherwise. : */
Since unit tests are intended to run on the host, is this comment relevant?
This is actually the excerpt from uut - src/lib/string.c. This is relevant in the sense that I wanted to show example of Cmocka assertion handling (dropped this idea already, see other comments). Actually we have idea of stages now, so this comment is a leftover and I should remove it.
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@34 PS2, Line 34: assert_int_equal(0, memcmp(str, duplicate, strlen(str)));
Cmocka seems to have an assert_string_equal().
Yes, but the trick is that assert_string_equal() is using.. strcmp, which in case of our binary will invoke coreboot implementation. We will actually test unit with unit under test.
At the same time, I'm using strlen() here, but this function is already tested with test_strlen_strings() at this time. One may say that it breaks the idea of test isolation - what do you think? I may change this to constant value, to be on the safe side.
I will add a comment at the top of this file, since this is rather rare case when you shouldn't use functions used "everyday".