Hello Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41045
to review the following change.
Change subject: tests: Add test.h default header and fix --gc-sections ......................................................................
tests: Add test.h default header and fix --gc-sections
<cmocka.h> requires a few standard headers to be explicitly included before itself or it will throw compilation errors. Having to always include these headers in the right order in every test is cumbersome. Instead, this patch encapsulates the problem in a new <test.h> header that is force-included from the Makefile command line into every test, so that tests only need to worry about non-framework-related headers they may want to include.
Also fix --gc-sections in the test framework which needs to be passed for linking, not for compiling.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I4284d74c8673708e21a5266eb42f7b9ae19a1b12 --- M tests/Makefile.inc M tests/device/i2c-test.c A tests/include/test.h M tests/lib/string-test.c 4 files changed, 25 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/41045/1
diff --git a/tests/Makefile.inc b/tests/Makefile.inc index 82f724e..d5c6575 100644 --- a/tests/Makefile.inc +++ b/tests/Makefile.inc @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only # This file is part of the coreboot project.
+testsrc = $(top)/tests testobj = $(obj)/tests
TEST_DEFAULT_CONFIG = $(top)/configs/config.emulation_qemu_x86_i440fx @@ -11,12 +12,12 @@ TEST_KCONFIG_SPLITCONFIG := $(testobj)/config TEST_KCONFIG_TRISTATE := $(testobj)/tristate.conf
-TEST_CFLAGS = -include$(src)/include/kconfig.h \ - -include$(src)/commonlib/bsd/include/commonlib/bsd/compiler.h \ - -include $(src)/include/rules.h \ +TEST_CFLAGS = -include $(src)/include/kconfig.h \ + -include $(src)/commonlib/bsd/include/commonlib/bsd/compiler.h \ + -include $(src)/include/rules.h -include $(testsrc)/include/test.h
# Include generic test mock headers, before original ones -TEST_CFLAGS += -Itests/include/mocks +TEST_CFLAGS += -I$(testsrc)/include/mocks
TEST_CFLAGS += -I$(src)/include -I$(src)/commonlib/include \ -I$(src)/commonlib/bsd/include -I$(src)/arch/x86/include \ @@ -25,10 +26,10 @@ TEST_CFLAGS += -I$(dir $(TEST_KCONFIG_AUTOHEADER))
TEST_CFLAGS += -std=gnu11 -Os -ffunction-sections -fdata-sections \ - -Wl,--gc-sections -fno-builtin + -fno-builtin
# Link against Cmocka -TEST_LDFLAGS = -lcmocka +TEST_LDFLAGS = -lcmocka -Wl,--gc-sections
# Extra attributes for unit tests, declared per test attributes:= srcs cflags mocks stage diff --git a/tests/device/i2c-test.c b/tests/device/i2c-test.c index 16e4d0d..5c4237b 100644 --- a/tests/device/i2c-test.c +++ b/tests/device/i2c-test.c @@ -11,13 +11,8 @@ * GNU General Public License for more details. */
-#include <stdarg.h> -#include <stddef.h> -#include <setjmp.h> -#include <limits.h> -#include <cmocka.h> - #include <device/i2c_simple.h> +#include <limits.h>
/* Simulate two i2c devices, both on bus 0, each with three uint8_t regs implemented. */ diff --git a/tests/include/test.h b/tests/include/test.h new file mode 100644 index 0000000..a628f80 --- /dev/null +++ b/tests/include/test.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef _TESTS_TEST_H +#define _TESTS_TEST_H + +/* + * Standard test header that is automatically included for all tests. For now it just + * encapsulates the include dependencies for Cmocka. + */ + +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> + +#endif /* _TESTS_TEST_H */ diff --git a/tests/lib/string-test.c b/tests/lib/string-test.c index 210aaba..fd53980 100644 --- a/tests/lib/string-test.c +++ b/tests/lib/string-test.c @@ -1,7 +1,3 @@ -#include <stdarg.h> -#include <stddef.h> -#include <setjmp.h> -#include <cmocka.h>
#include <string.h>
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add test.h default header and fix --gc-sections ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add test.h default header and fix --gc-sections ......................................................................
Patch Set 1: Code-Review+2
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add test.h default header and fix --gc-sections ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41045/1/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41045/1/tests/Makefile.inc@17 PS1, Line 17: -include $(src)/include/rules.h -include $(testsrc)/include/test.h While I really like your optimization, with putting all necessary headers into test.h, my personal opinion is to not use too many non-explicit magic, where it doesn't bring too many benefit. I mean, typing "#include <test.h>" line in every test is not a big deal, but may help developers to easily follow the logic of where all these cmocka*/assert* functions come from and why they get all necessary stdlib headers included.
That being said, I'm also fine with merging this in the shape which you proposed.
https://review.coreboot.org/c/coreboot/+/41045/1/tests/Makefile.inc@41 PS1, Line 41: subdirs:= tests/arch tests/commonlib tests/console tests/cpu tests/device Let's use newly-introduced $(testsrc) here.
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Jan Dabros,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41045
to look at the new patch set (#2).
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
tests: Add <tests/test.h> wrapper header and fix --gc-sections
<cmocka.h> requires a few standard headers to be explicitly included before itself or it will throw compilation errors. Having to always include these headers in the right order in every test is cumbersome. Instead, this patch encapsulates the problem in a new <tests/test.h> header that all tests should include (instead of <cmocka.h> directly).
Also fix --gc-sections in the test framework which needs to be passed for linking, not for compiling.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I4284d74c8673708e21a5266eb42f7b9ae19a1b12 --- M tests/Makefile.inc M tests/device/i2c-test.c A tests/include/tests/test.h M tests/lib/string-test.c 4 files changed, 27 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/41045/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41045/1/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41045/1/tests/Makefile.inc@17 PS1, Line 17: -include $(src)/include/rules.h -include $(testsrc)/include/test.h
While I really like your optimization, with putting all necessary headers into test. […]
Fair enough, I'm fine including it explicitly too. Maybe let me introduce an include/tests subdirectory so that we can have test headers without risking naming clashes to anything else.
https://review.coreboot.org/c/coreboot/+/41045/1/tests/Makefile.inc@41 PS1, Line 41: subdirs:= tests/arch tests/commonlib tests/console tests/cpu tests/device
Let's use newly-introduced $(testsrc) here.
$(testsrc) is absolute but in all other cases subdirs are defined relative, so I would keep it that way here too (even though I think it would also work absolute).
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41045/1/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41045/1/tests/Makefile.inc@41 PS1, Line 41: subdirs:= tests/arch tests/commonlib tests/console tests/cpu tests/device
$(testsrc) is absolute but in all other cases subdirs are defined relative, so I would keep it that […]
Yes, it should work - but OK.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41045/2/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41045/2/tests/Makefile.inc@29 PS2, Line 29: -fno-builtin nit: this should fit on the previous line
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41045/2/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41045/2/tests/Makefile.inc@29 PS2, Line 29: -fno-builtin
nit: this should fit on the previous line
Done
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41045/3/tests/include/tests/test.h File tests/include/tests/test.h:
https://review.coreboot.org/c/coreboot/+/41045/3/tests/include/tests/test.h@... PS3, Line 8: * Standard test header that is automatically included for all tests. For now it just This is no longer automatically included, right?
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Jan Dabros,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41045
to look at the new patch set (#4).
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
tests: Add <tests/test.h> wrapper header and fix --gc-sections
<cmocka.h> requires a few standard headers to be explicitly included before itself or it will throw compilation errors. Having to always include these headers in the right order in every test is cumbersome. Instead, this patch encapsulates the problem in a new <tests/test.h> header that all tests should include (instead of <cmocka.h> directly).
Also fix --gc-sections in the test framework which needs to be passed for linking, not for compiling.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I4284d74c8673708e21a5266eb42f7b9ae19a1b12 --- M tests/Makefile.inc M tests/device/i2c-test.c A tests/include/tests/test.h M tests/lib/string-test.c 4 files changed, 28 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/41045/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41045/3/tests/include/tests/test.h File tests/include/tests/test.h:
https://review.coreboot.org/c/coreboot/+/41045/3/tests/include/tests/test.h@... PS3, Line 8: * Standard test header that is automatically included for all tests. For now it just
This is no longer automatically included, right?
Right, fixed.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 4:
This one too please. ;)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 4: Code-Review+2
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Jan Dabros, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41045
to look at the new patch set (#5).
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
tests: Add <tests/test.h> wrapper header and fix --gc-sections
<cmocka.h> requires a few standard headers to be explicitly included before itself or it will throw compilation errors. Having to always include these headers in the right order in every test is cumbersome. Instead, this patch encapsulates the problem in a new <tests/test.h> header that all tests should include (instead of <cmocka.h> directly).
Also fix --gc-sections in the test framework which needs to be passed for linking, not for compiling.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I4284d74c8673708e21a5266eb42f7b9ae19a1b12 --- M tests/Makefile.inc M tests/device/i2c-test.c A tests/include/tests/test.h M tests/lib/string-test.c 4 files changed, 28 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/41045/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 5:
(trivial rebase conflict)
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 5: Code-Review+1
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 5: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
tests: Add <tests/test.h> wrapper header and fix --gc-sections
<cmocka.h> requires a few standard headers to be explicitly included before itself or it will throw compilation errors. Having to always include these headers in the right order in every test is cumbersome. Instead, this patch encapsulates the problem in a new <tests/test.h> header that all tests should include (instead of <cmocka.h> directly).
Also fix --gc-sections in the test framework which needs to be passed for linking, not for compiling.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I4284d74c8673708e21a5266eb42f7b9ae19a1b12 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41045 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Jan Dabros Reviewed-by: Paul Fagerburg pfagerburg@chromium.org --- M tests/Makefile.inc M tests/device/i2c-test.c A tests/include/tests/test.h M tests/lib/string-test.c 4 files changed, 28 insertions(+), 17 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Fagerburg: Looks good to me, approved Jan Dabros: Looks good to me, but someone else must approve
diff --git a/tests/Makefile.inc b/tests/Makefile.inc index debb2f9..9ee27cd 100644 --- a/tests/Makefile.inc +++ b/tests/Makefile.inc @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only
+testsrc = $(top)/tests testobj = $(obj)/tests
TEST_DEFAULT_CONFIG = $(top)/configs/config.emulation_qemu_x86_i440fx @@ -10,12 +11,12 @@ TEST_KCONFIG_SPLITCONFIG := $(testobj)/config TEST_KCONFIG_TRISTATE := $(testobj)/tristate.conf
-TEST_CFLAGS = -include$(src)/include/kconfig.h \ - -include$(src)/commonlib/bsd/include/commonlib/bsd/compiler.h \ - -include $(src)/include/rules.h \ +TEST_CFLAGS = -include $(src)/include/kconfig.h \ + -include $(src)/commonlib/bsd/include/commonlib/bsd/compiler.h \ + -include $(src)/include/rules.h
# Include generic test mock headers, before original ones -TEST_CFLAGS += -Itests/include/mocks +TEST_CFLAGS += -I$(testsrc)/include/mocks -I$(testsrc)/include
TEST_CFLAGS += -I$(src)/include -I$(src)/commonlib/include \ -I$(src)/commonlib/bsd/include -I$(src)/arch/x86/include \ @@ -24,10 +25,10 @@ TEST_CFLAGS += -I$(dir $(TEST_KCONFIG_AUTOHEADER))
TEST_CFLAGS += -std=gnu11 -Os -ffunction-sections -fdata-sections \ - -Wl,--gc-sections -fno-builtin + -fno-builtin
# Link against Cmocka -TEST_LDFLAGS = -lcmocka +TEST_LDFLAGS = -lcmocka -Wl,--gc-sections
# Extra attributes for unit tests, declared per test attributes:= srcs cflags mocks stage diff --git a/tests/device/i2c-test.c b/tests/device/i2c-test.c index 9303456..acf7b07 100644 --- a/tests/device/i2c-test.c +++ b/tests/device/i2c-test.c @@ -1,12 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <stdarg.h> -#include <stddef.h> -#include <setjmp.h> -#include <limits.h> -#include <cmocka.h> - #include <device/i2c_simple.h> +#include <limits.h> +#include <tests/test.h>
/* Simulate two i2c devices, both on bus 0, each with three uint8_t regs implemented. */ diff --git a/tests/include/tests/test.h b/tests/include/tests/test.h new file mode 100644 index 0000000..b4e0dd2 --- /dev/null +++ b/tests/include/tests/test.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef _TESTS_TEST_H +#define _TESTS_TEST_H + +/* + * Standard test header that should be included in all tests. For now it just encapsulates the + * include dependencies for Cmocka. Test-specific APIs that are so generic we would want them + * available everywhere could also be added here. + */ + +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> + +#endif /* _TESTS_TEST_H */ diff --git a/tests/lib/string-test.c b/tests/lib/string-test.c index 513b395..08f4177 100644 --- a/tests/lib/string-test.c +++ b/tests/lib/string-test.c @@ -1,11 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <stdarg.h> -#include <stddef.h> -#include <setjmp.h> -#include <cmocka.h> - #include <string.h> +#include <tests/test.h>
/* * Important note: In every particular test, don't use any string-related
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41045 )
Change subject: tests: Add <tests/test.h> wrapper header and fix --gc-sections ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3540 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3539 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3538 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3537
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Jan Dabros (Code Review)
-
Julius Werner (Code Review)
-
Paul Fagerburg (Code Review)
-
Paul Menzel (Code Review)