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

Fri Mar 3 10:29:12 UTC 2017

Looks good to me now.

/Erik

On 2017-03-03 10:39, Magnus Ihse Bursie wrote:
> 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
>>>
>>
>