Name of user not set #1002873 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39893 )
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Documentation: Add proposal for firmware unit testing
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: I552d6c3373219978b8e5fd4304f993d920425431 --- A Documentation/technotes/2020-03-unit-testing-coreboot.md M Documentation/technotes/index.md 2 files changed, 257 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39893/1
diff --git a/Documentation/technotes/2020-03-unit-testing-coreboot.md b/Documentation/technotes/2020-03-unit-testing-coreboot.md new file mode 100644 index 0000000..fcc9b0c --- /dev/null +++ b/Documentation/technotes/2020-03-unit-testing-coreboot.md @@ -0,0 +1,256 @@ +# Unit testing coreboot + +## Preface +First part of this document, Introduction, comprises disambiguation for what +unit testing is and what is not. This definition will be a basis for the whole +paper. + +Next, Rationale, explains why to use unit testing and how coreboot specifically +may benefit from it. + +This is followed by evaluation of different available free C unit test +frameworks. Firstly, collection of requirements is provided. Secondly, there is +a description of a few selected candidates. Finally, requirements are applied to +candidates to see if they might be a good fit. + +Fourth part is a summary of evaluation, with proposal of unit test frameworks +for coreboot to be used. This may be a good starting point for a discussion +within community. + +Finally, Implementation proposal paragraph touches how build system and coreboot +codebase in general should be organized, in order to support unit testing. This +comprises couple of design considerations which need to be addressed. + +## Introduction +A unit test is supposed to test a single unit of code in isolation. In C +language (in contrary to OOP) unit usually means a function. One may also +consider unit under test to be a single compilation unit which exposes some +API (set of functions). A function, talking to some external component can be +tested if this component can be mocked out. + +In other words (looking from C compilation angle), there should be no extra +dependencies (executables) required beside unit under test and test harness in +order to compile unit test binary. Test harness, beside code examining a +routines, may comprise test framework implementation. + +Unit testing is not an integration testing and it doesn't replace it. First of +all, integration tests cover larger set of components and interactions between +them. Positive integration test result gives more confidence than a positive +unit test does. Furthermore, unit tests are running on the build machine, while +integration tests usually are executed on the target (or simulator). + +## Rationale +Considering above, what is the benefit of unit testing, especially keeping in +mind that coreboot is low-level firmware? Unit tests should be quick, thus may +be executed frequently during development process. It is much easier to build +and run a unit test on a build machine, than any integration test. This in turn +may be used by dev to gather extra confidence early during code development +process. Actually developer may even write unit tests earlier than the code - +see [TDD](https://en.wikipedia.org/wiki/Test-driven_development) concept. + +That being said, unit testing embedded C code is a difficult task, due to +significant amount of dependencies on underlying hardware. 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. + +Writing unit tests for a code (both new and currently existing) may be favorable +for the code quality. It is not only about finding bugs, but in general - easily +testable code is a good code. + +coreboot benefits the most from testing common libraries (lib/, commonlib/, +payloads/libpayload), coreboot infrastructure (console/, device/, security/) and +to some extent hardware-related code. + +## Evaluation of unit testing frameworks + +### Requirements +Requirements for unit testing frameworks: + +* Easy to use +* Few dependencies + + Standard C library is all we should need + +* Isolation between tests +* Support for mocking +* Easy to integrate with build system/build tools + + JUnit-like XML output format for Jenkins + +* Compiler similarity + + Ideally the same compiler should be used for building firmware executables + and test binaries, however this is very unlikely (even if the ARCH for build + machine and target is the same). At least the compiler on build machine should + support the same dialect as the compiler for target executables. + +* Popularity is a plus + + The bigger community the better + +* Language similarity + + Assumption that all firmware devs know C + +* Extra features may be a plus +* Proper license + + This should not be a blocker, since test binaries are not distributed. + However ideally GPL. + +### Candidates +There is a lot of frameworks which allow unit testing C code +([list](https://en.wikipedia.org/wiki/List_of_unit_testing_frameworks#C) from +Wikipedia). While not all of them were evaluated for obvious reasons, couple +of them were selected based on the good opinions among C devs, popularity and +fitting above criteria. + +* [SputUnit](https://www.use-strict.de/sput-unit-testing/) +* [GoogleTest](https://github.com/google/googletest) +* [Cmocka](https://cmocka.org/) +* [Unity](http://www.throwtheswitch.org/unity) (CMock, Ceedling) + +Three more frameworks should be mentioned here, however they weren't tried +within coreboot: + * [Check](https://libcheck.github.io/check/) + * [Criterion](https://github.com/Snaipe/Criterion/) + +They offer similar functionality as Unity, while at the same time don’t seem +to have similarly active communities. + + * [CUnit](http://cunit.sourceforge.net/) + +Another one which is rather widely used, however it doesn't have good support +for mocking. + +If anyone has good experience with some framework which is not listed above, it +will be highly appreciated to give a note about this to the author. In such +cases, it may be necessary to investigate such tools deeply and then reconsider +the decision. + +### Evaluation +* [SputUnit](https://www.use-strict.de/sput-unit-testing/) + * Pros + * No dependencies, one header file to include - that’s all + * Pure C + * Very easy to use + * Cons + * Main repo doesn’t have support for generating JUnit XML reports for + Jenkins to consume - this feature is available only on the fork from + SputUnit called “Sput_report”. It makes it niche in a niche, so there are + some reservations whether support for this will be satisfying + * No support for mocks + * Not too popular + * No automatic test registration + * BSD license +* [GoogleTest](https://github.com/google/googletest) + * Pros + * Automatic test registration + * Support for different output formats (including XML for Jenkins) + * Good support, widely used, the biggest and the most active community ouf + of all frameworks that were investigated + * Available as a package in the most common distributions + * Test fixtures easily available + * Well documented + * Easy to integrate with an IDE + * Cons + * Requires C++11 compiler + * To make most out of it (use GMock) C++ knowledge is required + * BSD license +* [Cmocka](https://cmocka.org/) + * Pros + * Self-contained, autonomous framework + * Pure C + * API is well documented + * Multiple output formats (including XML for Jenkins) + * Available as a package in the most common distributions + * Used in some popular open source projects (libssh, OpenVPN, Samba) + * Test fixtures available + * Support for exception handling + * Cons + * No automatic test registration + * It will require some effort to make it works from within an IDE + * Apache 2.0 license (not compatible with GPLv2) +* [Unity](http://www.throwtheswitch.org/unity) (CMock, Ceedling) + * Pros + * Pure C (Unity testing framework itself, not test runner) + * Support for different output formats (including XML for Jenkins) + * There are some (rather easy) hints how to use this from an IDE (e.g. Eclipse) + * Cons + * Test runner (Ceedling) is not written in C - uses Ruby + * Mocking/Exception handling functionalities are actually separate tools + * No automatic test registration + * Not too popular + +### Summary & framework proposal +After research, proposal is to go with Cmocka unit test framework. It fulfills +all evaluation criteria we have. It is rather easy to use, doesn’t have +extra dependencies, written fully in C, allows for tests fixtures and some +popular open source projects already are using it. Cmocka also include support +for mocks. + +There are some cons, like for example lack of automatic test registration, +however this is minor issue, just requires minimal additional work from +developer. In the same time, it may be worth to propose improvement to Cmocka +community or simply apply some extra wrapper with demanded functionality. + +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. + +## Implementation proposal + +### Framework as a submodule or external package +Unit test frameworks may be either compiled from source (form a git submodule +under 3rdparty/) or pre-compiled as a package. The second option seems to be +easier to maintain, while in the same time may bring some unwanted consequences +(different version across distributions, frequent changes in API). It make sense +to initially experiment with packages and check how it works. If this will +brings any issues, then it is always possible to switch to submodule approach. + +### Build configuration +While building unit under test object file, it is necessary to apply some +configuration (config) just like when building whole firmware. For simplicity, +one config.h may be pre-built and then used for building all unit tests. In the +same time, if integrated with Jenkins, it may be preferred to use every +available config for periodic builds. + +My proposal is to go with `qemu_x86_i440fx` config for usual developer builds. + +### Integration with build system +To get the most out of unit testing framework, it should be integrated with +Jenkins automation server. Verification of all unit tests for new changes may +improve code reliability to some extent. + +### Directory structure +Tests should be kept separate from the code, while in the same time it must be +easy to match code with test harness. + +My proposal is to create new directory for test files and every test file name +should have added a prefix "-test" to the basename of a corresponding unit +under test module. Below example shows how this may be organized: + +```bash +├── build +│ ├── tests <-test binaries +│ +├── src +│ ├── lib +│ │ ├── malloc.c <- unit under test +│ │ ├── string.c <- unit under test +│ ├── commonlib +│ ├── sort.c <- unit under test +│ +├── tests +│ ├── include +│ │ ├── config.h <- pre-built config used for tests' builds +│ ├── Makefile.inc +│ ├── lib +│ │ ├── malloc-test.c <- test code for src/lib/malloc.c +│ │ ├── string-test.c <- test code for src/lib/string.c +│ ├── commonlib +│ ├── sort-test.c <- test code for src/commonlib/sort.c +├── +``` diff --git a/Documentation/technotes/index.md b/Documentation/technotes/index.md index 7c231fc..5367e69 100644 --- a/Documentation/technotes/index.md +++ b/Documentation/technotes/index.md @@ -2,3 +2,4 @@
* [Dealing with Untrusted Input in SMM](2017-02-dealing-with-untrusted-input-in-smm.md) * [Rebuilding coreboot image generation](2015-11-rebuilding-coreboot-image-generation.md) +* [Unit testing coreboot](2020-03-unit-testing-coreboot.md)
Patrick Georgi 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:
(5 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 100: GPL compatible to GPL? BSD-l wouldn't raise questions, while Apache-l might do (even though we don't distribute the binaries)
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 105: for obvious reasons "because that would take an excessive amount of time"? Not sure we should talk about "obvious" in documentation.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 176: Pros please take care of trailing whitespace
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 195: In the same time At the same time (also a couple of times elsewhere in the doc), see e.g. https://www.italki.com/question/337681
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 206: form from
Julius Werner 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:
(2 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 100: GPL
compatible to GPL? BSD-l wouldn't raise questions, while Apache-l might do (even though we don't dis […]
Considering that cmocka (Apache) seems to be the candidate, how do we feel about the compatibility issue? I read your "it's fine because we don't distribute binaries" argument and I guess it sounds reasonable in theory, but I am not a lawyer... are there any other known examples of someone using cmocka (or generally any Apache test framework) with GPL code? Have we asked a lawyer about this?
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 are the tests?
Tim Wawrzynczak 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:
(1 comment)
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 232: prefix suffix
Martin Roth 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:
(3 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 67: Requirements Are any of the frameworks small enough to build into a linuxboot payload so we can run unit tests as a payload on the actual device? Would there be any advantage to this other than building everything with the same compiler?
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 79: JUnit-like XML output format for Jenkins I'm not positive that this is needed - any machine parsable output should work, and can be translated. I'd put this is a nice-to-have, but not a requirement.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 84: however this is very unlikely Maybe expand on this statement? -The host compiler will be used to build the tests, whereas the coreboot toolchain will be used for building the firmware executables.
Patrick Georgi 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:
(1 comment)
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 79: JUnit-like XML output format for Jenkins
I'm not positive that this is needed - any machine parsable output should work, and can be translate […]
Right, Test Anything Protocol would work as well (worst case we'd need to put in some converter). But there needs to be machine readable output in _some_ specified format.
Julius Werner 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:
(2 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 67: Requirements
Are any of the frameworks small enough to build into a linuxboot payload so we can run unit tests as […]
I feel like that would sort of defeat the purpose of unit tests, which is to be something that anyone can quickly and easily run from their local development machine. It would also preclude us from running them in the CI (which I think we definitely want).
I think we should use the crossgcc x86_64-pc-linux-gnu toolchain to build these (at least as an option, and that should be what the CI uses), then we hopefully cover most of the compiler-specific details that an actual firmware build would also have. Most unit tests (at least at first) could probably be architecture-independent (even for src/arch/ code, unless it specifically uses inline assembly or stuff). If we ever really want to test all architectures (probably a low priority thing compared to getting test coverage across the code base first), we'd probably want to do something with QEMU (like the Chrome OS SDK does).
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. Cmocka has nice mocking support using that __wrap_ linker feature, but unfortunately it only works for actual link-level functions. Many places in coreboot use inlines (and for important reasons they have to be inlined, and would maybe not even work right if you forced them to be not inline).
For example, say I wanted to unit test src/arch/arm64/armv8/mmu.c -- it would be great to have a test that builds some page tables and then compares the output with hardcoded values to ensure they were built correctly. Unfortunately the code calls things like raw_write_ttbr0_el3() or tlbiall_el3() all over the place, which are inline functions inserting a specific inline assembly instruction. Those instructions won't work when building this for an x86 host, and I also want to confirm that they were called with the expected parameters, so I need to be able to mock them out somehow.
We would probably want to have an '-I test/include' before the normal coreboot include path when building for tests so we can mock those headers out. Since the same inline functions are used across many different files we'd want to test, we'll probably want to have a common library of header mocks that can be used by multiple tests somehow.
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:
(9 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 67: Requirements
Are any of the frameworks small enough to build into a linuxboot payload so we can run unit tests as […]
In general, I don't like the idea of running unit tests on the actual device. I expect unit test to be _very_ quick, so that I can run it a lot of times during development cycle - with building and flashing on the target, this cannot be met easily. For running on actual devices, integration testing makes more sense than unit testing. So answering your second question - I don't see this as a reasonable option, even if we can use the same compiler. That being said, Cmocka is a lightweight one, SputUnit also. Could you give me more info what is the "acceptable" size for linuxboot payload - how big unit test library can be at max?
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 79: JUnit-like XML output format for Jenkins
Right, Test Anything Protocol would work as well (worst case we'd need to put in some converter). […]
Yes, I think that any machine parsable output should work. Actually there are some examples of open-source converting scripts for some frameworks. Probably I should add a comment to this requirement explaining this. BTW, out of 4 candidates below, Cmocka & GoogleTest has support for generating TAP.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 84: however this is very unlikely
Maybe expand on this statement? […]
Sounds good.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 100: GPL
Considering that cmocka (Apache) seems to be the candidate, how do we feel about the compatibility i […]
Patrick, Correct.
Julius, My understanding of the license is that unless you are not distributing binaries, you should be OK. I'm looking at this more as a "private use" category. That being said, I'm not a lawyer - do we have a contact to someone with expertise in FOSS? Talking about example - OpenVPN (https://github.com/OpenVPN/openvpn) is GPLv2, while it is using CMocka for unit testing.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 105: for obvious reasons
"because that would take an excessive amount of time"? Not sure we should talk about "obvious" in do […]
I agree.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 176: Pros
please take care of trailing whitespace
Sure. Actually checkpatch found some whitespaces, when I was creating a commit (and fixed them), wonder how this slipped away.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 195: In the same time
At the same time (also a couple of times elsewhere in the doc), see e.g. https://www.italki. […]
Sure.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 206: form
from
Sure.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 232: prefix
suffix
Tim, Will fix.
Julius, I don't agree. I would like to avoid confusion between the two files - source.c and test.c - considering that they both have very similar path (src/* -> tests/*). Usually these files are being modified at the same time, I see a possibility of mistake here. Furthermore - if we want to create a dependency and object files for both test and source modules under build/tests/, it is necessary to have a different name. Lastly, other open source projects incorporating unit testing seem to follow that approach (however they sometimes use prefix instead of suffix as in my proposal).
Julius Werner 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:
(1 comment)
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 232: prefix
Furthermore - if we want to create a dependency and object files for both test and source modules under build/tests/, it is necessary to have a different name.
I don't really follow that logic. You can (and should) install the object files in different directories (mirroring the source tree directory structure), so there should be no problem with them having the same name. We have hundreds of files having the same name in different directories throughout coreboot. It's not really an issue.
But I don't feel strongly about this, if you think it's important to name them differently we can do that. It's just a bit more to type.
Julius Werner 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:
(1 comment)
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 232: prefix
Furthermore - if we want to create a dependency and object files for both test and source modules […]
(BTW, as I think about this more I'm pretty sure we won't always be able to have the test for X.c be called exactly X-test.c. That's a good default, but we may later find that we need more than one test per file because we want to test it with different Kconfig settings. So then I may want to have both X-test-<feature>-enabled.c and X-test-<feature>-disabled.c or something like that.)
Werner Zeh 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:
(2 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 151: ouf out
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 173: works work?
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
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.
Julius Werner 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:
(2 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 190: Cmocka also include support : for mocks.
Yes, this makes sense. We need to combine preprocessor and linker tricks in order to do this. […]
Well, I think once you have unit tests there's always some work to keep them in sync when the tested code changes. I think that's normal and this shouldn't be any different.
I understand that this would be an "advanced" feature for later, just wanna point out these things I can think of so we keep them in mind when deciding on the basic design. Doesn't mean they need to be supported in the first version.
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 since I want to avoid confusion between products of normal coreboot build and unit test build of particular module.
Do you mean you don't want to have subdirectories under build/tests (so the test binary would be build/tests/gpio-test, not build/tests/lib/gpio-test)? No, you can't do that, not if you want test names to directly map to source file names. Like I said, there are hundreds of cases in coreboot where two source files in different directories have the same name (e.g. src/lib/gpio.c and src/soc/rockchip/common/gpio.c), you cannot expect source file names to be globally distinct. The normal coreboot build system also builds a subdirectory hierarchy under build/ that mirrors the one under src/ (e.g. the object for src/lib/gpio.c is build/<stage>/lib/gpio.o).
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:
(2 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.
Makes sense, wasn't clear enough here. […]
I like the simple version you propose.
My statement about "unrelated changes" has to do with a unit test that is too closely tied to the implementation of the module under test. I've seen it happen mainly when testing a state machine; the unit test checks the state, instead of checking the outputs. So it's just something to be aware of when writing tests, to avoid testing internals of the code.
https://review.coreboot.org/c/coreboot/+/39893/1/Documentation/technotes/202... PS1, Line 94: Assumption that all firmware devs know C
My point is - ideally tests should be written in C, since this is the language which all coreboot de […]
OK, then state that instead, that unit tests will be written in C because the code being tested is also written in C.
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.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39893
to look at the new patch set (#2).
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Documentation: Add proposal for firmware unit testing
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: I552d6c3373219978b8e5fd4304f993d920425431 --- A Documentation/technotes/2020-03-unit-testing-coreboot.md M Documentation/technotes/index.md 2 files changed, 340 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39893/2
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 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39893/2/Documentation/technotes/202... File Documentation/technotes/2020-03-unit-testing-coreboot.md:
https://review.coreboot.org/c/coreboot/+/39893/2/Documentation/technotes/202... PS2, Line 85: tool toolchain You use the term toolchain below, so it makes sense to use it here.
https://review.coreboot.org/c/coreboot/+/39893/2/Documentation/technotes/202... PS2, Line 89: Compiler for the host : _must_ support the same language standards as the target compiler. This needs to be the first sentence in this section, as it is a requirement. Everything else is a nice-to-have or an explanation of why it's nice to have.
https://review.coreboot.org/c/coreboot/+/39893/2/Documentation/technotes/202... PS2, Line 106: * Same language for tests and code : : Unit tests will be written in C, because coreboot code is also written in C This might be a requirement, not a desirable. The only exception I see would be using GoogleTest (C++) to test C code. But using GoogleMock (also C++) to test C code is much harder.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39893
to look at the new patch set (#3).
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Documentation: Add proposal for firmware unit testing
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: I552d6c3373219978b8e5fd4304f993d920425431 --- A Documentation/technotes/2020-03-unit-testing-coreboot.md M Documentation/technotes/index.md 2 files changed, 336 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39893/3
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 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39893/2/Documentation/technotes/202... File Documentation/technotes/2020-03-unit-testing-coreboot.md:
https://review.coreboot.org/c/coreboot/+/39893/2/Documentation/technotes/202... PS2, Line 85: tool
toolchain […]
Done
https://review.coreboot.org/c/coreboot/+/39893/2/Documentation/technotes/202... PS2, Line 89: Compiler for the host : _must_ support the same language standards as the target compiler.
This needs to be the first sentence in this section, as it is a requirement. […]
Yes, I should change the order of sentences in this paragraph. I kept the description of nice-to-have here for simplicity, no need for extra bullet under "Desirables".
https://review.coreboot.org/c/coreboot/+/39893/2/Documentation/technotes/202... PS2, Line 106: * Same language for tests and code : : Unit tests will be written in C, because coreboot code is also written in C
This might be a requirement, not a desirable. […]
Actually I was thinking exactly about this, when moving this bullet from Requirements here - don't want to have a must-have which is broken by GoogleTest immediately. But let's keep it simple and follow your suggestion. After all this _requirement_ is the main reason why we don't choose GoogleTest.
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 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... File Documentation/technotes/2020-03-unit-testing-coreboot.md:
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 135: They offer similar functionality as Unity, while at the same time don’t seem If you decide to keep this section, indent this paragraph. Otherwise, it breaks up the bulleted list.
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 140: Another one which is rather widely used, however it doesn't have good support Indent here if keeping this section
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 130: Three more frameworks should be mentioned here, however they weren't tried : within coreboot: : * [Check](https://libcheck.github.io/check/) : * [Criterion](https://github.com/Snaipe/Criterion/) : : They offer similar functionality as Unity, while at the same time don’t seem : to have similarly active communities. : : * [CUnit](http://cunit.sourceforge.net/) : : Another one which is rather widely used, however it doesn't have good support : for mocking. : : If anyone has good experience with some framework which is not listed above, it : will be highly appreciated to give a note about this to the author. In such : cases, it may be necessary to investigate such tools deeply and then reconsider : the decision. I'm not sure this section adds value any more. You could probably just reduce it to a sentence that says something like "We looked at several other test frameworks, but decided not to do a full evaluation for various reasons such as functionality, size of the developer community, or compatibility."
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 254: ├── build I know that 'build' will get listed first because of alphabetical order, but I suggest moving it to the end of the tree. 'src' comes first because the source code exits. Then we write the tests in the 'tests' tree. After we build the tests, we have the 'build' tree.
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 285: one headers
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 285: replaces replace
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 298: test tests Use plural like the "Writing new tests" section below.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39893
to look at the new patch set (#4).
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Documentation: Add proposal for firmware unit testing
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: I552d6c3373219978b8e5fd4304f993d920425431 --- A Documentation/technotes/2020-03-unit-testing-coreboot.md M Documentation/technotes/index.md 2 files changed, 320 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39893/4
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 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... File Documentation/technotes/2020-03-unit-testing-coreboot.md:
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 135: They offer similar functionality as Unity, while at the same time don’t seem
If you decide to keep this section, indent this paragraph. […]
Indeed, I haven't notice this in the generated doc. Actually I don't like the way Sphinx is generating bullet lists in html. I mean, description for a bullet can be easily mistaken for usual paragraph text. This text is also very close to the consecutive bullet, I would prefer it to stick closer to upper one.
That being said, eventually I will remove completely this paragraph, see my reply below.
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 140: Another one which is rather widely used, however it doesn't have good support
Indent here if keeping this section
As above.
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 130: Three more frameworks should be mentioned here, however they weren't tried : within coreboot: : * [Check](https://libcheck.github.io/check/) : * [Criterion](https://github.com/Snaipe/Criterion/) : : They offer similar functionality as Unity, while at the same time don’t seem : to have similarly active communities. : : * [CUnit](http://cunit.sourceforge.net/) : : Another one which is rather widely used, however it doesn't have good support : for mocking. : : If anyone has good experience with some framework which is not listed above, it : will be highly appreciated to give a note about this to the author. In such : cases, it may be necessary to investigate such tools deeply and then reconsider : the decision.
I'm not sure this section adds value any more. […]
Yes, I agree. This is a leftover from my initial goal - to start discussions within coreboot community. Will remove this paragraph and also other parts, which bring no value in having them upstreamed. Your proposal sounds good.
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 254: ├── build
I know that 'build' will get listed first because of alphabetical order, but I suggest moving it to […]
Makes sense.
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 285: replaces
replace
Done
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 285: one
headers
Done
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 298: test
tests […]
Done
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 4: Code-Review+1
(1 comment)
I would +2 but I want other reviewers' feedback before we consider this ready to merge.
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... File Documentation/technotes/2020-03-unit-testing-coreboot.md:
https://review.coreboot.org/c/coreboot/+/39893/3/Documentation/technotes/202... PS3, Line 254: ├── build
Makes sense.
* exists I said "source code exits" but I meant to type "source code exists" /facepalm
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39893 )
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39893/4/Documentation/technotes/202... File Documentation/technotes/2020-03-unit-testing-coreboot.md:
https://review.coreboot.org/c/coreboot/+/39893/4/Documentation/technotes/202... PS4, Line 39: dafault s/dafault/default/
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39893 )
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Patch Set 4: Code-Review+2
Hello build bot (Jenkins), Paul Fagerburg, Julius Werner, Jett Rink,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39893
to look at the new patch set (#5).
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Documentation: Add proposal for firmware unit testing
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: I552d6c3373219978b8e5fd4304f993d920425431 --- A Documentation/technotes/2020-03-unit-testing-coreboot.md M Documentation/technotes/index.md 2 files changed, 320 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39893/5
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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39893/4/Documentation/technotes/202... File Documentation/technotes/2020-03-unit-testing-coreboot.md:
https://review.coreboot.org/c/coreboot/+/39893/4/Documentation/technotes/202... PS4, Line 39: dafault
s/dafault/default/
Done
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 5: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39893 )
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39893 )
Change subject: Documentation: Add proposal for firmware unit testing ......................................................................
Documentation: Add proposal for firmware unit testing
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: I552d6c3373219978b8e5fd4304f993d920425431 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39893 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Julius Werner jwerner@chromium.org --- A Documentation/technotes/2020-03-unit-testing-coreboot.md M Documentation/technotes/index.md 2 files changed, 320 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Paul Fagerburg: Looks good to me, approved
diff --git a/Documentation/technotes/2020-03-unit-testing-coreboot.md b/Documentation/technotes/2020-03-unit-testing-coreboot.md new file mode 100644 index 0000000..0d1d8ec --- /dev/null +++ b/Documentation/technotes/2020-03-unit-testing-coreboot.md @@ -0,0 +1,319 @@ +# Unit testing coreboot + +## Preface +First part of this document, Introduction, comprises disambiguation for what +unit testing is and what is not. This definition will be a basis for the whole +paper. + +Next, Rationale, explains why to use unit testing and how coreboot specifically +may benefit from it. + +This is followed by evaluation of different available free C unit test +frameworks. Firstly, collection of requirements is provided. Secondly, there is +a description of a few selected candidates. Finally, requirements are applied to +candidates to see if they might be a good fit. + +Fourth part is a summary of evaluation, with proposal of unit test framework +for coreboot to be used. + +Finally, Implementation proposal paragraph touches how build system and coreboot +codebase in general should be organized, in order to support unit testing. This +comprises couple of design considerations which need to be addressed. + +## Introduction +A unit test is supposed to test a single unit of code in isolation. In C +language (in contrary to OOP) unit usually means a function. One may also +consider unit under test to be a single compilation unit which exposes some +API (set of functions). A function, talking to some external component can be +tested if this component can be mocked out. + +In other words (looking from C compilation angle), there should be no extra +dependencies (executables) required beside unit under test and test harness in +order to compile unit test binary. Test harness, beside code examining a +routines, may comprise test framework implementation. + +It is hard to apply this strict definition of unit test to firmware code in +practice, mostly due to constraints on speed of execution and size of final +executable. coreboot codebase often cannot be adjusted to be testable. Because +of this, coreboot unit testing subsystem should allow to include some additional +source object files beside unit under test. That being said, the default and +goal wherever possible, should be to isolate unit under test from other parts. + +Unit testing is not an integration testing and it doesn't replace it. First of +all, integration tests cover larger set of components and interactions between +them. Positive integration test result gives more confidence than a positive +unit test does. Furthermore, unit tests are running on the build machine, while +integration tests usually are executed on the target (or simulator). + +## Rationale +Considering above, what is the benefit of unit testing, especially keeping in +mind that coreboot is low-level firmware? Unit tests should be quick, thus may +be executed frequently during development process. It is much easier to build +and run a unit test on a build machine, than any integration test. This in turn +may be used by dev to gather extra confidence early during code development +process. Actually developer may even write unit tests earlier than the code - +see [TDD](https://en.wikipedia.org/wiki/Test-driven_development) concept. + +That being said, unit testing embedded C code is a difficult task, due to +significant amount of dependencies on underlying hardware. Mocking can handle +some hardware dependencies. However, complex mocks make the unit test +susceptible to failing and can require significant development effort. + +Writing unit tests for a code (both new and currently existing) may be favorable +for the code quality. It is not only about finding bugs, but in general - easily +testable code is a good code. + +coreboot benefits the most from testing common libraries (lib/, commonlib/, +payloads/libpayload) and coreboot infrastructure (console/, device/, security/). + +## Evaluation of unit testing frameworks + +### Requirements +Requirements for unit testing frameworks: + +* Easy to use +* Few dependencies + + Standard C library is all we should need + +* Isolation between tests +* Support for mocking +* Support for some machine parsable output +* Compiler similarity + + Compiler for the host _must_ support the same language standards as the target + compiler. Ideally the same toolchain should be used for building firmware + executables and test binaries, however the host complier will be used to build + unit tests, whereas the coreboot toolchain will be used for building the + firmware executables. For some targets, the host compiler and the target + compiler could be the same, but this is not a requirement. + +* Same language for tests and code + + Unit tests will be written in C, because coreboot code is also written in C + +### Desirables + +* Easy to integrate with build system/build tools + + Ideally JUnit-like XML output format for Jenkins + +* Popularity is a plus + + We want a larger community for a couple of reasons. Firstly, easier access to + people with knowledge and tutorials. Secondly, bug fixes for the top of tree + are more frequent and known issues are usually shorter in the pending state. + Last but not least, larger reviewer pool means better and easier upstream + improvements that we would like to submit. + +* Extra features may be a plus +* Compatible license + + This should not be a blocker, since test binaries are not distributed. + However ideally compatible with GPL. + +* IDE integration + +### Candidates +There is a lot of frameworks which allow unit testing C code +([list](https://en.wikipedia.org/wiki/List_of_unit_testing_frameworks#C) from +Wikipedia). While not all of them were evaluated, because that would take an +excessive amount of time, couple of them were selected based on the good +opinions among C devs, popularity and fitting above criteria. + +* [SputUnit](https://www.use-strict.de/sput-unit-testing/) +* [GoogleTest](https://github.com/google/googletest) +* [Cmocka](https://cmocka.org/) +* [Unity](http://www.throwtheswitch.org/unity) (CMock, Ceedling) + +We looked at several other test frameworks, but decided not to do a full evaluation +for various reasons such as functionality, size of the developer community, or +compatibility. + +### Evaluation +* [SputUnit](https://www.use-strict.de/sput-unit-testing/) + * Pros + * No dependencies, one header file to include - that’s all + * Pure C + * Very easy to use + * BSD license + * Cons + * Main repo doesn’t have support for generating JUnit XML reports for + Jenkins to consume - this feature is available only on the fork from + SputUnit called “Sput_report”. It makes it niche in a niche, so there are + some reservations whether support for this will be satisfactory + * No support for mocks + * Not too popular + * No automatic test registration +* [GoogleTest](https://github.com/google/googletest) + * Pros + * Automatic test registration + * Support for different output formats (including XML for Jenkins) + * Good support, widely used, the biggest and the most active community out + of all frameworks that were investigated + * Available as a package in the most common distributions + * Test fixtures easily available + * Well documented + * Easy to integrate with an IDE + * BSD license + * Cons + * Requires C++11 compiler + * To make most out of it (use GMock) C++ knowledge is required +* [Cmocka](https://cmocka.org/) + * Pros + * Self-contained, autonomous framework + * Pure C + * API is well documented + * Multiple output formats (including XML for Jenkins) + * Available as a package in the most common distributions + * Used in some popular open source projects (libssh, OpenVPN, Samba) + * Test fixtures available + * Support for exception handling + * Cons + * No automatic test registration + * It will require some effort to make it work from within an IDE + * Apache 2.0 license (not compatible with GPLv2) +* [Unity](http://www.throwtheswitch.org/unity) (CMock, Ceedling) + * Pros + * Pure C (Unity testing framework itself, not test runner) + * Support for different output formats (including XML for Jenkins) + * There are some (rather easy) hints how to use this from an IDE (e.g. Eclipse) + * MIT license + * Cons + * Test runner (Ceedling) is not written in C - uses Ruby + * Mocking/Exception handling functionalities are actually separate tools + * No automatic test registration + * Not too popular + +### Summary & framework proposal +After research, we propose using the Cmocka unit test framework. Cmocka fulfills +all stated evaluation criteria. It is rather easy to use, doesn’t have extra +dependencies, written fully in C, allows for tests fixtures and some popular +open source projects already are using it. Cmocka also includes support for +mocks. + +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. At the same time, it may be worth to propose improvement to Cmocka +community or simply apply some extra wrapper with demanded functionality. + +## Implementation + +### Framework as a submodule or external package +Unit test frameworks may be either compiled from source (from a git submodule +under 3rdparty/) or pre-compiled as a package. The second option seems to be +easier to maintain, while at the same time may bring some unwanted consequences +(different version across distributions, frequent changes in API). It makes sense +to initially experiment with packages and check how it works. If this will +cause any issues, then it is always possible to switch to submodule approach. + +### Integration with build system +To get the most out of unit testing framework, it should be integrated with +Jenkins automation server. Verification of all unit tests for new changes may +improve code reliability to some extent. + +### Build configuration (Kconfig) +While building unit under test object file, it is necessary to apply some +configuration (config) just like when building usual firmware. For simplicity, +there will be one default tests .config `qemu_x86_i440fx` for all unit tests. At +the same time, some tests may require running with different values of particular +config. This should be handled by adding extra header, included after config.h. +This header will comprise #undef of old CONFIG values and #define of the +required value. When unit testing will be integrated with Jenkins, it may be +preferred to use every available config for periodic builds. + +### Directory structure +Tests should be kept separate from the code, while at the same time it must be +easy to match code with test harness. + +We create new directory for test files ($(toplevel)/tests/) and mimic the +structure of src/ directory. + +Test object files (test harness, unit under tests and any additional executables +are stored under build/tests/<test_name> directory. + +Below example shows how directory structure is organized for the two test cases: +tests/lib/string-test and tests/device/i2c-test: + +```bash +├── src +│ ├── lib +│ │ ├── string.c <- unit under test +│ │ +│ ├── device +│ ├── i2c.c +│ +├── tests +│ ├── include +│ │ ├── mocks <- mock headers, which replace original headers +│ │ +│ ├── Makefile.inc <- top Makefile for unit tests subsystem +│ ├── lib +│ │ ├── Makefile.inc +│ │ ├── string-test.c <- test code for src/lib/string.c +│ │ │ +│ ├── device +│ │ ├── Makefile.inc +│ ├── i2c-test.c +│ +├── build +│ ├── tests <-all test-related executables + ├── config.h <- default config used for tests builds + ├── lib + │ ├── string-test <- all string-test executables + │ │ ├── run <- final test binary + │ │ ├── tests <- all test harness executables + │ │ ├── lib + │ │ ├── string-test.o <-test harness executable + │ │ ├── src <- unit under test and other src executables + │ │ ├── lib + │ │ ├── string.o <- unit under test executable + ├── device + ├── i2c-test + ├── run + ├── tests + │ ├── device + │ ├── i2c-test.o + ├── src + ├── device + ├── i2c.o +``` + +### Adding new tests +For purpose of this description, let's assume that we want to add a new unit test +for src/device/i2c.c module. Since this module is rather simple, it will be enough +to have only one test module. + +Firstly (assuming there is no tests/device/Makefile.inc file) we need to create +Makefile.inc in main unit test module directory. Inside this Makefile.inc, one +need to register new test and can specify multiple different attributes for it. + +```bash +# Register new test, by adding its name to tests variable +tests-y += i2c-test + +# All attributes are defined by <test_name>-<attribute> variables +# <test_name>-srcs is used to register all input files (test harness, unit under +# test and others) for this particular test. Remember to add relative paths. +i2c-test-srcs += tests/device/i2c-test.c +i2c-test-srcs += src/device/i2c.c + +# We can define extra cflags for this particular test +i2c-test-cflags += -DSOME_DEFINE=1 + +# For mocking out external dependencies (functions which cannot be resolved by +# linker), it is possible to register a mock function. To register new mock, it +# is enough to add function-to-be-mocked name to <test_name>-mocks variable. +i2c-test-mocks += platform_i2c_transfer + +# Similar to coreboot concept, unit tests also runs in the context of stages. +# By default all unit tests are compiled to be ramstage executables. If one want +# to overwrite this setting, there is <test_name>-stage variable available. +i2c-test-stage:= bootblock +``` + +### Writing new tests +Full description of how to write unit tests and Cmocka API description is out of +the scope of this document. There are other documents related to this +[Cmocka API](https://api.cmocka.org/) and +[Mocks](https://lwn.net/Articles/558106/). diff --git a/Documentation/technotes/index.md b/Documentation/technotes/index.md index 7c231fc..5367e69 100644 --- a/Documentation/technotes/index.md +++ b/Documentation/technotes/index.md @@ -2,3 +2,4 @@
* [Dealing with Untrusted Input in SMM](2017-02-dealing-with-untrusted-input-in-smm.md) * [Rebuilding coreboot image generation](2015-11-rebuilding-coreboot-image-generation.md) +* [Unit testing coreboot](2020-03-unit-testing-coreboot.md)
-
Jett Rink (Code Review) -
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Name of user not set (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Fagerburg (Code Review)
-
Tim Wawrzynczak (Code Review)
-
Werner Zeh (Code Review)