Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Evan Benn.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69620 )
Change subject: tests: Convert selfcheck to unit tests ......................................................................
Patch Set 6:
(14 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69620/comment/34556d50_b100b020 PS6, Line 7: Convert Replace Convert with Add. We cannot convert selfcheck yet, so it stays. Unit tests are added.
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/69620/comment/47182442_e17a34df PS6, Line 2527: const size_t board_matches_size = ARRAY_SIZE(board_matches); Why this can't be in the test code?
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/69620/comment/ae068992_f1013e8a PS6, Line 25: well_formed I would rename all occurrences of "well_formed" into "selfcheck": file names, test methods names etc etc (and don't forget the comments ;)). The tests are verifying more than just tables being well formed, but also checking valid values in table rows. And secondly "selfcheck" is almost like a brand name at this stage, so let's keep the name.
File tests/well_formed_tables.c:
https://review.coreboot.org/c/flashrom/+/69620/comment/82af0fc9_ef8cb9b5 PS6, Line 23: name Maybe rename to prog_name?
https://review.coreboot.org/c/flashrom/+/69620/comment/c152b936_7196a5a1 PS6, Line 26: #assertion I assume this would print "p is false" which can be confusing. I would add one more parameter to macro, string assertion_name and print that instead. Sp that you can pass "Programmer entry", "Programmer name" etc and have very clear error message. I would like the error message be very clear to the developer without needing to look into test code. I am happy to have more code if this achieves clearer error messages for test failures.
https://review.coreboot.org/c/flashrom/+/69620/comment/2875c2c3_2c4943c0 PS6, Line 26: is false is false or NULL
https://review.coreboot.org/c/flashrom/+/69620/comment/ab1dd362_20450fab PS6, Line 24: do { \ : if (!(assertion)) \ : fail_msg(#assertion " is false for index:%zu name:%s", (index), \ : (name) ? (name) : "unknown"); \ : } while (0)
I'm confused. Why is this a do-while? Is it really needed? It just runs one time.
I was also confused in the beginning, but then I thought this is probably a trick to avoid swallowing a semicolon https://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html but I will let Evan tell us, because he knows for sure!
https://review.coreboot.org/c/flashrom/+/69620/comment/36cb8b6f_9afe64bb PS6, Line 33: (void)state; /* unused */ Add a new line after `(void)state; /* unused */` and the same for all other test methods.
https://review.coreboot.org/c/flashrom/+/69620/comment/4ddce5ef_ade6c279 PS6, Line 41: if (strcmp("internal", p->name) != 0) : assert_table(p->devs.note, i, p->name); Original code has this check for all programmers with p->type == OTHER Lets keep it as is.
https://review.coreboot.org/c/flashrom/+/69620/comment/6bb0f54d_c2ab5d9d PS6, Line 51: assert_false(flashchips_size <= 1 || flashchips[flashchips_size - 1].name != NULL); Break down into two `assert_true`s
https://review.coreboot.org/c/flashrom/+/69620/comment/6b3c7499_d4a6c5b7 PS6, Line 58: } You missed `selfcheck_eraseblocks` functionality? It can be one more test method
https://review.coreboot.org/c/flashrom/+/69620/comment/05e52b4e_75f24017 PS6, Line 66: assert_false(board_matches_size <= 1 : || board_matches[board_matches_size - 1].vendor_name != NULL); Break down into two `assert_true`s
https://review.coreboot.org/c/flashrom/+/69620/comment/7b31618d_0053f817 PS6, Line 83: SKIP_TEST Indent with tab
https://review.coreboot.org/c/flashrom/+/69620/comment/b5ae9e91_9298d757 PS6, Line 84: // CONFIG_INTERNAL /* comment */