Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62319 )
Change subject: tests/linux_spi: Validate param file path ......................................................................
Patch Set 4:
(6 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62319/comment/a214cb5d_ecae28a2 PS4, Line 10: linux_spi unit-test does not : check I think unit-tests in general had no implementation to mock repeated calls to open, it is not specific to linux_spi. Since you are adding generic open mock and state which support multiple calls to open, you can say something like "add ABCD infrastructure to unit tests and use this infra for linux_spi test"?
Also, maybe it is just me, but I didn't understand what "subsequent path validation" is until I read the code. Can we call it "repeated calls to open"? It really only supports open, not fopen or any other path.
Also I would emphasise it more, in a separate paragraph (even if small) that this patch also adds something to cover "/dev/null".
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/62319/comment/46b4d526_cf15c469 PS4, Line 39: #define DEFAULT_MOCK_FD 0xC0FE Is there a specific meaning behind this number? We have already a NON_ZERO which is defined as #define NON_ZERO (0xf000baaa)
is possible to reuse? NON_ZERO is defined in include/test.h so it can be used anywhere in tests.
https://review.coreboot.org/c/flashrom/+/62319/comment/0186ca20_981c8ca1 PS4, Line 41: default_io_mock_state I would like to rename this, I have two reasons in mind. The most important, "default" doesn't tell much about what the state actually use. Something like "multipath_io_state" gives information about the state. Another reason, I am not sure how much this is actually default, because default is no state. Also further in the chain in CB:62324 you create another struct with the same name, but it can't be two defaults with the same name.
https://review.coreboot.org/c/flashrom/+/62319/comment/dab03cc0_d7c4c5de PS4, Line 43: 2 Just wondering, why 2 ?
https://review.coreboot.org/c/flashrom/+/62319/comment/8ce143f4_396b04d2 PS4, Line 48: default_open For the same reasons as above, I would rename this to something more self-explanatory, maybe "multipath_open" ?
https://review.coreboot.org/c/flashrom/+/62319/comment/88ec349b_a9bb9031 PS4, Line 305: data I would keep the naming to indicate this piece of data belongs to linux_spi test, "linux_spi_io_state". Even if the struct type is shared between multiple tests, this piece of data belongs to this one test.
-
Anastasia Klimchuk (Code Review)