Attention is currently required from: Felix Singer, Thomas Heijligen, Alexander Goncharov.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67664 )
Change subject: tests: add basic lifecycle test for ch341a_spi ......................................................................
Patch Set 5:
(10 comments)
Patchset:
PS5: Nice work, thanks for the patch!
File tests/ch341a_spi.c:
https://review.coreboot.org/c/flashrom/+/67664/comment/afbcd384_b54474da PS5, Line 22: transfer->status = LIBUSB_TRANSFER_COMPLETED; : transfer->actual_length = transfer->length; : transfer->callback(transfer); Are you sure these lines needed? What happens if you remove them, and just return 0? Maybe I am missing something, but I looked through the programmer code and I can't find where status, actual_length and callback are used? Especially, calling callback in a mock should not be needed, since return value is not used (or maybe it's void).
https://review.coreboot.org/c/flashrom/+/67664/comment/78fa1389_087fe5ce PS5, Line 43: mediatek_i2c_spi_basic_lifecycle_test_success This is a different test probably a copy-paste ;) You need to use your test method name here, `ch341a_spi_basic_lifecycle_test_success`
The meaning behind this is: if the programmer is disabled in the build, the corresponding test should not run either (since there is nothing to test).
File tests/libusb_wraps.c:
PS5:
The new added wraps should go into a separate patch pointing to this one.
Why? In all previous tests, wraps were added in the same patch with the test that needed those wraps.
https://review.coreboot.org/c/flashrom/+/67664/comment/14496f0e_a3359de5 PS5, Line 39: /* https://libusb.sourceforge.io/api-1.0/group__libusb__lib.html#ga5f8376b7a863... */
I'm going to delete such links after everyone says they're OK with the patch. […]
Alright, thank you! I will just mark this unresolved so that you won't forget to remove links at the last moment :)
https://review.coreboot.org/c/flashrom/+/67664/comment/d8eeb5b5_1f1d8d48 PS5, Line 75: You need 2 tabs here as indentation (see below __wrap_libusb_get_config_descriptor in the same situation)
https://review.coreboot.org/c/flashrom/+/67664/comment/b12c66dd_5a56331a PS5, Line 196: calloc(1, sizeof(struct libusb_transfer)); This is a custom logic (a small one but still), so it should go into custom mocks. Same as you did for __wrap_libusb_submit_transfer.
https://review.coreboot.org/c/flashrom/+/67664/comment/0f3672bb_6fd2ef67 PS5, Line 219: free(transfer); Same here, custom logic, please move to io_mock, and implement in tests/ch341a_spi.c
File tests/usb_unittests.h:
https://review.coreboot.org/c/flashrom/+/67664/comment/2409520b_9d2edd1c PS5, Line 28: CONFIG_CH341A_SPI == 1 You can run tests with this option enabled and disabled. When you run with option disabled, the test will be SKIPPED, but everything should be built and tests run (so, no errors).
To try that: In your builddir you can set option disabled, run meson configure -Dprogrammer=<list some programmers, but not ch341a> for example meson configure -Dprogrammer=dediprog,linux_mtd,linux_spi Then run tests, and check that tests for the rest of programmers are skipped as expected
Then to return back with everything enabled you can run meson configure -Dprogrammer=auto or meson configure -Dprogrammer=all depending on what was you previous setting
This will also verify that SKIP_TEST macro is set up correctly.
https://review.coreboot.org/c/flashrom/+/67664/comment/277c46a5_62332897 PS5, Line 58: struct libusb_transfer; If you add typedef like the others, you can avoid repeating `struct` in wraps.
-
Anastasia Klimchuk (Code Review)