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 4:
(11 comments)
https://review.coreboot.org/c/coreboot/+/39894/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39894/3//COMMIT_MSG@10 PS3, Line 10: based on
built using
Done
https://review.coreboot.org/c/coreboot/+/39894/3/Makefile File Makefile:
https://review.coreboot.org/c/coreboot/+/39894/3/Makefile@145 PS3, Line 145: UNIT_TEST:=1
We might want to test here that no non-test goals are specified together with test goals, and error […]
OK.
https://review.coreboot.org/c/coreboot/+/39894/3/Makefile@171 PS3, Line 171: include $(DOTCONFIG)
Would we ever want to include this $(DOTCONFIG) when building unit tests? I would assume tests shoul […]
I was thinking about that we may want to include toolchain.inc if someone would like to use crosgcc to build unit tests in future (currently it is $(HOSTCC) only). But probably I should then include this file in tests/Makefile.inc. tolchain.inc requires $(DOTCONFIG).
Let me conditionally include both toolchain.inc & $(DOTCONFIG) only when there is no unit-testing target of Make.
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)))
One more comment which I have regarding your proposal above, namely about $(test)-runner. […]
Mark as completed.
https://review.coreboot.org/c/coreboot/+/39894/3/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39894/3/tests/Makefile.inc@5 PS3, Line 5: override testobj := $(abspath $(testobj))
The use of override here implies that $(testobj) can be overridden manually by the user, but conside […]
Yes, this makes sense. Actually I want to be able to change test output directory, but this won't work in current form. Thanks for pointing this out.
https://review.coreboot.org/c/coreboot/+/39894/3/tests/Makefile.inc@18 PS3, Line 18: -include $(TEST_KCONFIG_AUTOHEADER)
Sorry, you were right... if you have kconfig.h you shouldn't have this as well.
Done
https://review.coreboot.org/c/coreboot/+/39894/3/tests/Makefile.inc@58 PS3, Line 58: $$($(1)-stage)
Can we also check if it is in $(classes-y) somewhere and error out otherwise, to guard against typos […]
This won't be doable unless we do traverse through all src/directories (currently, we are not invoking includemakefiles macro if building unit tests). I also don't think this will be justified considering extra time needed for this. I will prepare a Make instruction to check whether value is from static list. Let me know, whether this is acceptable.
https://review.coreboot.org/c/coreboot/+/39894/3/tests/Makefile.inc@61 PS3, Line 61: build
You're using 'build/' a bunch of times directly in here where I think you always want to be using $( […]
Done
https://review.coreboot.org/c/coreboot/+/39894/3/tests/Makefile.inc@115 PS3, Line 115: - ./$^
Why discard errors? Don't we want to stop as soon as we find one? (If you want maybe there can be an […]
Hmm.. Yes, actually I was thinking about running all tests to completion and gathering statistics about them afterwards. Let's say that Jenkins will run unit tests for all new commits and then show the results. For example 60% passes. As you said, -k should do the trick.
Current approach is definitely unacceptable considering usual developers needs. Result should be a binary. Now with 2 tests everything is clear, but yeah, let me change this.
https://review.coreboot.org/c/coreboot/+/39894/3/tests/Makefile.inc@121 PS3, Line 121: run-unit-tests: $(alltests)
In our vboot test framework we discovered that when running all tests in parallel it can be tricky t […]
Makes sense.
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h File tests/include/config.h:
PS1:
Yes, time to generate the configs is a good point. […]
Done