Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Thomas Heijligen.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82251?usp=email )
Change subject: tests/erase: record the opcode for each erase ......................................................................
Patch Set 5:
(4 comments)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/82251/comment/ddd3453b_cee6c526 : PS5, Line 407: 5
Can this be a macro in flash. […]
Done. I also moved the declaration of this into flash.h, alongside the other `g_test_*_injector`s so it should trigger a compile-time error if somebody changes the definition without also changing the declaration.
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/82251/comment/015dbef3_8ea1a89d : PS5, Line 325: TEST_ERASE_INJECTOR
Maybe remove this value? I know some existing tests are using it, but they can be adjusted. […]
Done
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/82251/comment/d462a9bc_73829dd4 : PS5, Line 198: block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip
What if […]
I had avoided doing that just because it requires updating all of the existing test cases, but done now. It does seem worth verifying that, now that we have the capability.
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/82251/comment/c873eb11_21789d04 : PS5, Line 127: '-DFLASHROM_TEST',
Just to test , have you built with this option undefined? […]
Yes, building the regular flashrom binary still works.
I added this because it may not be clear that the features it currently guards are intended only to enable unit testing. Guarding them with a symbol that we only define when building tests makes that clearer, and doesn't add much complexity.
-
Peter Marheine (Code Review)