Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41046 )
Change subject: tests: Add region-test for rdev API ......................................................................
Patch Set 4:
(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();
Good to hear that someone else is also verifying test infrastructure! […]
Yeah, I could see many ways of how to make rdev_mock_defaults() more flexible to support more cases, but that would quickly become very complicated and like I wrote in that GitLab bug report I feel like I'd be basically reimplementing the stuff that the test framework was supposed to be there for then.
I agree chances for an upstream fix are probably unlikely. Let's go with this for now. If we have a couple of more tests we can decide how serious of a problem it really is and whether we need to build more custom infrastructure on top of (or maybe even fork) cmocka to address it.
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)? […]
Yeah... actually there's also a problem with these large sizes because the tests would run on a 32-bit arch. I would like to test them this large to make sure there are no large number handling issues, but we probably can't just have the tests not work on some hosts either.
I found a new way to solve this issue which I think works better.
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 […]
I'd say this is a mock in the purest sense of the word? struct region_device is a class (as far as C can have classes), struct region_device_ops are its methods, and this struct is a mock object implementing that class with the mock functions defined above as its methods.
But I also don't think we need to be super strict about naming local data in a test file. It doesn't really matter that much. In the vboot tests we call anything "mock" that's fake data for testing purposes in any sense of the word, that's just what I'm used to.
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. […]
Is there some good way we can put this in the Makefile, maybe? Some automatic check to make sure the cmocka version is larger than the minimum supported? (Maybe a bare -lcmocka is a bit brittle there anyway... don't people normally use pkg-config for this kind of stuff? I'm not very familiar with the practical aspects of building userspace code but I think that's what I see people do most of the time... and I think that has some kind of version awareness support as well?)
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. […]
Done