RFR: 7903373: Add ability to customize test execution using main wrapper plugin [v5]

[Jonathan Gibbons]

On Thu, 10 Nov 2022 00:00:14 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> The fix adds support of a plugin that customizes test execution.
>
> Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   empty lines removed.

I think there should be more documentation for this feature, both internal and for end users.

Generally, I find the naming based on "main wrapper" and "custom main wrapper" to be confusing and/or misleading, although you might be able to work around that with better documentation.

At the core, the name `CustomMainWrapper` suggests a variant, perhaps a subtype, of `MainWrapper`, but that's not the case.`MainWrapper` delegates to `CustomMainWrapper`,
and (somewhat more weirdly) `MainActionHelper` uses `CustomMainWrapper` as well.

In fact, `CustomMainWrapper` doesn't seem to be a custom main-wrapper at all; it is really just a custom thread factory, as can be seen by the only uses in the two instances of the expression `CustomMainWrapper.getInstance(...).createThread()`.  Indeed, `createThread` is the only real abstract method on the interface; `setAction` is just there for use during initialization, since the instance of `CMW.getInstance` is discarded after a thread has been created.

In terms of documentation, I suggest:

* better, more accurate, more specific documentation on the new `CustomMainWrapper` class. In particular, this documentation should indicate that the class just provides the ability to specify a thread factory to be used by the standard `MainWrapper` class

* more info in the i18.properties file (which is used to generate the command-line help.)
 In particular, although I have not compiled and run the code, I suspect that the command-line help for the `-mainwrapper` option will look something like
       `-mw <classname>, -mainwrapper <classname>   Main wrapper classname.`
which is essentially content-free, giving no useful info at all to the reader

* in the tag spec, in the appropriate Appendix, you should list any system properties that are set during this feature.

src/share/classes/com/sun/javatest/regtest/agent/CustomMainWrapper.java line 38:

> 36: /**
> 37:  * Interface which should be implemented to customize execution of test.
> 38:  * It is used by main and driver actions to execute test.

This comment needs a lot of work. More than anywhere else, this seems like the place to document this feature.  In particular, the only aspect of the execution that can be customized is the thread that is created to run the test.

src/share/classes/com/sun/javatest/regtest/agent/CustomMainWrapper.java line 44:

> 42:         String[] args = mainWrapper.split(":", 2);
> 43:         String className = args[0];
> 44:         String actionName = args[1].split("=")[1];

It would help if somewhere there was a comment or info on the proposed format for the `mainWrapper` string, illustrating the use of the colon and equals separators.

I don't (yet) see the overall significance of `args[1]` beyond the `actionName`

src/share/classes/com/sun/javatest/regtest/agent/CustomMainWrapper.java line 61:

> 59:             Constructor<? extends CustomMainWrapper> ctor = clz.getDeclaredConstructor();
> 60:             CustomMainWrapper wrapper = ctor.newInstance();
> 61:             wrapper.setAction(actionName);

Is the `actionName` ever going to be useful?   Di you use it in the Loom implementation?  Why would the thread created by `createThread` depend on the difference between `main` and `driver` actions?

(In general, a `driver` action is handled much earlier in the execution, when `RegressionScript` decides what VM options (if any) should be used for the agent.

src/share/classes/com/sun/javatest/regtest/agent/CustomMainWrapper.java line 63:

> 61:             wrapper.setAction(actionName);
> 62:             return wrapper;
> 63:         } catch (Exception e) {

What's the rationale for catching all exceptions here, instead of just checked exceptions?

src/share/classes/com/sun/javatest/regtest/agent/CustomMainWrapper.java line 70:

> 68:     /**
> 69:      * This method should be implemented to run test task.
> 70:      * Default jtreg implementation is return new Thread(tg, task);

This line, about the default jtreg implementation, is confusing, because the default jtreg behavior is not an implementation of this class or method.

src/share/classes/com/sun/javatest/regtest/agent/CustomMainWrapper.java line 80:

> 78:      * @param actionName name of action
> 79:      */
> 80:     default void setAction(String actionName) {

The comment says "get information" but the method name is "setAction". What am I missing?

src/share/classes/com/sun/javatest/regtest/config/RegressionParameters.java line 634:

> 632: 
> 633:     private static final String CUSTOM_MAIN_WRAPPER = ".customMainWrapper";
> 634: 

maybe remove this blank line, to group the customMainWrapper properties together

src/share/classes/com/sun/javatest/regtest/exec/MainAction.java line 489:

> 487:         if (script.getCustomWrapperPath() != null) {
> 488:             javaProps.put(MainWrapper.MAIN_WRAPPER_PATH, script.getCustomWrapperPath());
> 489:         }

these properties should probably be documented in the appendix at the end of the tag spec

src/share/classes/com/sun/javatest/regtest/exec/MainAction.java line 630:

> 628:         Agent agent;
> 629:         try {
> 630:             String wrapper = script.getCustomWrapper() == null ? null : script.getCustomWrapper() + ":" + "action=" + this.getName();

Is this the other side of the parsing done in CustomMainWrapper:44?
Is there any need for the `action=` part of the string, since it does not appear to be checked later on

src/share/classes/com/sun/javatest/regtest/tool/i18n.properties line 177:

> 175: help.main.mw.desc=Main wrapper classname.
> 176: help.main.mwp.arg=<path>
> 177: help.main.mwp.desc=Specifies classspath with custom main wrapper implementation.

There should be more documentation, at least somewhere, about what this mechanism is for and how to use it.

-------------

PR: https://git.openjdk.org/jtreg/pull/136