Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39894 )
Change subject: tests: Add build subsystem and example for unit tests ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc@37 PS1, Line 37: TEST_LDFLAGS = -lcmocka
Yes, I need to do this. […]
Some syntactic sugar to handle this would definitely be nice... whether we keep it in the source file or the Makefile (the latter is probably easier). Having a separate file just for that seems a bit overkill. (We generally use that concept of doing '<sourcefile>-<attribute> += <value>' in the subdirectory Makefile.inc to declare special build metadata related to a file a lot in coreboot, so I think it would make sense to use it for the tests as well.)
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc@50 PS1, Line 50: $$(addprefix $$(SRC_DIR),$$(subst -test,.c,$$*))
If this constraint cannot be met, it means that code is not well designed (or cannot be tested). […]
Yes, I agree on using this to declare the mock functions too, that would probably work well.
I think for the most part we can probably stick to the one test file per source file approach, that's definitely a sane default. But keeping the flexibility open to occasionally have multiple test files for the same source file sounds like a good idea. (So for my example Makefile.inc that probably means test/lib/fmap-test.c and src/lib/fmap.c should be added explicitly to the list of test sources. Which also means we probably need something like
tests += fmap-test ...
at the top of each Makefile.inc.)
https://review.coreboot.org/c/coreboot/+/39894/1/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/39894/1/tests/lib/string-test.c@21 PS1, Line 21: /* Below will fail the test */ : size = strlen(NULL);
The only purpose was to have test failing, showing that Cmocka can catch such a failure. […]
I think that's probably okay. It's good that it catches segfaults cleanly, but we probably don't need to test that a certain function causes a null pointer access in certain cases... if we actually expected those cases to happen we'd probably change the function to have a more defined result instead.
A related thing that *is* important, I think, is to make sure that expect_assert_failure() works right for things like assert(), BUG(), die() and halt(). That should probably be easy enough to do once we figure out that header mocking.