RFR: JDK-8176084 Developer-friendly run-test facility

[Erik Joelsson]

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
>