Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43305 )
Change subject: tests: Add lib/memrange-test test case ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/43305/3/tests/lib/memrange-test.c File tests/lib/memrange-test.c:
https://review.coreboot.org/c/coreboot/+/43305/3/tests/lib/memrange-test.c@3... PS3, Line 31: struct device mem_device_mock[] = {
Yeah, I have a strong feeling that next test case for bootmem is much better this one.. […]
Done
https://review.coreboot.org/c/coreboot/+/43305/3/tests/lib/memrange-test.c@3... PS3, Line 36: static void test_memrange_basic(void **state)
It would be good to have 1 or 2 sentences describing what each of these tests are verifying.
Done
https://review.coreboot.org/c/coreboot/+/43305/3/tests/lib/memrange-test.c@6... PS3, Line 66: /* Remove initial memrange */
Yeah, the comments would be fine, too. […]
Done
https://review.coreboot.org/c/coreboot/+/43305/4/tests/lib/memrange-test.c File tests/lib/memrange-test.c:
https://review.coreboot.org/c/coreboot/+/43305/4/tests/lib/memrange-test.c@2... PS4, Line 21: struct resource res_mock[] = {
nit: Might be nice to have some more edge case tests (e.g. […]
I would like to polish this now and create complete solution (for sure there will be next versions, but I want to include as much as I can if there are only suggestions to improve).
There is already a range above 4GB (RESERVED one). I will add one more that starts at 0.
https://review.coreboot.org/c/coreboot/+/43305/4/tests/mocks/device/device_u... File tests/mocks/device/device_util.c:
https://review.coreboot.org/c/coreboot/+/43305/4/tests/mocks/device/device_u... PS4, Line 14: all_devices = mock_ptr_type(struct device *);
Why don't you just define a global all_devices in your test file (and then compile in the real searc […]
That is a good question. My intention was to isolate this particular functionality (and thus tests using it) from whole device/device_util.c. I thought (without enough checking) that device_util.c depends on some other modules and I will need to live with all these dependencies. However, I checked that it is enough to define all_devices pointer and compile original module in the tests. Indeed, this seems like a cleaner way. Let me change this.
-
Jan Dabros (Code Review)