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 3:
(9 comments)
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 out with a message if they are.
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 should always independent of whatever the user set in menuconfig for their previous build. I would just exclude this with ifneq ($(UNIT_TEST),1).
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 considering that you are using $(obj)/tests directly multiple times below, I don't think it can. Maybe it's better to just get rid of it and always use $(obj)/tests directly?
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.
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?
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 $(obj).
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 extra command line variable or a separate target to run them all despite errors, but I think the default behavior should be to stop on the first error. Also, I think make -k basically gives you the other behavior already.)
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 to figure out if there were errors or not (because the errors may scroll off the screen from a bunch of successful tests that ran in parallel). We solved that by echoing a big fat multi-line "all tests passed" message in the recipe for the overall "run tests" target. That way, if you see the message you know that everything passed, if you don't you know there must be an error hidden somewhere further up. I would suggest doing that here as well. (This requires that you don't discard errors when running tests, see above.)
https://review.coreboot.org/c/coreboot/+/39894/1/tests/include/config.h File tests/include/config.h:
PS1:
Actually at the time of writing above, I was unaware of the easy method of defining variable per-tar […]
Yes, time to generate the configs is a good point. It's probably better to go with generating the extra header by hand as you mentioned, that should be much faster and I think it should be good enough. Agree that this can be pushed until later.