Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39893 )
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Patch Set 1:
(20 comments)
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... File Documentation/technotes/2020-03-unit-testing-coreboot.md:
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 52: This dependencies : may be broken using mocking concept, to some degree. However when mocks need to : be too complex, then such a unit test cannot address an idea of being quick and : simple. The way this sentence is written, it makes it sound like mocking will cause the code not to compile. Consider rewording as "Mocking can handle some hardware dependencies. However, complex mocks can require significant development effort and make the unit test susceptible to failing when other unrelated changes occur elsewhere in the code."
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 62: and : to some extent hardware-related code I think it is sufficient to just state the areas that will benefit the most from unit testing, and leave hardware-related code out of this statement.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 67: Requirements
In general, I don't like the idea of running unit tests on the actual device. […]
I agree with Jan; unit tests should build and run on the host. The faster the unit tests build and run, the more often people will run them, or even include them as part of a standard build.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 84: however this is very unlikely
Sounds good.
I would re-word this to say that the compiler for the host must support the same language standards as the target compiler, and then to Martin's comment, explicitly state that for some x86 targets, the host compiler and the target compiler could be the same, but that this is not a requirement.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 88: is a plus "requirement" and "a plus" are mutually exclusive. Consider making a separate section with these "plus" items, so that it's clear that we would like these things, but if they are not present it isn't a deal-breaker.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 90: The bigger community the better Explain this a little more. We want a larger community for (fill in the blank). These reasons could be general support/tutorials/knowledgeable people, bug fixes, a larger reviewer pool for any upstream improvements that we may choose to submit, etc.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 92: Language similarity Similarity to what? It's unclear what you're requiring here.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 94: Assumption that all firmware devs know C How does this relate to the bullet point that it's nested under? What are we stating here?
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 97: Proper Compatible, or maybe Acceptable
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 142: satisfying satisfactory
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 156: * Easy to integrate with an IDE This was neither stated as a requirement nor as a "nice to have". The evaluation should be based on requirements and desirables, so if IDE integration is a valid evaluation criterion, then add it to a section above.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 173: * It will require some effort to make it works from within an IDE Same comment - IDE integration has not been stated as a requirement or a desirable.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 179: * There are some (rather easy) hints how to use this from an IDE (e.g. Eclipse) Same comment - IDE integration has not been stated as a requirement or a desirable.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 187: It Cmocka
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 187: proposal is to go with "we propose using the"
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 188: evaluation criteria we have stated evaluation criteria
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 190: include includes
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 193: There are some cons, like for example lack of automatic test registration, : however this is minor issue, just requires minimal additional work from : developer. "Cmocka's limitations, such as the lack of automatic test registration, are considered minor issues that will require only minimal additional work from a developer."
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 198: That being said, Unity and GoogleTest frameworks are also very good. However : GoogleTest requires C++ familiarity and compiler, and Unity is rather not too : popular framework (which doesn’t mean worse). It is worth to mention, that such : evaluation will always be impacted by author subjective point of view. I'm not sure we need this paragraph at all. However, if we do, it should be rephrased to say that we decided not to use GTest/GMock because it requires C++ and the code being tested is pure C, and we decided not to use Unity because the community is subjectively too small.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 211: brings cause