Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59075 )
Change subject: [RFC] tests: test write protection ......................................................................
Patch Set 5:
(13 comments)
Patchset:
PS5: Thank you for working on tests!! I did more detailed code review. I didn't look into previous patches in this chain, but tell me if I should.
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/59075/comment/1b8079ce_3cc9d432 PS5, Line 26: write_protect I know I asked you to rename the file, but now I did more detailed review and I think the better name would be chip_wp.c, since there are similarities with existing chip.c. How bad is to rename it again? :) And old name is still in tests.c, tests.h so you would need to go through the code once more anyway. Thanks!!
File tests/tests.h:
https://review.coreboot.org/c/flashrom/+/59075/comment/bb96dfa6_a862f7c5 PS5, Line 67: write_protection.c chip_wp.c
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/59075/comment/1115a91d_69922325 PS5, Line 357: write_protection chip_wp
https://review.coreboot.org/c/flashrom/+/59075/comment/8fe473a5_2f91010c PS5, Line 364: write_protection.c chip_wp.c
File tests/write_protect.c:
https://review.coreboot.org/c/flashrom/+/59075/comment/3f5c0b70_e44e4e6e PS5, Line 27: No mocking. Using WP emulation in dummy programmer. Maybe a complete phrase is better, like "Tests in this file do not use any mocking, because using write-protect emulation in dummyflasher programmer is sufficient".
https://review.coreboot.org/c/flashrom/+/59075/comment/fb8b3054_3c2f6241 PS5, Line 101: invalid_setup We can be more specific in test name, since it covers invalid range (not just any invalid setup). Invalid wp range, to be even more specific. So maybe invalid_wp_range_dummyflasher_test_success
https://review.coreboot.org/c/flashrom/+/59075/comment/37d9d026_95013534 PS5, Line 117: struct flashrom_wp_chip_config cfg; Lets move this declaration in the beginning , together with flash, layout, mock_chip. (And the same for other test methods)
https://review.coreboot.org/c/flashrom/+/59075/comment/3afbe98d_8de24338 PS5, Line 120: /* Invalid range. */ : range.start = 0x1000; : range.len = 0x1000; why chip_len is initialised at creation time, but start/len initialised separately, few lines later? they are initialised from constants anyway.
https://review.coreboot.org/c/flashrom/+/59075/comment/3fddf531_213b8902 PS5, Line 143: wp=no I haven't looked at the rest of your chain (tell me if I should), but this looks confusing: param says "wp=no", but test name is "setup_wp". So it wp enabled or disabled? what are you testing?
https://review.coreboot.org/c/flashrom/+/59075/comment/4c246984_b2c4c0e6 PS5, Line 152: /* Use first 4 KiB for a range. */ : range.len = 0x1000; : assert_int_equal(0, flashrom_wp_set_range(&cfg, &range)); : : /* Change range to last 4 KiB. */ : range.start = 0x00fff000; : assert_int_equal(0, flashrom_wp_set_range(&cfg, &range)); This code looks like as if test is testing two scenarios? If there are indeed two different scenarios, you need to split into two tests. Each test would init wp_range once (and init the rest) and then run scenario. To have multiple assertions in one test is totally fine. But this looks like: "lets setup a range, test it works... now lets setup a different range, test it works" which should be two tests.
https://review.coreboot.org/c/flashrom/+/59075/comment/0557a3de_ab8f252e PS5, Line 164: /* Switch modes in configuration only. */ : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_HARDWARE)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_DISABLED)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_POWER_CYCLE)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_PERMANENT)); Do all these need flash context, chip, layout? If not, extract into separate small tests that only declare `struct flashrom_wp_chip_config cfg;` and then do *one* assertion. Yes, you will have many small tests! This is perfect :) Small granular tests are very easy to debug, and easy to understand what's broken when they fail on someone's patch.
https://review.coreboot.org/c/flashrom/+/59075/comment/2a56f918_722059f9 PS5, Line 170: /* Switch modes in dummyflasher. */ : assert_int_equal(0, flashrom_wp_write_chip_config(&flash, &cfg)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_POWER_CYCLE)); : assert_int_equal(0, flashrom_wp_write_chip_config(&flash, &cfg)); : : /* Check final mode. */ : assert_int_equal(0, flashrom_wp_read_chip_config(&flash, &cfg)); : assert_int_equal(0, flashrom_wp_get_mode(&cfg, &mode)); : assert_int_equal(WP_MODE_PERMANENT, mode); Which ones of these need layout, if any? Which ones of these need dummyflasher? Do these actions have to be performed in this sequence, or are they independent?
Independent actions need to be independent tests. Also for setup, you don't need to invoke heavy-weight setup if it's not needed. Ideally you only setup what you need for the given test.
Same questions apply to "wp_init_from_status_dummyflasher_test_success".