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:
(31 comments)
Hi,
After long break caused by my vacation, I'm sending version 2 of coreboot unit test proposal, which is a significant rework of unit tests build subsystem. Highlights of most important changes v1->v2: -> Address all comments from v1; -> Allow to compile and link more than 2 modules for particular unit tests; -> Autogeneration of Kconfig for unit tests; -> Create Makefile.inc tree similar to what is done under src/; -> Update documentation; -> Put Makefile and two examples each in a separate commits; -> Rebase on origin/master.
Thanks for all the comments for v1, looking forward to your remarks.
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.
I like the simple version you propose. […]
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 62: and : to some extent hardware-related code
Makes sense.
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 67: Requirements
I agree with Jan; unit tests should build and run on the host. […]
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 79: JUnit-like XML output format for Jenkins
Yes, I think that any machine parsable output should work. […]
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 84: however this is very unlikely
I would re-word this to say that the compiler for the host must support the same language standards […]
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 88: is a plus
Sure. I will add "nice-to-have" items in a separate group.
Done
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). […]
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 94: Assumption that all firmware devs know C
OK, then state that instead, that unit tests will be written in C because the code being tested is a […]
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 97: Proper
Compatible, or maybe Acceptable
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 100: GPL
Patrick, […]
I think we can close this thread considering example I have shown. However I haven't contacted with any lawyer, so if you think we should still block this, please let me know.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 105: for obvious reasons
I agree.
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 142: satisfying
satisfactory
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 151: ouf
out
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 156: * Easy to integrate with an IDE
Will add this to nice-to-have items.
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 173: works
work?
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 176: Pros
Sure. […]
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 187: It
Cmocka
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 187: proposal is to go with
"we propose using the"
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 188: evaluation criteria we have
stated evaluation criteria
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 190: include
includes
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 190: Cmocka also include support : for mocks.
Well, I think once you have unit tests there's always some work to keep them in sync when the tested […]
In v2 I've added "assert.h" mocking header, which replaces dead_code() macro with mock_assert(). This header is actually generic and used through all codebase, so it make sense to put this in for all unit tests. For the per-tests mock headers, we will need to add an option to append test-cflags before all default coreboot headers.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 195: In the same time
Sure.
Done
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 […]
Done
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.
Yes, I will remove this. […]
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 206: form
Sure.
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 211: brings
cause
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 231: every test file name : should have added a prefix "-test"
Why? Seems like unnecessary boilerplate if the directory structure already makes clear which files a […]
Done
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 232: prefix
My point with object files' names is that I don't want to mimic directory structure under /build s […]
Seems that we are not on the same page. In general, I was referring to my old proposal, where there were only two object files - test harness and uut. This is no longer valid with v2 completely reworked. New proposal is to generated per-test dir under build/tests/ and test binary will be placed in the same dir as main test harness (with "-test" suffix). It will be better to discuss inline in v2 code what do you think about my idea.