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:
(10 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
Sure.
Done
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc@26 PS1, Line 26: # Minimal required set for simplicity sake
If this is included as part of coreboot's toplevel Makefile then yes, you should be able to just ref […]
Yes, I agree that going forward, we will need to adjust these flags. Considering this, I've added a support for defining per-test cflags. This should be flexible enough for our needs. In the same time in v2, I've added flags which you mentioned here.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc@37 PS1, Line 37: TEST_LDFLAGS = -lcmocka
Some syntactic sugar to handle this would definitely be nice... […]
In v2 I've added completely new unit test for src/device/i2c.c. It is showing how the __wrap feature works. From the dev point of view, it is enough to add test-file-mocks += in Makefile.inc, generic Makefile is handling this.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/Makefile.inc@50 PS1, Line 50: $$(addprefix $$(SRC_DIR),$$(subst -test,.c,$$*))
Yes, I agree on using this to declare the mock functions too, that would probably work well. […]
This thread is actually a basis for whole v2 rework. Solution is much more flexible than initial one. Hopefully I've included all good proposals from you. In the same time, new approach is easily extensible, so adding new attributes should be straightforward. Resolving this thread for now.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h File tests/include/config.h:
PS1:
No, I definitely didn't mean that users should manually adjust .config when running certain tests. […]
In v2 I've added automatic generation of config for all tests. By default, qemu-x86 .config is used. None of the tests' examples I've wrote, has support for the feature which we are discussing here. Let's keep this for future enhancements - hopefully this is acceptable. That being said, I don't think that embedding support for building different configs via Makefile targets will be easy to do. Even with some extra logic to handle this, we will face issues with parallel execution of MAKE - Kconfig targets are invoked by recursion to SUB-MAKE. If you have any thoughts how this may work (multiple targets each building its own config), I will be glad to try. In my opinion, we should try adding an extra header to the include path, right after kconfig.h (which in turn includes config.h). Inside this header, one should #undef particular config and add define with new required value. This may be done inside Makefile, with some echo's, so that dev will only need to define ```test-configs+= CONFIG_SOME_FEATURE=value``` This design will assume, that there will be a separate test created per-config. However one test file will be used - I believe this will be cleaner solution.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h@7 PS1, Line 7: CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS
I simply disabled util/lint/lint-stable-008-kconfig when committing this. […]
This is no longer an issue in v2 - config is autogenerated there.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h@100 PS1, Line 100: #define CONFIG_SPI_FLASH_DONT_INCLUDE_ALL_DRIVERS 0
'DONT' may be misspelled - perhaps 'DON'T'?
This is no longer an issue in v2 - config is autogenerated there.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h@255 PS1, Line 255: #define CONFIG_PXE_NO_PROMT 0
'PROMT' may be misspelled - perhaps 'PROMPT'?
This is no longer an issue in v2 - config is autogenerated there.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/39894/1/tests/lib/string-test.c@13 PS1, Line 13: {"is\0very", 2},
Initially, I planned that this commit won't be merged into the tree, and be only an example (actuall […]
I kept this test file in v2, however added special comment as you proposed.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/lib/string-test.c@21 PS1, Line 21: /* Below will fail the test */ : size = strlen(NULL);
I think that's probably okay. […]
I've added an example of handling dead_code() using mock_assert() in mocked assert.h header. This is working good, Cmocka is passing such test with expect_assert_failure(). I also tried assert() - it is also working correctly. In my example (v2), I removed test_strlen_null, since this is actually not acceptable for merging to upstream.