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

[Magnus Ihse Bursie]

I have an updated webrev.

Here is a differential webrev showing the changes compared to the 
previous webrev:
http://cr.openjdk.java.net/~ihse/JDK-8176084-introduce-run-test/webrev.02

And here is a full webrev showing all changes:
http://cr.openjdk.java.net/~ihse/JDK-8176084-introduce-run-test/webrev.03

Replies to specific comments inline.

On 2017-03-02 11:40, Erik Joelsson wrote:
> 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.
Yeah, I agree, it's a bit corny. It grew out of a non-caching solution. 
According to our off-list discussion, I have now removed the caching 
mechanism instead. It didn't do much for performance and just 
complicated things. Instead I include FindTests.gmk where it is needed.
>
> FindTests.gmk
> 51: This conditional seems redundant, did you mean to put a wildcard 
> there?
Yes I did. Thanks.
> 63: should be 2 spaces
Really? I think it's a single line I had to break into two, similar to 
what's on line 66. Do you consider it to be a logical indentation? If 
so, I should probably move the closing parenthesis to a separate line, 
but that feels overkill. In general, I have not considered simple 
foreach:es to be logical construct but one-liners.

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

>
> 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.
No sh*t. :-& I've updated the code to match this pattern.

>
> RunTests.gmk
> 74: I thought Christian made the default for hotspot be min(12, 
> $(NUM_CORES) / 2)?
I tried to mimic what I could extract from the existing logic. However, 
I see now that you say it that I've missed some concurrency-related 
hacks in the old hotspot JTreg Makefile. (Both this and the 
MaxRAMFraction). Talk about convoluted... :-( I fixed this now by 
porting the old behavior verbatim, but I think this area could need some 
cleanup in the future.

Also, I noticed that the fix to limit JTreg jobs to 50 had disappeared 
somewhere when I worked with handling the hotspot special logic. I have 
now restored that.

> 238: The command is wrapped in ExecuteWithLog, why pipe to yet another 
> log file?
For consistency, to store the log output in the rest-results directory, 
like the other test suites do. Gtest does not have a built-in way of 
storing the test output to a log file. I could have copied the 
ExecuteWithLog file at the end, but that felt like I meddled with a 
private part of the interface. What if we introduce a way to disable 
logging in ExecuteWithLog? Then the gtest parsing would fail.
> 242,361: shouldn't parse-test have run-test as prerequisite (even if 
> .NOTPARALLEL is in effect)?
Yes, it should indeed. Fixed.
> 261: Would be nice with a line of # heading this section too
Okie-dokie.
> 325: The MaxRAMFraction for at least hotspot was made dynamic on the 
> concurrency AFAIK
Correctly. I missed that part as well. Fixed.
> 430: I think it would be better just adding $(TARGETS) as prereq
Better like this?

/Magnus

>
> /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
>>
>