Name of user not set #1002873 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:
(8 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. […]
Makes sense, wasn't clear enough here. I wanted to say, that if mock needs to be very complex, then such a unit test should be tested on its own (what we don't want and won't do). In such case (complex mock), unit testing code is susceptible to failing due to complexity of the logic. I'm not sure what do you mean by "unrelated changes elsewhere in the code" - is this about unit test code? Or module under test? What about simpler version: "Mocking can handle some hardware dependencies. However, complex mocks make the unit test susceptible to failing and can require significant development effort." ?
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 l […]
Makes sense.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 88: is a plus
"requirement" and "a plus" are mutually exclusive. […]
Sure. I will add "nice-to-have" items in a separate group.
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?
My point is - ideally tests should be written in C, since this is the language which all coreboot developers are familiar with. Will rewrite this bullet and description.
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". […]
Will add this to nice-to-have items.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 190: Cmocka also include support : for mocks.
One thing to investigate would be how we can mock out headers and inline functions. […]
Yes, this makes sense. We need to combine preprocessor and linker tricks in order to do this. First replace an original header with a "test header" (remove static inline), then mock such function out. However, with this approach there will be an issue of sync between original header and test header. In general, unit testing hardware-related code is a demanding task, which I plan to address later. That being said, it's good to discuss and prepare architecture for doing this.
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. […]
Yes, I will remove this. Initially I thought about this paper to be kind of my subjective opinion, which will be a starting point for a discussion. But let's keep this more formal and objective.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 232: prefix
(BTW, as I think about this more I'm pretty sure we won't always be able to have the test for X. […]
My point with object files' names is that I don't want to mimic directory structure under /build since I want to avoid confusion between products of normal coreboot build and unit test build of particular module. Yes, I need to address our problem with multiple Kconfig settings for one module (we have another thread in the commit with my example code), but I'm not a fun of multiple tests per file. Let's see if we can go with some different approach.