RFR: JDK-8176084 Developer-friendly run-test facility (original) (raw)

Erik Joelsson erik.joelsson at oracle.com
Thu Mar 2 10:40:58 UTC 2017

Hello Magnus,

Overall this is excellent work! But I still have some opinions and questions.

I think the implementation of the caching mechanism is a bit weird and it will not handle changes to jtreg test groups (or other test name definition files). I don't think we can expect people to have to manually clean if they edit such a file. I would very much prefer if this was done the same way as we calculate modules. This would be achieved by: 1. including FindTests.gmk from Main.gmk instead of shell-call. 2. In FindTests.gmk -include $(NAMED_TESTS_INCLUDE). 3. In FindTests.gmk add all actual source files for test names as prerequisites to $(NAMED_TESTS_INCLUDE). 4. Modify FindTests.gmk to only actually do work if the recipe for $(NAMED_TESTS_INCLUDE) is triggered. This can be achieved by moving the calls to the recipe. This means the variables you generate are always read from the generated makefile.

FindTests.gmk 51: This conditional seems redundant, did you mean to put a wildcard there? 63: should be 2 spaces

MakeBase.gmk 772: an -> a 774: did you really mean = and not :=?

Note that when using $(eval) inside a macro body that will also be evaled, you actually need 4 dollars on all references inside it if you want to be sure to handle any $ in the variable values correctly. In this case, if $ is ever a character in one of the parameters to jtreg/gtest, it's likely going to fail. Then we have the special case of the foreach local variable, which has to be referenced with the same amount of dollars as the foreach call, so you can't just put 4 on it. Here is an example of how I figured you can deal with this: (foreach f, ($1_SRC_FILES),
(eval fd := (call DoubleDollar, (f)))
(if (filter /%, (f)),
(eval r := $$(patsubst $$($1_SRC_BASE_DIR)/%, %, $$(fd)))
)
)

Basically for the foreach variable, you need to create a doubledollar version and use the new one in evals, with 4 dollars, and the original in all other cases. Yes, this becomes very convoluted.

RunTests.gmk 74: I thought Christian made the default for hotspot be min(12, $(NUM_CORES) / 2)? 238: The command is wrapped in ExecuteWithLog, why pipe to yet another log file? 242,361: shouldn't parse-test have run-test as prerequisite (even if .NOTPARALLEL is in effect)? 261: Would be nice with a line of # heading this section too 325: The MaxRAMFraction for at least hotspot was made dynamic on the concurrency AFAIK 430: I think it would be better just adding $(TARGETS) as prereq

/Erik

On 2017-03-02 08:42, Magnus Ihse Bursie wrote:

A long-time issue has been a consistent way for developers to effortlessly run tests on local builds. This patch introduces a new, alternative "run-test" target, which allows for a smoother developer experience in running tests. It does not modify or remove any existing ways of running tests, which are still needed for automated test systems and old scripts.

Bug: https://bugs.openjdk.java.net/browse/JDK-8176084 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8176084-introduce-run-test/webrev.01 /Magnus

More information about the build-dev mailing list