Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39894 )
Change subject: tests: Add build subsystem for unit testing coreboot ......................................................................
Patch Set 2:
(13 comments)
Thanks, I think this looks a *lot* better already! I'm excited about all the things we'll be able to do with this!
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@52 PS2, Line 52: # Include Makefile.inc, clean test-specific variables; : # Add directory prefix to all test-specific variables and test name : # itself in order to allow the same test name in different directories : includemakefiles= \ : $(foreach value,tests test-subdirs, \ : $(eval $(value):=)) \ : $(eval -include $(1)) \ : $(foreach test, $(tests), \ : $(foreach attribute, $(attributes), \ : $(eval $(addprefix $(dir $(1)), $(test)-$(attribute)):= \ : $($(test)-$(attribute))))) \ : $(foreach test, $(tests), \ : $(foreach attribute, $(attributes), \ : $(eval $(test)-$(attribute):=))) \ : $(eval alltests+=$(addprefix $(dir $(1)), $(tests))) \ : $(eval subdirs+=$(subst $(CURDIR)/,, \ : $(abspath $(addprefix $(dir $(1)),$(test-subdirs))))) : : # Traverse through the unit-tests tree and invoke includemakefiles : evaluate_subdirs= \ : $(eval cursubdirs:=$(subdirs)) \ : $(eval subdirs:=) \ : $(foreach dir,$(cursubdirs), \ : $(eval $(call includemakefiles,$(dir)/Makefile.inc))) \ : $(if $(subdirs),$(eval $(call evaluate_subdirs)))
this is copied from the toplevel Makefile. […]
Well, this is a different includemakefiles specific for tests. However, I think it might be possible to use the default version by making 'tests' a special class. Then you'd basically have this in Makefile.inc:
tests-y += i2c-test # can auto-detect that i2c-test.c is the runner source file, see below i2c-test-srcs += src/device/i2c.c i2c-test-mocks += platform_i2c_transfer
...and then you need to define a handler roughly like this:
define tests-handler alltests += $(1)/$(2) $(2)-runner := $(if $($(2)-runner)),$($(2)-runner),$(2).c) $(2)-srcs += $(1)/$($(2)-runner)) $(foreach attribute,$(attributes),$(eval $(1)/$(2)-$(attribute) += $(2)-$(attribute))) $(foreach attribute,$(attributes),$(eval $(2)-$(attribute):=)) endef
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@86 PS2, Line 86: $(eval $(call evaluate_subdirs)) Does this actually need the eval here?
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@88 PS2, Line 88: .SECONDEXPANSION: IIRC you only need this once per 'make' invocation, so if this file is always included from toplevel you shouldn't need it here.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@93 PS2, Line 93: $(eval WRAP_LD_FLAGS:= $(foreach mock,$($(1)-mocks),-Wl,--wrap=$(mock))) Does this need to use eval?
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@96 PS2, Line 96: $$$$(filter tests/$$$$*.c, $($(1)-srcs)) $(TEST_KCONFIG_AUTOHEADER) If you didn't cut the 'tests/' out of the name (see below) this could also just be way simpler as '$$$$*.c' instead of all the filtering.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@98 PS2, Line 98: $(HOSTCC) $(HOSTCFLAGS) $(TEST_CFLAGS) $($(1)-cflags) -MMD \ Shouldn't there be an
-include $(TEST_KCONFIG_AUTOHEADER)
somewhere (either here or in $(TEST_CFLAGS))?
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@99 PS2, Line 99: $$(@) nit: Let's try to be consistent about using $@ or $(@) (though I know other parts of coreboot aren't either). I think $@ is more common for automatic variables.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@102 PS2, Line 102: $(HOSTCFLAGS) $(TEST_CFLAGS) I think usually we don't pass CFLAGS for linking (or is there a good reason to here?).
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@103 PS2, Line 103: $(TEST_LDFLAGS) $(WRAP_LD_FLAGS) -o $$@ Please always add an extra indent for wrapped lines.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@107 PS2, Line 107: # tests/ dir Somewhat confused why you cut the "tests/" out, since you're not cutting the "src/" in supporting files out either. I feel like keeping them both would be more consistent.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@112 PS2, Line 112: %-test.o So is this the main reason you want to suffix all tests with '-test'? I think you could have this much easier by just calling the binary
$(eval $(test)-bin:=build/$(test)/run)
or something. There's no reason why the binary name needs to match the test file if you've sequestered it in its own directory already, after all (but if you want that you could have that too with $(notdir (test)) or whatever).
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@119 PS2, Line 119: $(patsubst %.c,%.o, $(subst tests/,,$($(test)-srcs)))))) Why not just use $($(test)-objs)?
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h File tests/include/config.h:
PS1:
In v2 I've added automatic generation of config for all tests. By default, qemu-x86 .config is used. […]
I'm not really sure I understand the dependency problem you're describing? You just need to make the rule to build the test dependent on the test-specific config.h (which could be stored under build/test/..path../file-test.config.h or something like that), and then you write a recipe to create that config.h file by creating a minimal .config with the desired options and running the equivalent of $(MAKE) oldconfig on it (or just running the 'conf' utility directly if that's easier).
But anyway, if that's too hard I think the poor-man's Kconfig by hacking up a header with #undefs as you describe would probably be good enough as well. I think it would be okay to start with that and see how far it gets us.