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

View Change

13 comments:

• Patchset:

  • Patch Set #5:

    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:

  • 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 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:

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

    chip_wp.c

• File tests/tests.c:

  • Patch Set #5, Line 357: write_protection

    chip_wp

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

    chip_wp.c

• 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 write-protect emulation in dummyflasher programmer is sufficient".

  • 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). Invalid wp range, to be even more specific. So maybe
     invalid_wp_range_dummyflasher_test_success

  • Patch Set #5, 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)

  • 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? they are initialised from constants anyway.

  • 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 says "wp=no", but test name is "setup_wp". So it wp enabled or disabled? what are you testing?

  • 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 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.

  • 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 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.

  • 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?
    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".

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