Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.

View Change

13 comments:

• Patchset:

  • Patch Set #5:

    Thank you for working on tests!! I did more detailed code review. […]

    Previous patches in the chain make tests possible. I've seen your name in `git blame` for dummyflasher, so I'd appreciate if you could take a look at those changes.

• File tests/meson.build:

  • Patch Set #5, 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 nam […]

    Done

• File tests/tests.h:

  • Patch Set #5, Line 67: write_protection.c

    chip_wp. […]

    Done

• File tests/tests.c:

  • Patch Set #5, Line 357: write_protection

    chip_wp

    Done

  • Patch Set #5, Line 364: write_protection.c

    chip_wp. […]

    Done

• File tests/write_protect.c:

  • Patch Set #5, 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 wr […]

    Done

  • Patch Set #5, Line 101: invalid_setup

    We can be more specific in test name, since it covers invalid range (not just any invalid setup). […]

    Done

  • Patch Set #5, Line 117: struct flashrom_wp_chip_config cfg;

    Lets move this declaration in the beginning , together with flash, layout, mock_chip. […]

    Done

  • Patch Set #5, 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? […]

    There used to me more checks, but changes to WP patches cased their removal. Done.

  • Patch Set #5, Line 143: wp=no

    I haven't looked at the rest of your chain (tell me if I should), but this looks confusing: param sa […]

    Here "wp" means WP pin of the flash chip. Could rename it to "wppin" or "hwp" (hardware write-protection). The latter might be better because actual pin is negated and I wanted param to set state of hardware protection rather than state of a pin.

  • Patch Set #5, 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 scenario […]

    Broke it into set_wp_range_dummyflasher_test_success and set_wp_mode_dummyflasher_test_success.

  • Patch Set #5, 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 d […]

    Done

  • Patch Set #5, 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? […]

    Reduced checks performed by the two tests. Layout isn't needed, but dummyflasher is needed to test how WP code behaves on saving state.

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