Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Evan Benn.

View Change

14 comments:

• Commit Message:

  • Patch Set #6, Line 7: Convert

    Replace Convert with Add.
    We cannot convert selfcheck yet, so it stays. Unit tests are added.

• File board_enable.c:

  • Patch Set #6, Line 2527: const size_t board_matches_size = ARRAY_SIZE(board_matches);

    Why this can't be in the test code?

• File tests/meson.build:

  • Patch Set #6, Line 25: well_formed

    I would rename all occurrences of "well_formed" into "selfcheck": file names, test methods names etc etc (and don't forget the comments ;)).
    The tests are verifying more than just tables being well formed, but also checking valid values in table rows.
    And secondly "selfcheck" is almost like a brand name at this stage, so let's keep the name.

• File tests/well_formed_tables.c:

  • Patch Set #6, Line 23: name

    Maybe rename to prog_name?

  • Patch Set #6, Line 26: #assertion

    I assume this would print "p is false" which can be confusing. I would add one more parameter to macro, string assertion_name and print that instead. Sp that you can pass "Programmer entry", "Programmer name" etc and have very clear error message.
    I would like the error message be very clear to the developer without needing to look into test code. I am happy to have more code if this achieves clearer error messages for test failures.

  • Patch Set #6, Line 26: is false

    is false or NULL

  • Patch Set #6, Line 24:

    	do {                                                                                   \
    		if (!(assertion))                                                              \
    			fail_msg(#assertion " is false for index:%zu name:%s", (index),        \
    				 (name) ? (name) : "unknown");                                 \
    	} while (0)
    

    I'm confused. Why is this a do-while? Is it really needed? It just runs one time.

    I was also confused in the beginning, but then I thought this is probably a trick to avoid swallowing a semicolon https://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html
    but I will let Evan tell us, because he knows for sure!

  • Patch Set #6, Line 33: (void)state; /* unused */

    Add a new line after `(void)state; /* unused */` and the same for all other test methods.

  • Patch Set #6, 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
    Lets keep it as is.

  • Patch Set #6, Line 51: assert_false(flashchips_size <= 1 || flashchips[flashchips_size - 1].name != NULL);

    Break down into two `assert_true`s

  • Patch Set #6, Line 58: }

    You missed `selfcheck_eraseblocks` functionality?
    It can be one more test method

  • Patch Set #6, Line 66:

    assert_false(board_matches_size <= 1
    		     || board_matches[board_matches_size - 1].vendor_name != NULL);
    

    Break down into two `assert_true`s

  • Patch Set #6, Line 83: SKIP_TEST

    Indent with tab

  • Patch Set #6, Line 84: // CONFIG_INTERNAL

    /* comment */

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