Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk.

10 comments:

File tests/chip_wp.c:

/* Check initial mode. */
	assert_int_equal(0, flashrom_wp_get_mode(&cfg, &mode));
	assert_int_equal(WP_MODE_DISABLED, mode);

	/* Switch modes in dummyflasher. */
	assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_PERMANENT));
	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));

	/* Final mode should be "permanent". */
	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);

I think it's both: […]

Yes it's better, I mark this one as resolved. There are two minor comments on this test method, but big question is solved.

File tests/chip_wp.c:
- Patch Set #10, Line 141:
```
	/* Use first 4 KiB for a range. */
	range.len = 0x1000;
	assert_int_equal(0, flashrom_wp_set_range(&flash, &cfg, &range));

	/* Change range to last 4 KiB. */
	range.start = 0x00fff000;
	assert_int_equal(0, flashrom_wp_set_range(&flash, &cfg, &range));
```
  This looks like 2 in 1 tests (1 for first 4KiB range, 2 for last 4KiB).
  Is there a reason to have set range twice in a sequence, or this can be two tests?
- Patch Set #10, Line 159: set_wp_mode_dummyflasher_test_success
  I think the test is now good, given the comment, but maybe better name is "switch_wp_mode_...." instead of set_wp_mode....
- Patch Set #10, Line 188:
```
    /*
     * Check that "permanent" mode can't be unset and it's not considered to be
     * an error at the level of library calls (verification should be done
     * higher).
     */
```
  That's a very good comment, thank you! Only need to fix indentation.
- Patch Set #10, Line 220:
```
cfg.srp_bit_count = 1;
	assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_DISABLED));
```
  I see two parts here: 1) with srp_bit_count = 2, 2) with srp_bit_count = 1
  Similar to other tests, a question is whether this is a sequence needed for a test scenario (then just add a comment), or this are two independent scenarios (then split).
  And the same for 3 more tests below.
- Patch Set #10, Line 290:
```
	assert_int_equal(0x004000, range.start);
	assert_int_equal(0xffc000, range.len);
```
  just to check, do range start/len come from spi_status? the comment above param_dup only mentions register bits.
- Patch Set #10, Line 310:
```
	assert_int_equal(0, flashrom_layout_include_region(layout, "head"));
	assert_int_equal(0, flashrom_layout_include_region(layout, "tail"));
```
  It is not clear in the context of test method where "head" and "tail" come from. I think these two assertions need to be moved to setup_chip.
- Patch Set #10, Line 313: Write protection takes effect only after changing SRP values.
```
if you can add a bit more for example
 , so at this stage WP is not enabled and erase completes successfully.
```
  or something similar
- Patch Set #10, Line 329: Try erasing the chip again
```
also you can add continue the comment say
 , however because now wp is enabled, erase operation fails.
```
  or something
- Patch Set #10, Line 364:
```
	printf("Erase chip operation started.\n");
	assert_int_equal(0, do_erase(&flash));
	printf("Erase chip operation done.\n");
```
  Oh now I compare this test scenario and previous, and looks like first 4 KiB are special? Because in this scenario, erase completes successfully even with wp enabled?
  Let's add more comments to explain what these two tests are testing, and what is expected behaviour. The code itself looks good, just need to be more documented.

To view, visit change 59075. To unsubscribe, or for help writing mail filters, visit settings.