Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46458 )
Change subject: tests: Add lib/imd_cbmem-test test case ......................................................................
Patch Set 9:
(7 comments)
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c File tests/lib/imd_cbmem-test.c:
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c@... PS9, Line 45: assert_ptr_equal(cbmem_top_chipset(), cbmem_top()); Where is cbmem_top_chipset() defined for this test?
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c@... PS9, Line 111: static void test_cbmem_recovery(void **state) This doesn't really test cbmem_recovery. The reason this is called recovery is that when is_wakeup is true, an earlier CBMEM data structure that's already in memory can be recovered and reused. So a more complete test would be to copy in the raw data of an existing CBMEM structure with a couple of entries below _cbmem_top_ptr, run cbmem_recovery(1), and check that the entries have been correctly recovered. (Then do it again with cbmem_recovery(0) and check that CBMEM is empty afterwards.)
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c@... PS9, Line 151: } One important aspect of cbmem_add() is that when adding an ID that already exists, it will find the existing one rather than adding it again (i.e. compare pointers to make sure it returns the same entry again). Should maybe also compare pointers for different IDs to make sure the returned entries are *not* the same.
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c@... PS9, Line 168: Please also check the sizes (and it might be nice if you could use something else than CBMEM_ROOT_SIZE every once in a while... e.g. try sizes not divisible by 4 and make sure it still all works correctly with that, and pointers for new entries still conform to the enforced alignment even when the sizes of entries before that didn't).
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c@... PS9, Line 245: cbmem_add_bootmem(); You're just calling functions and ignoring their effects here, this doesn't really test anything. You should mock bootmem_add_range() and check that it is called with exactly the inputs expected (and maybe on a CBMEM with more than one entry).
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c@... PS9, Line 264: assert_int_not_equal(0, size); You should be able to check the exact values here (based on _cbmem_top_ptr and the size), not just that they're not 0.
https://review.coreboot.org/c/coreboot/+/46458/9/tests/lib/imd_cbmem-test.c@... PS9, Line 284: I think it might be good to have a general data structure format test: initialize CBMEM, add a few entries with sizes, write contents to them, then memcmp() the whole thing with a hardcoded copy of what it's supposed to look like. You could reuse the same hardcoded data that I suggested using to test cbmem_recovery() there. The CBMEM data format is generally not supposed to change, so this would be a good tests to make sure minor modifications to the source code don't change the final results.