Name of user not set #1002873 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39894 )
Change subject: tests: Add build subsystem and example for unit tests ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc@19 PS1, Line 19: build
$(obj) probably?
Sure.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc@26 PS1, Line 26: # Minimal required set for simplicity sake
We'll likely need to reuse the normal coreboot CFLAGS because some of them modify behavior (e.g. […]
Understand. This CFLAGS variable is simply exported to lower-level Makefiles? I wonder if we may see some issues when applying this CFLAGS to HOSTCC.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc@50 PS1, Line 50: $$(addprefix $$(SRC_DIR),$$(subst -test,.c,$$*))
This is an interesting approach but I don't think it will be flexible enough to support everything we'll want to do.
This problem, which you mentioned here, is very important. Namely - what we are defining as a unit test. I hope my document is addressing this to some extent. Thanks for bringing this here!
I guess strict "unit testing" would only cover one function
I don't think that (even strict) unit testing should cover only one function. Using static functions and all functions within module is perfectly fine for testing the interface, which that module is exposing.
but I think ultimately we'll want to be able to cover larger components consisting of multiple files as well. Otherwise, we'll just force weird constraints on ourselves about what needs to be put in the same file in the main sources
If we want to be strict, then I would say there should be _no more than the two_ obj files linked against each other - code module and unit test module. If this constraint cannot be met, it means that code is not well designed (or cannot be tested). Of course, we can adjust tests so that they will pass, but in my opinion this should go another way around - code should be modified to be testable and to pass all the tests. I know that probably it's sounds a little bit like a unreal and theory-only approach, but this should be our goal and this still may be very useful.
Can you think of and point me to the example, where we would need to incorporate multiple files in the unit test binary? Sounds rather like an integration testing, not unit testing. I mean - maybe such a big framework isn't meant to be covered by unit tests? (I have no illusions that we can cover whole coreboot codebase with them.) Maybe we should mock-out some external dependencies? Or finally, maybe we should adjust code itself?
(and may create big headaches about how to port the tests whenever some refactoring splits a function in two).
When talking about splitting a function in two - does this happen frequently for functions available externally?
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h File tests/include/config.h:
PS1:
What's our overall plan with Kconfig for the unit tests? Do we have one yet? I assume for most tests […]
?Good question. I'm proposing to use qemu-x86 .config for running unit tests and I believe this should work for 90% cases.. At the same time, I would like to hear community opinion about what are the needs. While the examples you are pointing should be addressed simply by allowing user to adjust .config (right?), the thing is what to do with test automation (that is Jenkins running all unit tests). Should we do this with every known-good config? Or even some extra combinations? Definitely configs should be auto-generated - in this example I just wanted to keep change as simple as possible.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h@7 PS1, Line 7: CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS
we'd need to extend util/lint/lint-stable-008-kconfig to not care about this file for this commit to […]
I simply disabled util/lint/lint-stable-008-kconfig when committing this. Eventually (see comment above),we should autogenerate this file, when running unit test target.