Name of user not set #1002873 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/c/coreboot/+/40538/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40538/1//COMMIT_MSG@20 PS1, Line 20: make unit-tests
Here's what I get: […]
Thanks for trying to test! There was an issue with config generation, I need to first construct config from miniconfig (olddefconfig) and then generate autoheader (silentoldconfig). There was an issue with cleaning artifacts in the environment, that's why I haven't caught it earlier.This problem is also fixed in v3.
https://review.coreboot.org/c/coreboot/+/40538/1//COMMIT_MSG@13 PS1, Line 13: In order to run this example, one need to install (beside general : coreboot dependencies) cmocka package: : sudo apt-get install -y libcmocka-dev : sudo emerge dev-util/cmocka : yum install libcmocka-devel : : After invoking: : make unit-tests : report from unit test will be created and shown on the screen
Change this to a TEST= line, such as […]
OK.
https://review.coreboot.org/c/coreboot/+/40538/1//COMMIT_MSG@23 PS1, Line 23: This test harness definitely isn't complete, as it only checks for some : functions within module. Purpose of this code is only an example and a : starting point for a discussion about implementation. :
You don't need to explain this; the first paragraph already says this is a basic example.
Correct.
https://review.coreboot.org/c/coreboot/+/40538/1/tests/include/mocks/assert.... File tests/include/mocks/assert.h:
https://review.coreboot.org/c/coreboot/+/40538/1/tests/include/mocks/assert.... PS1, Line 21: Fix vendorcode headers
vendorcode is code that we tend not to touch because it comes from the outside (although we lifted t […]
Frankly speaking - I just took src/include/assert.h and wanted to keep it unchanged as much as possible. After all, my intention was to mock dead_code() for string-test purpose. But as you may see from other comments from Julius, it is not necessarily the best idea. In future, when there will be an example which really requires mocking assert(), I will probably go with option to mock hlt() and won't copy src/include/assert.h anymore.
https://review.coreboot.org/c/coreboot/+/40538/1/tests/include/mocks/assert.... PS1, Line 33: hlt(); \
I don't think you really need to mock out this header, I think it should be enough to mock out hlt() […]
Yes, it seems to be better approach considering that I won't mock dead_code() - which wasn't the best idea after all. See another comment.
Actually I won't mock hlt() either for this particular example. Let's add it in future, when writing tests for some module which can really assert().
https://review.coreboot.org/c/coreboot/+/40538/1/tests/include/mocks/assert.... PS1, Line 64: * called.
Can you provide an example?
Not sure if I get you correct. You are asking for an example for expect_assert_failure()? I provided one in this example and it works, but let's add a new example somewhere in the future, were assert() actually should be tested.
I think dead_code() should be independent of unit tests (there is no need to write a test to "check" whether a dead_code() works, because it is already a build time check) but shouldn't cause problems for it either. When a dead_code() triggers that usually means that you're building with an invalid combination of Kconfig options. In that case I think it is correct that that fails to build even for unit tests -- we should run our tests with valid configurations.
I mocked this header mainly for the purpose of showing how.. we can mock headers for unit testing. But I agree this is probably not the best idea to rework dead_code() which shouldn't be called anyway with proper configuration.
Let me add a header mocking (at least I'm convinced we can do this) example in future, when this base support makes it to the tree. I will chose a module which is really calling assert() and will benefit from mocking hlt().
https://review.coreboot.org/c/coreboot/+/40538/1/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/1/tests/lib/string-test.c@22 PS1, Line 22: strlen
how can we ensure that we're not just testing libc's implementation here?
Hmm.. when talking about experiment - I'm sure that this is coreboot's one, because altering src/lib/string.c alters the results of test. When talking about how it works - we are simply linking against src/ module object first. libc definitions are weak. Do you think, that I should add some extra measures to confirm this? If so - do you have any idea? I don't think that running with -nostdlib is not an option in this case, we need to easily produce executables for our host.
That being said - I see a potential problem, that you may for example not add src/lib/string.c to the $(test)-srcs and end up with properly compiling executable which is testing.. libc not coreboot lib. However such an error will be probably caught rather easily at the beginning of development, when you always try to break a test to see if this is testing what you are working on.
https://review.coreboot.org/c/coreboot/+/40538/1/tests/lib/string-test.c@30 PS1, Line 30: #ifdef __RAMSTAGE__
The "which stage are we running in" concept is central to coreboot, so I think you should be adding […]
I'am adding support for $(test)-stage in v3. Will remove this #ifdef from here and set string-test-stage:=ramstage in Makefile.inc for reference for others (even though it is default stage).