Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41046 )
Change subject: tests: Add region-test for rdev API ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/41046/1/tests/commonlib/region-test... File tests/commonlib/region-test.c:
https://review.coreboot.org/c/coreboot/+/41046/1/tests/commonlib/region-test... PS1, Line 172: rdev_mock_defaults();
Ack
Good to hear that someone else is also verifying test infrastructure!
I see your problem and frankly - I'm also surprised, that implementation is not stack-based as described in the doc. Let's see what will be the response from community. However I'm afraid, that introducing such a significant change in test framework may break a lot of tests people have already written.
If there will be no need for putting specific return values at the beginning of a queue (but simply put value on the stack), then we would be able to use Cmocka's test fixtures with some init function like your rdev_mock_defaults().
I think that your proposal is the best what we can have as of now. Initially I thought about changing implementation of rdev_mock_defaults() to have some extra parameters, which allows us configure return values per-test, but this will be more complicated I think.
https://review.coreboot.org/c/coreboot/+/41046/3/tests/commonlib/region-test... File tests/commonlib/region-test.c:
https://review.coreboot.org/c/coreboot/+/41046/3/tests/commonlib/region-test... PS3, Line 8: struct region outer = { .offset = 0x200000000, .size = 0x400000000 }; What about using some macros like SIZE_8GB, SIZE_16GB, or 0x390000000 -> (SIZE_16GB - SIZE_1_75GB)? My point is that, it is very easy to get lost with all these zeros.
https://review.coreboot.org/c/coreboot/+/41046/3/tests/commonlib/region-test... PS3, Line 97: struct region_device mock_rdev = REGION_DEV_INIT(&mock_rdev_ops, 0, 0xffffffffffffffff); In this driver, you are calling your fake region_device as mock_rdev, while in i2c-test, we have i2c_ex_dev (from _ex_ample). We are inconsistent here - do we want to change this (either here or there)? My issue with _mock_ is that it has nothing to do with mocking of functions - which is actually the most fundamental feature of Cmocka.
https://review.coreboot.org/c/coreboot/+/41046/3/tests/commonlib/region-test... PS3, Line 119: will_return_maybe(mock_mmap, mmap_result); Just a note, this test won't build by default on Ubuntu 16.04 (I expect newer ones to also have the same problem), since it is distributed with libcmocka0 1.0.1-2. This version doesn't have support for will_return_maybe() and -2 argument for expect_value_count(). Do you think, that I should document requirements regarding version of Cmocka lib somewhere?
https://review.coreboot.org/c/coreboot/+/41046/3/tests/commonlib/region-test... PS3, Line 184: const size_t wrap_offs = 0xffffffff00000000; This variable is used also for non-wrap cases, I will simply use offs. This init value for offset isn't necessarily indicating overflow condition, only with conjunction with some values of size.