Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56911 )
Change subject: tests: add init_shutdown test for realtek_mst_i2c_spi ......................................................................
Patch Set 5:
(10 comments)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/56911/comment/a9169b01_3f4e8764 PS5, Line 217: if (strcmp(pathname, "/dev/i2c-254") != 0) Actually, given that this is test, let's make it an assert. In the case `assert_string_equal()` , full API is here https://api.cmocka.org/group__cmocka__asserts.html
The reason is: if pathname is incorrect, assert will fail with a message like " <some string> != "/dev/i2c-254" line 217 init_shutdown.c" , and by looking at the line 217 it would be clear where the error comes from.
Without assert, execution returns back to realtek_mst_i2c_spi.c and at some point init function fails, and the test fails with message like "0 != 1 line 29 init_shutdown.c". So you can only know that programmer_init failed and would need to spend more time debugging.
https://review.coreboot.org/c/flashrom/+/56911/comment/ff664d45_3a32c579 PS5, Line 219: if ((flags & O_RDWR) != O_RDWR) assert_int_equal
https://review.coreboot.org/c/flashrom/+/56911/comment/21a91ca1_00732b77 PS5, Line 229: if (fd != REALTEK_MST_MOCK_FD) assert_int_equal
https://review.coreboot.org/c/flashrom/+/56911/comment/6f8d2981_822b3fc4 PS5, Line 231: if (request != I2C_SLAVE) assert_int_equal
https://review.coreboot.org/c/flashrom/+/56911/comment/fc2169fd_e34618cb PS5, Line 235: if (addr != 0x4a) assert_int_equal
https://review.coreboot.org/c/flashrom/+/56911/comment/a9d0680d_ef007adc PS5, Line 243: if (fd != REALTEK_MST_MOCK_FD || sz != 1) assert_int_equal twice (for FD and for sz)
https://review.coreboot.org/c/flashrom/+/56911/comment/b813e3df_a0becbb4 PS5, Line 250: if (fd != REALTEK_MST_MOCK_FD) assert_int_equal
https://review.coreboot.org/c/flashrom/+/56911/comment/74028640_6fb3fe95 PS5, Line 252: if (sz == 1 || sz == 2) assert_in_set
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/56911/comment/b269771c_1d1a2342 PS5, Line 78: int (*open)(void *state, const char *pathname, int flags); In addition to my comment in tests.c: one more line here: int (*open64)(void *state, const char *pathname, int flags);
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/56911/comment/f3e73aab_0208fe2d PS5, Line 93: return __wrap_open(pathname, flags); When I said "wrap doesn't need to redirect to another wrap" I meant this:
LOG_ME; if (current_io && current_io->open64) return current_io->open64(current_io->state, pathname, flags); return MOCK_HANDLE;
And then if the tests want to handle both open and open64 in the same way, const struct io_mock realtek_mst_io = { .open = realtek_mst_open, .open64 = realtek_mst_open, ...... }
The reason is: this layer (tests.c) is the same for all tests, I don't want to make assumptions what some test needs, test can decide. You are right, in most cases it doesn't matter, but let the test decide.
-
Anastasia Klimchuk (Code Review)