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 4: Code-Review+1
(5 comments)
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 };
Yeah... […]
I like your idea, more portable implementation.
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);
I'd say this is a mock in the purest sense of the word? struct region_device is a class (as far as C […]
I didn't want to say that your naming is improper. Object of this struct is a mock as you said. But for last couple of weeks I'm consistently working on Cmocka only, so when I hear "mock" I'm thinking - wrapping a function and mocking its implementation.. which is a rather shallow view when thinking about this more :) So feel free to ignore this and keep mock_rdev.
https://review.coreboot.org/c/coreboot/+/41046/3/tests/commonlib/region-test... PS3, Line 119: will_return_maybe(mock_mmap, mmap_result);
Is there some good way we can put this in the Makefile, maybe? Some automatic check to make sure the […]
Yes, pkg-config should do the trick with checking package version (and automatically provide flags for linker). However, I'm a little bit worried about how this will work on different distributions (since I think devs here are using different build systems). Will need to check this before proposing a solution.
https://review.coreboot.org/c/coreboot/+/41046/4/tests/commonlib/region-test... File tests/commonlib/region-test.c:
https://review.coreboot.org/c/coreboot/+/41046/4/tests/commonlib/region-test... PS4, Line 212: assert_ptr_equal(rdev_mmap(&mock_rdev, offs, wrap_size), NULL); Can you add a short comment why we are expecting failure of API calls here? Similar to what you have done for test_rdev_chain. This way, your test acts (almost) as a documentation, explaining what the rdev module is doing.
https://review.coreboot.org/c/coreboot/+/41046/4/tests/commonlib/region-test... PS4, Line 247: /* Remaining test use rdev chained to [child_offs:child_size) subregion. */ nit: *tests