On Sat, 4 Jun 2022 01:51:20 GMT, Leo Korinth <lkori...@openjdk.org> wrote:
> One can select a testcase by ID when running a jtreg test case directly from > jtreg (using the testcase.java#testID syntax). However, this has not been > possible to do when launching jtreg indirectly from make. > > This fix attempts to address this issue. I have not tested this thoroughly > yet, I wanted to show the code to get feedback first. The idea is to *not* > emulated destructive imperative assignments through the use of eval. I > instead try to handle it in a functional style without reassigning variables. > I have tried to make the change as small as possible. > > I am not used to change the build system, so I would appreciate thorough > review. > > `IfAppend` and `IfPrepend` are general purpose functions. If similar > functions exists elsewhere inside the code base or in make itself I should > probably use those instead. If they do not exist, they might be useful > elsewhere and maybe should be placed in a common make file instead of the > RunTests.gmk file. I think this is a nice fix. I mostly have style opinions. I would also like to have Magnus look at this as he wrote this originally. As for testing, it would be good to verify a decently sized job in Mach5, as well as trying to use the TestID selection on multiple platforms. make/RunTests.gmk line 39: > 37: > ################################################################################ > 38: > 39: HASH:=\# HASH is already defined in MakeBase.gmk so should be available here. make/RunTests.gmk line 47: > 45: define IfPrepend > 46: $(if $(strip $1),$(strip $2)$(strip $1),) > 47: endef These two probably fits better in make/common/Utils.gmk. Also please have a look at the [Code Conventions for the Build System](http://openjdk.java.net/groups/build/doc/code-conventions.html). One liner macros like this we like to format like this: IfPrepend = \ $(if $(strip $1),$(strip $2)$(strip $1),) make/RunTests.gmk line 51: > 49: define TestID > 50: $(subst .java,,$(subst .java$(HASH),,$(suffix $(notdir $1)))) > 51: endef This macro deserves a comment explaining what it's trying to do. make/RunTests.gmk line 53: > 51: endef > 52: > 53: define HashTestID This one too. make/RunTests.gmk line 377: > 375: # either absolute or relative to any of the TEST_BASEDIRS or test roots. > 376: define ParseJtregTestSelection > 377: $(call IfAppend, \ Please adjust indentation of the whole body here. make/RunTests.gmk line 444: > 442: $(strip $(foreach parser,ParseCustomTestSelection > ParseGtestTestSelection ParseMicroTestSelection ParseJtregTestSelection > ParseSpecialTestSelection \ > 443: ,$(call $(parser),$1))) > 444: endef As this is logically a one-liner macro, please format according to guidelines. Also try to keep lines shortish. Something like this: Suggestion: ParseTestSelection = \ $(strip $(foreach parser, ParseCustomTestSelection ParseGtestTestSelection ParseMicroTestSelection \ ParseJtregTestSelection ParseSpecialTestSelection, \ $(call $(parser), $1) \ )) make/RunTests.gmk line 448: > 446: # Now intelligently convert the test selection given by the user in TEST > 447: # into a list of fully qualified test descriptors of the tests to run. > 448: TESTS_TO_RUN:=$(strip $(foreach test,$(TEST),$(call > ParseTestSelection,$(test)))) I think this rewrite is more elegant, but logically there is a difference as it no longer breaks early. I would like to have Magnus' opinion on this as he wrote this originally. Style wise, please leave spaces around operators and space after comma when possible. The top level strip should handle any whitespace issues caused by this. ------------- PR: https://git.openjdk.java.net/jdk/pull/9028
