Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42117 )
Change subject: spi/winbond: Add a test for block protection status bits ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42117/3/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/c/coreboot/+/42117/3/src/drivers/spi/spi_flash.c... PS3, Line 339: int fill_spi_flash(const struct spi_slave *spi, struct spi_flash *flash, Sorry, our test infrastructure is still very new so there are a lot of exciting "firsts" to discover. I think you're the first one who wants to test a static function.
While I agree things need to be testable I always prefer tests to effect real firmware images as little as possible (and something like this may effect what the compiler can inline, for example). How about we create a new macro static_testable or something like that which resolves to 'static' when building firmware normally but to nothing when building for tests?
To tell the two apart you would have to add something like -D__UNIT_TEST__ to TEST_CFLAGS in tests/Makefile.inc. The macro itself could go into src/commonlib/bsd/include/commonlib/bsd/helpers.h (which is where we keep a lot of random utility stuff, it's always included as part of <types.h>).
https://review.coreboot.org/c/coreboot/+/42117/3/tests/drivers/winbond-spi-t... File tests/drivers/winbond-spi-test.c:
https://review.coreboot.org/c/coreboot/+/42117/3/tests/drivers/winbond-spi-t... PS3, Line 274: int do_printk(int msg_level, const char *fmt, ...) See my comment in https://review.coreboot.org/c/coreboot/+/42313/1/tests/lib/b64_decode-test.c... , I think we should be creating reusable stubs for all of these that are likely to be needed by many different tests. Here you could just reuse the console-stub.c I suggested to add on that CL.
https://review.coreboot.org/c/coreboot/+/42117/3/tests/drivers/winbond-spi-t... PS3, Line 296: void timer_monotonic_get(struct mono_time *mt) This would be another prime candidate for a common stub file. I would suggest actually calling clock_gettime() to get a real monotonic timer value here, otherwise code might behave weirdly because the struct mono_time that's passed in may be an uninitialized stack allocation (so it can be completely random, including making the time go backwards and things like that).