Name of user not set #1002873 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
tests: Add lib/string-test test case
This commit's purpose is to show basic example of how unit testing may be applied for coreboot project. It adds test harness for lib/string.c module.
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
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.
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: Ibf5554d1e99a393721a66bdd35af0122c2e412c4 --- A tests/include/mocks/assert.h A tests/lib/Makefile.inc A tests/lib/string-test.c 3 files changed, 152 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40538/1
diff --git a/tests/include/mocks/assert.h b/tests/include/mocks/assert.h new file mode 100644 index 0000000..0b0499b --- /dev/null +++ b/tests/include/mocks/assert.h @@ -0,0 +1,83 @@ +/* + * This file is part of the coreboot project. + * + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __ASSERT_H__ +#define __ASSERT_H__ + +#include <arch/hlt.h> +#include <console/console.h> + +/* TODO: Fix vendorcode headers to not define macros coreboot uses or to be more + properly isolated. */ +#ifdef ASSERT +#undef ASSERT +#endif + +/* GCC and CAR versions */ +#define ASSERT(x) { \ + if (!(x)) { \ + printk(BIOS_EMERG, "ASSERTION ERROR: file '%s'" \ + ", line %d\n", __FILE__, __LINE__); \ + if (CONFIG(FATAL_ASSERTS)) \ + hlt(); \ + } \ +} + +#define ASSERT_MSG(x, msg) { \ + if (!(x)) { \ + printk(BIOS_EMERG, "ASSERTION ERROR: file '%s'" \ + ", line %d\n", __FILE__, __LINE__); \ + printk(BIOS_EMERG, "%s", msg); \ + if (CONFIG(FATAL_ASSERTS)) \ + hlt(); \ + } \ +} + +/* Compile-out BUG() macro, since checkpatch is not happy with it, + * to be revised later. + */ +/* #define BUG() { \ + printk(BIOS_EMERG, "ERROR: BUG ENCOUNTERED at file '%s'"\ + ", line %d\n", __FILE__, __LINE__); \ + if (CONFIG(FATAL_ASSERTS)) \ + hlt(); \ +} */ + +#define assert(statement) ASSERT(statement) + +/* + * Original dead_code() from src/include/assert.h breaks compilation when + * non-intended code is compiled in. This is not suitable for unit testing, + * thus we are changing this macro to be run-time assert. mock_assert() allows + * Cmocka to expect such assert and pass test, when dead_code() is correctly + * called. + */ + +extern void mock_assert(const int result, const char * const expression, + const char * const file, const int line); + +#define dead_code() \ + mock_assert(0, "dead_code", __FILE__, __LINE__) + +#undef assert +#define assert(expression) \ + mock_assert((int)(expression), #expression, __FILE__, __LINE__) + +/* This can be used in the context of an expression of type 'type'. */ +#define dead_code_t(type) ({ \ + dead_code(); \ + *(type *)(uintptr_t)0; \ +}) + +#endif // __ASSERT_H__ diff --git a/tests/lib/Makefile.inc b/tests/lib/Makefile.inc new file mode 100644 index 0000000..5f73446 --- /dev/null +++ b/tests/lib/Makefile.inc @@ -0,0 +1,20 @@ +## +## This file is part of the coreboot project. +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; version 2 of the License. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +# object filest should be under build/tests/ build/test/src/ build/test/run/ +# two examples - first should be simply string.c, second should use -wrap + +tests += string-test + +string-test-srcs += tests/lib/string-test.c +string-test-srcs += src/lib/string.c diff --git a/tests/lib/string-test.c b/tests/lib/string-test.c new file mode 100644 index 0000000..3ce0563 --- /dev/null +++ b/tests/lib/string-test.c @@ -0,0 +1,49 @@ +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> + +#include <string.h> + +struct strings_t { + char *str; + size_t size; +} strings[] = { + {"coreboot", 8}, + {"is\0very", 2}, /* strlen should be 2 because of the embedded \0 */ + {"nice\n", 5} +}; + +static void test_strlen_strings(void **state) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(strings); i++) + assert_int_equal(strings[i].size, strlen(strings[i].str)); +} + +/* Since strdup() requires malloc() it may only be invoked during ramstage + * phase. Expect dead_code/assert invocation otherwise. + */ +static void test_strdup(void **state) +{ +#ifdef __RAMSTAGE__ + char str[] = "Hello coreboot\n"; + char *duplicate; + + duplicate = strdup(str); + assert_int_equal(0, memcmp(str, duplicate, strlen(str))); +#else + expect_assert_failure(strdup(NULL)); +#endif +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_strlen_strings), + cmocka_unit_test(test_strdup), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 1:
(3 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:
$ make unit-tests * * Restart config... * * * General setup * Local version string (LOCALVERSION) [] (NEW)
and it's waiting for me to answer all the config questions. Was there a missing step?
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 TEST=Install cmocka via appropriate command: * sudo apt-get install -y libcmocka-dev * sudo emerge dev-util/cmocka * yum install libcmocka-devel Build and run unit tests via `make unit-tests` Check the output to see that tests passed.
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.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 1:
(2 comments)
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 that restriction for amd agesa given that it isn't updated anymore), so I guess better isolation is the way to go.
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?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 1:
(3 comments)
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 33: hlt(); \ I don't think you really need to mock out this header, I think it should be enough to mock out hlt().
https://review.coreboot.org/c/coreboot/+/40538/1/tests/include/mocks/assert.... PS1, Line 64: * called. Can you provide an example? 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.
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@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 that to the Makefile framework. I would suggest adding a new attribute $(test)-stage to decide that. It can default to ramstage when not provided (I think that makes most sense, it's generally our most feature-rich stage). Then it should automatically define the right macro like __RAMSTAGE__ to make sure the <rules.h> stuff works right.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40538
to look at the new patch set (#2).
Change subject: tests: Add lib/string-test test case ......................................................................
tests: Add lib/string-test test case
This commit's purpose is to show basic example of how unit testing may be applied for coreboot project. It adds test harness for lib/string.c module.
TEST=Install cmocka via appropriate command: sudo apt-get install -y libcmocka-dev sudo emerge dev-util/cmocka yum install libcmocka-devel * Build and run unit tests via `make unit-tests` * Check the output to see that tests passed.
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: Ibf5554d1e99a393721a66bdd35af0122c2e412c4 --- A tests/lib/Makefile.inc A tests/lib/string-test.c 2 files changed, 66 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40538/2
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).
Paul Fagerburg 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@25 PS2, Line 25: /* Since strdup() requires malloc() it may only be invoked during ramstage : * phase. Expect dead_code/assert invocation otherwise. : */ Since unit tests are intended to run on the host, is this comment relevant?
Paul Fagerburg 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40538/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40538/2//COMMIT_MSG@9 PS2, Line 9: This commit's purpose is to show basic example of how unit testing may : be applied for coreboot project. It adds test harness for lib/string.c : module. Show a basic example of how unit testing can be applied for the coreboot project. Add a test harness for lib/string.c module.
Julius Werner 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:
(3 comments)
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 64: * called.
Can you provide an example? […]
Sorry, I just wrote these comments in the order I read your code. I mean an example of where dead_code() breaks compilation. I see now that your issue was with strdup() but I think providing the -stage attribute was the more appropriate solution to that, so this is no longer relevant.
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/Makefile.inc File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/Makefile.inc@21 PS2, Line 21: string-test-stage:= ramstage Actually, I'd rather you leave this out so that the reference to others is that you don't need to provide it unless you care about the stage explicitly (because people will just copy&paste this).
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@34 PS2, Line 34: assert_int_equal(0, memcmp(str, duplicate, strlen(str))); Cmocka seems to have an assert_string_equal().
Paul Fagerburg 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:
(1 comment)
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
Thanks for trying to test! There was an issue with config generation, I need to first construct conf […]
After cherry-picking the latest patchset, I am able to build and run the unit tests.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40538
to look at the new patch set (#3).
Change subject: tests: Add lib/string-test test case ......................................................................
tests: Add lib/string-test test case
Show a basic example of how unit testing can be applied for the coreboot project. Add a test harness for lib/string.c module.
TEST=Install cmocka via appropriate command: sudo apt-get install -y libcmocka-dev sudo emerge dev-util/cmocka yum install libcmocka-devel * Build and run unit tests via `make unit-tests` * Check the output to see that tests passed.
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: Ibf5554d1e99a393721a66bdd35af0122c2e412c4 --- A tests/lib/Makefile.inc A tests/lib/string-test.c 2 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40538/3
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 3:
(6 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
After cherry-picking the latest patchset, I am able to build and run the unit tests.
Great, thanks!
https://review.coreboot.org/c/coreboot/+/40538/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40538/2//COMMIT_MSG@9 PS2, Line 9: This commit's purpose is to show basic example of how unit testing may : be applied for coreboot project. It adds test harness for lib/string.c : module.
Show a basic example of how unit testing can be applied for the coreboot project. […]
Done
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 64: * called.
Sorry, I just wrote these comments in the order I read your code. […]
Done
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/Makefile.inc File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/Makefile.inc@21 PS2, Line 21: string-test-stage:= ramstage
Actually, I'd rather you leave this out so that the reference to others is that you don't need to pr […]
OK.
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@25 PS2, Line 25: /* Since strdup() requires malloc() it may only be invoked during ramstage : * phase. Expect dead_code/assert invocation otherwise. : */
Since unit tests are intended to run on the host, is this comment relevant?
This is actually the excerpt from uut - src/lib/string.c. This is relevant in the sense that I wanted to show example of Cmocka assertion handling (dropped this idea already, see other comments). Actually we have idea of stages now, so this comment is a leftover and I should remove it.
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@34 PS2, Line 34: assert_int_equal(0, memcmp(str, duplicate, strlen(str)));
Cmocka seems to have an assert_string_equal().
Yes, but the trick is that assert_string_equal() is using.. strcmp, which in case of our binary will invoke coreboot implementation. We will actually test unit with unit under test.
At the same time, I'm using strlen() here, but this function is already tested with test_strlen_strings() at this time. One may say that it breaks the idea of test isolation - what do you think? I may change this to constant value, to be on the safe side.
I will add a comment at the top of this file, since this is rather rare case when you shouldn't use functions used "everyday".
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@34 PS2, Line 34: assert_int_equal(0, memcmp(str, duplicate, strlen(str)));
Yes, but the trick is that assert_string_equal() is using.. […]
In that case, add a comment in this function that you're using assert_int_equal and memcmp instead of assert_string_equal because you don't want to use strcmp.
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 3: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@25 PS2, Line 25: /* Since strdup() requires malloc() it may only be invoked during ramstage : * phase. Expect dead_code/assert invocation otherwise. : */
This is actually the excerpt from uut - src/lib/string.c. […]
nit: not actually removed?
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@34 PS2, Line 34: assert_int_equal(0, memcmp(str, duplicate, strlen(str)));
In that case, add a comment in this function that you're using assert_int_equal and memcmp instead o […]
Fair enough about assert_string_equal(). For strlen() you could use __builtin_strlen() (since it's a constant you could also just use (sizeof(str) - 1) in this case, but I think providing an example for using __builtin for this might be helpful). Guess we'll have to be a bit careful when testing libc stuff in general (thankfully, there's not too much of it in coreboot anyway).
https://review.coreboot.org/c/coreboot/+/40538/3/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/3/tests/lib/string-test.c@13 PS3, Line 13: */ Maybe mention that __builtin_xxx can be used for many of the most simple mem*()/str*() functions if you want a non-coreboot one.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Paul Fagerburg, Jett Rink,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40538
to look at the new patch set (#4).
Change subject: tests: Add lib/string-test test case ......................................................................
tests: Add lib/string-test test case
Show a basic example of how unit testing can be applied for the coreboot project. Add a test harness for lib/string.c module.
TEST=Install cmocka via appropriate command: sudo apt-get install -y libcmocka-dev sudo emerge dev-util/cmocka yum install libcmocka-devel * Build and run unit tests via `make unit-tests` * Check the output to see that tests passed.
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: Ibf5554d1e99a393721a66bdd35af0122c2e412c4 --- A tests/lib/Makefile.inc A tests/lib/string-test.c 2 files changed, 74 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40538/4
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 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@25 PS2, Line 25: /* Since strdup() requires malloc() it may only be invoked during ramstage : * phase. Expect dead_code/assert invocation otherwise. : */
nit: not actually removed?
Indeed, sorry. Some operational error here.
https://review.coreboot.org/c/coreboot/+/40538/2/tests/lib/string-test.c@34 PS2, Line 34: assert_int_equal(0, memcmp(str, duplicate, strlen(str)));
In that case, add a comment in this function that you're using assert_int_equal and memcmp instead of assert_string_equal because you don't want to use strcmp.
I've added a comment above this call. Makes sense, since this code will be used as an example, it's worth to mention that Cmocka is equipped with string-comparison functions.
Fair enough about assert_string_equal(). For strlen() you could use __builtin_strlen() (since it's a constant you could also just use (sizeof(str) - 1) in this case, but I think providing an example for using __builtin for this might be helpful). Guess we'll have to be a bit careful when testing libc stuff in general (thankfully, there's not too much of it in coreboot anyway).
Will use __builtin_strlen, good idea.
https://review.coreboot.org/c/coreboot/+/40538/3/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/3/tests/lib/string-test.c@13 PS3, Line 13: */
Maybe mention that __builtin_xxx can be used for many of the most simple mem*()/str*() functions if […]
Done
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 4: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40538/4/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/4/tests/lib/string-test.c@40 PS4, Line 40: /* There is a more suitable Cmocka's function 'assert_string_equal()', but it The two allowed coreboot comment styles are
/* short form, note the lack of an asterisk on the continuation lines. */
/* * Long form with * a blank line both * above and below. */
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 4:
(1 comment)
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
Hmm.. […]
I wasn't aware that libc marks its symbols weak (and I'm not sure that this is the case everywhere). One option to go at this in a structured way could be to rename symbols in string.o during linking or with objcopy, but that gets tedious real quick (you have to carry around a full list of that kind of symbols).
For testing that it picks up the right symbols (and that libc really takes the backseat), we could do as follows:
1. Add some non-sense implementation of a libc function (e.g. a strlen that always returns 42) to tests/, 2. Add a test that uses this function and expects it to return 42 no matter the (non-42 char length) input.
That way we have a signal indicating that the tests are looking at the right thing in the normal case, too, right? If we see that there are major libc implementations that fail this, we can still reconsider our options.
That's all not for this commit, but please put it on your backlog.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Paul Fagerburg, Julius Werner, Jett Rink,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40538
to look at the new patch set (#5).
Change subject: tests: Add lib/string-test test case ......................................................................
tests: Add lib/string-test test case
Show a basic example of how unit testing can be applied for the coreboot project. Add a test harness for lib/string.c module.
TEST=Install cmocka via appropriate command: sudo apt-get install -y libcmocka-dev sudo emerge dev-util/cmocka yum install libcmocka-devel * Build and run unit tests via `make unit-tests` * Check the output to see that tests passed.
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: Ibf5554d1e99a393721a66bdd35af0122c2e412c4 --- A tests/lib/Makefile.inc A tests/lib/string-test.c 2 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40538/5
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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40538/4/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/40538/4/tests/lib/string-test.c@40 PS4, Line 40: /* There is a more suitable Cmocka's function 'assert_string_equal()', but it
The two allowed coreboot comment styles are […]
You are right.. I've fixed this also in patch 4/4.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
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
I wasn't aware that libc marks its symbols weak (and I'm not sure that this is the case everywhere). […]
I don't think the libc definitions actually are weak. It's just normal linker behavior to first look in the objects directly specified on the command line and then look in libraries for any symbols it didn't find there yet (and it doesn't complain about duplicates in libraries). The vboot test framework is also based on that principle.
I think as long as someone once (manually) made sure that this works as intended, we don't need to check anything extra in to keep retesting it, or build a lot of extra support in the framework to support symbol renaming. We don't have that many libc functions anyway, I don't think this will be a big problem in the long run.
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 5:
(1 comment)
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
I wasn't aware that libc marks its symbols weak (and I'm not sure that this is the case everywhere).
Actually, I mixed up things a little bit here. _weak_ attribute won't affect the order in which linker looks for symbols resolution. And as you said - this is not the case everywhere for libc. The real thing which happens, is that linker (all linkers I have tried so far, however I don't think that this is obligatory/forced in any spec) looks for symbols firstly in the files provided explicitly as an input. Only if there are still some unresolved ones, linker will search in other libraries (like libc). Thus, when providing src/lib/string.c on command line for gcc, we are (should be) safe.
One option to go at this in a structured way could be to rename symbols in string.o during linking or with objcopy, but that gets tedious real quick (you have to carry around a full list of that kind of symbols).
Yes, this is the "most proper" option I believe. But something hard to implement and maintain.
For testing that it picks up the right symbols (and that libc really takes the backseat), we could do as follows:
- Add some non-sense implementation of a libc function (e.g. a strlen that always returns 42) to tests/,
- Add a test that uses this function and expects it to return 42 no matter the (non-42 char length) input.
That way we have a signal indicating that the tests are looking at the right thing in the normal case, too, right? If we see that there are major libc implementations that fail this, we can still reconsider our options.
Every test is a separate binary (especially input files are listed explicitly for each test) and tests are isolated from each other. Even if we will create one test, which is really checking that we are linking against correct implementation, this doesn't necessarily mean, that other binaries are built the way we really want it to.
We would need to link such a fake function against every test touching coreboot's libc implementation. Of course, we cannot use strlen() or any other function defined in coreboot inside unit under test. As a consequence - we wouldn't really check which one is invoked. In other words - vicious circle...
That's all not for this commit, but please put it on your backlog.
Sounds reasonable. Let me think about some other ideas, how we may tackle this problem.
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 5:
(1 comment)
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
I wasn't aware that libc marks its symbols weak (and I'm not sure that this is the case everywhere […]
Oh, looks like some race condition, I haven't seen your comment earlier Julius.
Yes, my first idea was that when you are writing tests, you really try to break something. I mean, at least I will be very suspicious if everything work out of the box for me... :)
That being said, it is enough to simply not include uut to srcs and we won't be aware that something bad happened. We need to be very careful (in general, not only because of this issue) when writing tests for coreboot libc implementation.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
tests: Add lib/string-test test case
Show a basic example of how unit testing can be applied for the coreboot project. Add a test harness for lib/string.c module.
TEST=Install cmocka via appropriate command: sudo apt-get install -y libcmocka-dev sudo emerge dev-util/cmocka yum install libcmocka-devel * Build and run unit tests via `make unit-tests` * Check the output to see that tests passed.
Signed-off-by: Jan Dabros jsd@semihalf.com Change-Id: Ibf5554d1e99a393721a66bdd35af0122c2e412c4 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40538 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org --- A tests/lib/Makefile.inc A tests/lib/string-test.c 2 files changed, 73 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/tests/lib/Makefile.inc b/tests/lib/Makefile.inc new file mode 100644 index 0000000..86ac323 --- /dev/null +++ b/tests/lib/Makefile.inc @@ -0,0 +1,20 @@ +## +## This file is part of the coreboot project. +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; version 2 of the License. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +# object filest should be under build/tests/ build/test/src/ build/test/run/ +# two examples - first should be simply string.c, second should use -wrap + +tests-y += string-test + +string-test-srcs += tests/lib/string-test.c +string-test-srcs += src/lib/string.c diff --git a/tests/lib/string-test.c b/tests/lib/string-test.c new file mode 100644 index 0000000..210aaba --- /dev/null +++ b/tests/lib/string-test.c @@ -0,0 +1,53 @@ +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> + +#include <string.h> + +/* + * Important note: In every particular test, don't use any string-related + * functions other than function under test. We are linking against + * src/lib/string.c not the standard library. This is important for proper test + * isolation. One can use __builtin_xxx for many of the most simple str*() + * functions, when non-coreboot one is required. + */ + +struct strings_t { + char *str; + size_t size; +} strings[] = { + {"coreboot", 8}, + {"is\0very", 2}, /* strlen should be 2 because of the embedded \0 */ + {"nice\n", 5} +}; + +static void test_strlen_strings(void **state) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(strings); i++) + assert_int_equal(strings[i].size, strlen(strings[i].str)); +} + +static void test_strdup(void **state) +{ + char str[] = "Hello coreboot\n"; + char *duplicate; + + duplicate = strdup(str); + + /* There is a more suitable Cmocka's function 'assert_string_equal()', but it + is using strcmp() internally. */ + assert_int_equal(0, memcmp(str, duplicate, __builtin_strlen(str))); +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_strlen_strings), + cmocka_unit_test(test_strdup), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40538/6/tests/lib/string-test.c File tests/lib/string-test.c:
PS6: You forgot the license header here, could you please upload another patch to add it?
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40538 )
Change subject: tests: Add lib/string-test test case ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40538/6/tests/lib/string-test.c File tests/lib/string-test.c:
PS6:
You forgot the license header here, could you please upload another patch to add it?
Thanks for a note. I've sent a patch for review - https://review.coreboot.org/c/coreboot/+/41088. Actually I updated also headers for other files under tests/ to the new format.
-
Jan Dabros (Code Review) -
Jett Rink (Code Review)
-
Julius Werner (Code Review)
-
Name of user not set (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Fagerburg (Code Review)