Attention is currently required from: Nikolai Artemiev, Peter Marheine, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82251?usp=email )
Change subject: tests/erase: record the opcode for each erase ......................................................................
Patch Set 6:
(7 comments)
Patchset:
PS6: Sorry for delay in review! This looks great, I only have few small comments. This is very useful addition to unit tests!
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/82251/comment/ac3849e9_432c58bd?usp... : PS6, Line 334: /* special cases must come last. */ I think this comment better go under FLASHROM_TEST, otherwise it is orphaned in the non-test code. Don't know if anyone ever looks into generated code after pre-processor stage (I did few times only), but still.
https://review.coreboot.org/c/flashrom/+/82251/comment/62256236_74314662?usp... : PS6, Line 335: #ifndef FLASHROM_TEST : }; : #else : TEST_ERASE_INJECTOR_1, : TEST_ERASE_INJECTOR_2, : TEST_ERASE_INJECTOR_3, : TEST_ERASE_INJECTOR_4, : TEST_ERASE_INJECTOR_5, : }; : : #define NUM_TEST_ERASE_INJECTORS 5 : extern erasefunc_t *g_test_erase_injector[NUM_TEST_ERASE_INJECTORS]; : #endif This style is different from the others (read and write), ifndef vs ifdef and I know it saves few lines, but I think maybe in the case consistency would be good?
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/82251/comment/76917ae6_6e134596?usp... : PS5, Line 198: block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip
I had avoided doing that just because it requires updating all of the existing test cases, but done […]
Thank you for doing this. I think it's valuable to test, if we have test running we prevent it from being broken in future!
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/82251/comment/db5f2548_b18f0b00?usp... : PS6, Line 205: block_erase_chip_1, : block_erase_chip_2, : block_erase_chip_3, : block_erase_chip_4, : block_erase_chip_5, I think it can help readers, to add per-line comment, since the suffixes (1, 2, 3, 4, 5) do not correspond to block-length/opcodes (1b, 2b, 4b, 8b, 16b). I know this can be deducted by going through all the code, but a comment here can be a good time-saver.
Either, each line N comment `TEST_ERASE_INJECTOR_N` or, `erasefn for opcode with corresponds to block size N` or maybe you prefer some other comment
https://review.coreboot.org/c/flashrom/+/82251/comment/e9aa0334_08b0bae1?usp... : PS6, Line 649: {0xf, 0x1, TEST_ERASE_INJECTOR_1}, : {0x8, 0x1, TEST_ERASE_INJECTOR_1}, : {0x9, 0x1, TEST_ERASE_INJECTOR_1}, : {0xa, 0x1, TEST_ERASE_INJECTOR_1}, : {0xb, 0x1, TEST_ERASE_INJECTOR_1}, : {0xc, 0x1, TEST_ERASE_INJECTOR_1}, : {0xd, 0x1, TEST_ERASE_INJECTOR_1}, : {0xe, 0x1, TEST_ERASE_INJECTOR_1}, : {0x3, 0x1, TEST_ERASE_INJECTOR_1}, : {0x4, 0x1, TEST_ERASE_INJECTOR_1}, : {0x5, 0x1, TEST_ERASE_INJECTOR_1}, : {0x6, 0x1, TEST_ERASE_INJECTOR_1}, : {0x7, 0x1, TEST_ERASE_INJECTOR_1}, : {0x0, 0x1, TEST_ERASE_INJECTOR_1}, : {0x1, 0x1, TEST_ERASE_INJECTOR_1}, These lines have trailing whitespace (shows as bright pink in gerrit :))
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/82251/comment/fe23678e_d7ef82fa?usp... : PS5, Line 127: '-DFLASHROM_TEST',
Yes, building the regular flashrom binary still works. […]
After thinking more, I agree with you.