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 for unit testing coreboot ......................................................................
Patch Set 2:
(13 comments)
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@18 PS2, Line 18: tests
$(testobj) since we try to keep the source tree clean.
Yes, this is correct. Actually putting this autoheader mixed with source code lead to error, where I was not removing it with "clean-unit-tests" target. Location of this autoheader will be adjusted to new Kconfig-handling approach.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@27 PS2, Line 27: TEST_KCONFIG_FLAGS:= DOTCONFIG=$(TEST_DOTCONFIG) \
it's possible to override variables per-target. […]
Makes sense.
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)))
Well, this is a different includemakefiles specific for tests. […]
Yes, as Julius pointed this is not the copy from top Makefile - however indeed I was using this as a reference.
I was evaluating concept of using classes, since this is used throughout coreboot build system. While usual classes will be hard to use, this special classes is something which suits better - as you have shown in your example.
The reason why I finally decided to create my own implementation is that I wanted to avoid extra changes to the top Makefile, which will be necessary in this case. Furthermore, there is Makefile.inc under payloads which is also defining its own, so I replicate this approach.
That being said, since usage of special-class will be a nice "link" between my work and coreboot build system, I will make such change in v3. But please take a look at top Makefile changes whether you like them.
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?
No - I mean, at least functionality isn't altered by simply invoking $call. That being said, for couple of $(eval)'s in this file, I was using the same syntax as top Makefile. Wasn't quite sure why they need to be there, however wanted to be on the safe side. I assume this may be a possibility to fix also code which is already in the tree.
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 […]
This needs to be declared _before_ the first target which uses this feature. Since this Makefile is included before this target in top Makefile, Make wasn't happy. However with my change for top Makefile in v3, this will be no longer necessary.
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?
You got me there - this is something tricky, which I must admit I'm not fully understand. Actually after removing this $(eval), I'm facing strange issue, that is $(WRAP_LD_FLAGS) is "visible" only for the consecutive iteration of foreach, not the intended one. Looks like eval creates a Makefile constructs not in the order I'm expecting them. For example, consider that first foreach iteration is for i2c-test and second string-test. Mock should be registered only for the first one. In the recipe for *-bin targets I left only one step: echo "inside $$@ $(WRAP_LD_FLAGS). I can see following output inside build/tests/device/i2c-test/i2c-test-run #note that $(WRAP_LD_FLAGS) is empty here inside build/tests/lib/string-test/string-test-run -Wl,--wrap=platform_i2c_transfer #This should be already empty here
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 '$ […]
Indeed. I was so proud of creating such a complicated syntax.. (yes, I know Keep It Simply Stupid) :) I will not remove tests/ from the path and prereq list much simpler.
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 […]
I've added a "-Ibuild/tests" to TEST_CFLAGS earlier, with a comment that it is for config.h. However it will be much better to use TEST_CFLAGS += -I$(dir $(TEST_KCONFIG_AUTOHEADER)).
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?).
Yes, this doesn't make much sense, I will separate linker and compiler flags anyway by putting do different variables. However I will keep $($(1)-cflags), since don't want to introduce another attribute for devs.
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 fi […]
Frankly, at this point in time, I'm no longer sure what was my reasoning behind removing tests/ from obj path within particular test directory. Let's not overcomplicate things and keep both tests/ and srcs/.
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'?
No, as I said I want to differentiate name of uut and test module. This code is just making use of it :)
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).
Thanks, this is much simpler solution - will use it. Initially I was thinking that different name of binary may be useful, when we will (for some reason) copy them all to one common dir. However one thing I missed - test names may be the same across project, so the only thing to differentiate is the whole path to binary. So after all - I will keep naming test binaries "run". One more point which you has shown, though not explicitly - it makes sense to put binary right under /build/test/ directory instead of putting it in the same dir as main test module.
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)?
Good point.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@128 PS2, Line 128: cp $(DEFAULT_CONFIG) $(TEST_DOTCONFIG)
I cherry-picked the set of changes to my local tree, followed the instructions in the next CL, and g […]
Indeed, there is an issue with wrong cp argument - will fix in v3. I made this as a last minute change - however still perform a test of cleaning and building once again. The problem is that my clean isn't removing .config.. so this target was not run again. I need to fix this too.