Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69620 )
Change subject: tests: Add selfcheck to unit tests ......................................................................
Patch Set 6:
(15 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69620/comment/bcb98b22_522cba6c PS6, Line 7: Convert
Replace Convert with Add. […]
Done
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/69620/comment/f1d180e1_08c13711 PS6, Line 2527: const size_t board_matches_size = ARRAY_SIZE(board_matches);
Why this can't be in the test code?
the size of the array is only known in the translation unit it is defined in
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/69620/comment/f6d42ec1_2eaba2ae PS6, Line 25: well_formed
I would rename all occurrences of "well_formed" into "selfcheck": file names, test methods names etc […]
Done
File tests/well_formed_tables.c:
https://review.coreboot.org/c/flashrom/+/69620/comment/dcdd942e_02a36d86 PS6, Line 23: name
Maybe rename to prog_name?
its only prog_name for one of the callsites
https://review.coreboot.org/c/flashrom/+/69620/comment/22ff445a_ce150d65 PS6, Line 26: is false
is false or NULL
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/6c50d1e5_8d6c15aa PS6, Line 26: #assertion
I assume this would print "p is false" which can be confusing. […]
#assertion is copied from what normal cmocka assertions print. We can do better.
https://review.coreboot.org/c/flashrom/+/69620/comment/2ff04049_0534353d PS6, Line 24: do { \ : if (!(assertion)) \ : fail_msg(#assertion " is false for index:%zu name:%s", (index), \ : (name) ? (name) : "unknown"); \ : } while (0)
I was also confused in the beginning, but then I thought this is probably a trick to avoid swallowin […]
yes its the standard semicolon trick for macros. So you can call it like a function.
https://review.coreboot.org/c/flashrom/+/69620/comment/db31623b_d14b0e8a PS6, Line 33: (void)state; /* unused */
Add a new line after `(void)state; /* unused */` and the same for all other test methods.
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/3f408137_585db4a4 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 […]
the original code does this check for all types except 'default', and for default it does an unconditional assertion. I replaced that assertion with assert(p->type), although that is not as good as it no longer checks for non-valid values of the enum. I added that back in.
https://review.coreboot.org/c/flashrom/+/69620/comment/45140d0d_f5bc04d8 PS6, Line 51: assert_false(flashchips_size <= 1 || flashchips[flashchips_size - 1].name != NULL);
Break down into two `assert_true`s
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/b2bc0543_4d49ff81 PS6, Line 58: }
You missed `selfcheck_eraseblocks` functionality? […]
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/43193fb6_5e543a55 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
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/83248581_22f7bf60 PS6, Line 71: assert_table(b->board_name, i, b->board_name); : assert_table(b->board_name, i, b->board_name);
The same line twice. […]
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/4da57f91_eb0b43fc PS6, Line 83: SKIP_TEST
Indent with tab
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/2b68e0b2_33a8f7b6 PS6, Line 84: // CONFIG_INTERNAL
/* comment */
Done