Attention is currently required from: Sam McNally, Daniel Campello. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 )
Change subject: tests: assert pathname and flags when calling open() ......................................................................
Patch Set 7:
(10 comments)
Patchset:
PS7: Thanks!
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/63227/comment/31c81a69_f2ccfdf0 PS7, Line 63: /* Maximum number of open to mock. */ The number is probably arbitrary? If yes, you can add this clarification into a comment, "the number is arbitrary". This way, if in future we need more calls, it will be easier to increase the number (no need to search history for the reasons why the number is this).
https://review.coreboot.org/c/flashrom/+/63227/comment/4466f4c5_bc827d07 PS7, Line 64: 3 If you are in a good mood, let's make this number power of two :) like 4 for example.
https://review.coreboot.org/c/flashrom/+/63227/comment/8fe149e6_7e375ab7 PS7, Line 118: open_state Another thing: could you please add a comment above this line, to explain what that is, something like "An alternative to custom open mock, a test can register one of: either its own mock open function or fallback_open".
https://review.coreboot.org/c/flashrom/+/63227/comment/52561be1_1ce4126d PS7, Line 118: struct io_mock_open_state *open_state; This is, as I understand from the code, an alternative to custom open mocking, and mutually exclusive (a test can either mock open function or setup open state). Let's have more descriptive naming for this. For example: struct fallback_open_state *fallback_open
I remember we agreed with Edward on "fallback" name in some earlier patch, let's use this name.
File tests/io_mock.c:
https://review.coreboot.org/c/flashrom/+/63227/comment/bf96023f_0039c312 PS7, Line 24: assert_true(io == NULL || io->open == NULL || io->open_state == NULL); I would also add a comment above this line, to make it crystal clear: "A test can register only one of: either custom mock for open function, or fallback open state".
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/63227/comment/d90d19f8_415e9955 PS7, Line 77: wrap_open_and_friends Lets just call it "mock_open"
https://review.coreboot.org/c/flashrom/+/63227/comment/5e81318a_fdf67e4f PS7, Line 81: Could you please add new line between two conditionals? It would be easier to read.
https://review.coreboot.org/c/flashrom/+/63227/comment/5e1008fe_a5743e9c PS7, Line 84: Also here, please add new line between declaration of flags and asserts
https://review.coreboot.org/c/flashrom/+/63227/comment/f1e7592e_4acdbec9 PS7, Line 91: Also here, please add new line before return statement.
-
Anastasia Klimchuk (Code Review)