RFR: JDK-8176084 Developer-friendly run-test facility
Magnus Ihse Bursie
magnus.ihse.bursie at oracle.com
Fri Mar 3 09:39:46 UTC 2017
I have an updated webrev.
Here is a differential webrev showing the changes compared to the
And here is a full webrev showing all changes:
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
> 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.
> 51: This conditional seems redundant, did you mean to put a wildcard
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.
> 772: an -> a
> 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.
> 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
> 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?
> 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
More information about the build-dev