Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk, Alexander Goncharov.
Felix Singer 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 6:
(1 comment)
File tests/libusb_wraps.c:
PS5:
Why? In all previous tests, wraps were added in the same patch with the test that needed those wraps […]
Commits should only do one thing and not multiple things. I think this commit does two things:
* Introducing lots of functions in common code * Introducing tests which are based on the new common code
Speaking generally, I think it's a bad practice to submit a commit of this kind, because it's not atomic. The commit title doesn't even give me an idea that new common code is introduced, but many comments are related to the common code.
Assuming that future commits get merged, which depend on the newly introduced common code, and we decide or we would like to revert this commit (for whatever reason, everything can happen), then we also have to revert all others that depend on it.
Don't get me wrong. I'm not saying that this will be the case for this commit. I just would like to move to this practice, since it gives us more possibilities in the future when we try or even have to fix something. Like a revert might be faster and easier than trying to fix the specific issue.
Other advantages: People are able to concentrate on the specific changes during the review and the comment history stays focused.
-
Felix Singer (Code Review)