Name of user not set #1002873 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:
(4 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
You should definitely add at least one mock function to the example code.
Yes, I need to do this.
Regarding linker flags - I wonder it will be better to keep list of mocks in a Makefile or to have some metadata file (probably per-test), or even header at the top of test code .c module, which will list all functions which should be mocked. This will be parsed inside Makefile and proper flags should be added then.
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). Of course, we can adjust tests so that they will pass, but in my opinion this should go another way around - code should be modified to be testable and to pass all the tests.
Well, I would be careful with this mindset. We have to remember that we are dealing with firmware code and not any odd userspace tool. In userspace it may often be acceptable to design for testability first and performance/size/whatever later, and it's no big deal to add another function call boundary here and there or artificially overcomplicate an interface to make it easier to mock. But for firmware code, restrictions may be more serious. I think we all want to add tests, but we do not want to make coreboot any worse for it.
If there's really no way to test something without changes to coreboot we can evaluate whether those changes are acceptable, but needing to link more than one file doesn't really sound complicated enough to warrant that. It just sounds like a pretty arbitrary restriction to me.
I see your point. Definitely, we cannot change the coreboot, to be worse because of tests. My opinion was rather to _not_ add tests (which are not unit test) in such case. In the same time, let's prepare a flexible framework and then we can decide on case by case basis. After all we need something practical, so that people will use it.
Can you think of and point me to the example, where we would need to incorporate multiple files in the unit test binary? Sounds rather like an integration testing, not unit testing.
Well... hmmm... I guess I'm mostly worried about common helper stuff that's used everywhere. Say you want to test fmap_locate_area() in src/lib/fmap.c. A reasonable unit test would be that you have a hardcoded FMAP blob in the test and you try to check that you can find all areas in there. That basically only involves functions from that file, so it should work... except that it's also using the rdev API (src/commonlib/region.c) which is a an API used all over coreboot to interface with data sources (like SPI flash in actual firmware, or a mock buffer in memory in this case). The rdev API already supports in-memory buffers with mem_region_device so it would be very easy to just use that to mock out boot_device_ro() in my test and return a device representing my mocked FMAP buffer. But in order to do that, we have to be able to link src/commonlib/region.c somehow. (Of course there'd be another unit test covering the rdev API itself, so that when we use it to test fmap.c we can already be sure it is internally correct.)
Thanks for the example. This definitely doesn't follow a strict definition of a unit test, since you are testing the interface between rdev and FMAP. But as above - let's focus on unit tests being practical. And used by devs.
When talking about splitting a function in two - does this happen frequently for functions available externally?
Yes, I think changing underlying interfaces while keeping the API the same is a common thing. We refactor and modernize stuff in coreboot all the time. I'm trying to come up with a good example... hmm... let's say the difference between these two patch sets that I recently reviewed: https://review.coreboot.org/c/coreboot/+/39002/6..7/src/lib/edid_fill_fb.c . Patrick originally just wrote this with a implicit linked list hardcoded in the structure. Then I mentioned we have src/lib/list.c instead which can be used for that. In this case this change already happened during review, but let's say patch set 6 got committed as is (including unit tests) and then some time later we decided to switch to a common list framework throughout coreboot. Suddenly all the unit tests for files that used their own list structures like this before would become invalid because of this otherwise transparent change. Does this mean the test that still tests the same thing suddenly became an integration test, just because the list framework got factored out? Regardless of the ideological answer to that question, I think it is nonsensical from a practical standpoint to throw away or rewrite all those tests in such a situation. They would still be perfectly fine tests -- in fact, you *want* to run them after that refactoring change to make sure it behaves with the new list implementation exactly the same as before, that's the main point of having tests! What we don't want is create a barrier to helpful refactoring and cleanup by creating an artificial requirement to rewrite a disproportionate amount of tests from a small, otherwise transparent change.
Thanks, it makes sense. Examples which you provided have slightly changed my view on this.. :)
I think the most practical solution here would be to have a Makefile.inc in every test subdirectory (same as we do for the normal source tree) and then allow every test to specify which files it needs to link. With the right Makefile magic you should be able to make that look decent enough... for example you could have a test/lib/Makefile.inc with something like:
fmap-test += src/commonlib/region.c fmap-test += test/helpers/rdev_mock_helper.c # whether test/lib/fmap-test.c and src/lib/fmap.c need to be added explicitly or get deduced automatically is something we can decide
# could also do something like this for the header mocking I mentioned in the other patch fmap-test-includes += test/include/mocks/arch/cache_mocks
# would be extensible to provide other stuff we may find we need later, too... fmap-test-stage = verstage fmap-test-config += CONFIG_NO_FMAP_CACHE=y
We can also think of adding mock list (for linker flags) here. Let me prepare a more complex example and try to refactor Makefiles, so that the framework will be more flexible. In general considering discussion above, we will switch a definition of unit from module (.c) to functionality (like fmap). Or to be more precise mix of them (since I expect commonlib functions to not be as complicated as fmap).
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@13 PS1, Line 13: {"is\0very", 2},
Maybe add a comment that strlen is only 2 because of the embedded \0 character?
Initially, I planned that this commit won't be merged into the tree, and be only an example (actually too simple example, will work on this) for community. However, after all it may be worth to have something like this for devs (with proper description in the doc), who would like to start writing test for their code.
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);
In general, taking strlen of NULL (or str-anything of NULL) is a bad idea. […]
The only purpose was to have test failing, showing that Cmocka can catch such a failure. Below is the output from running this test: [==========] Running 2 test(s). [ RUN ] test_strlen_null Test failed with exception: Segmentation fault
[ FAILED ] test_strlen_null [ RUN ] test_strlen_strings [ OK ] test_strlen_strings [==========] 2 test(s) run. [ PASSED ] 1 test(s). [ FAILED ] 1 test(s), listed below: [ FAILED ] test_strlen_null
1 FAILED TEST(S)
Cmocka has exception handling for signals, so it is able to recover. However, I cannot find a macro like Gtests' "EXPECT_DEATH" in Cmocka. Do you find this as a major issue?