RFR: CODETOOLS-7903748 - jcstress: Test list should honor concurrency settings [v6]

[Aleksey Shipilev]

On Thu, 4 Jul 2024 08:11:44 GMT, Jiří Vaněk <jvanek at openjdk.org> wrote:

>> This is extracting  List<TestConfig> configs =prepareRunProgram(classes, tests);
>> with all he HW/switches setup to separated method and reusing it in `-l` mode
>> 
>> I'm aware of triplicated removal of -agentlib, and will clean it up as
>> CODETOOLS-7903756 will progress.
>> 
>> -l now honours also verbose mode, in which it prints not just matching
>> tests but all really run tests, and thus enabling much more easy
>> determining of all tests
>> 
>> help adjusted.
>> 
>> Maybe I'm missing plain quick initial all tests metod now, but with 
>> artificial -c MAX it seems doing exactly that
>
> Jiří Vaněk has updated the pull request incrementally with one additional commit since the last revision:
> 
>   replaced long string concatenation with StringBuilder

Suggestions:

jcstress-core/src/main/java/org/openjdk/jcstress/JCStress.java line 60:

> 58:     public void run() throws Exception {
> 59:         ConfigsWithScheduler configsWithScheduler = getConfigs();
> 60:         if (configsWithScheduler == null) return;

Suggestion:

        ConfigsWithScheduler configs = getConfigs();
        if (configs == null) {
            return;
        }

jcstress-core/src/main/java/org/openjdk/jcstress/JCStress.java line 86:

> 84:     }
> 85: 
> 86: 

Suggestion:

jcstress-core/src/main/java/org/openjdk/jcstress/JCStress.java line 118:

> 116:         }
> 117:         ConfigsWithScheduler configsWithScheduler = new ConfigsWithScheduler(scheduler, configs);
> 118:         return configsWithScheduler;

Suggestion:


        return new ConfigsWithScheduler(scheduler, configs);

jcstress-core/src/main/java/org/openjdk/jcstress/Main.java line 58:

> 56:         JCStress jcstress = new JCStress(opts);
> 57:         if (opts.shouldList()) {
> 58:             listTests(opts, jcstress);

So, for symmetry with `parseResults` and `run`, why not make it the instance method of `JCStress`?

This will also allow to make `JCStress.ConfigsWithScheduler` `private`, AFAICS.

jcstress-core/src/main/java/org/openjdk/jcstress/Options.java line 51:

> 49: 
> 50:     private static final String LIST_OPTION_DESCRIPTION="List the available tests matching the requested settings, " +
> 51:             "after all filters (like CPU count) are applied. In verbose mode it prints all real combinations which will run.";

Introducing a `static final String` for option description is not the style for this file. Please inline it back.

jcstress-core/src/main/java/org/openjdk/jcstress/infra/runners/TestConfig.java line 249:

> 247: 
> 248: 
> 249:     public String toDetailedTest() {

The output looks a bit ugly. I think I understand the motivation for printing out more data about the tests, but then it should be more clean. Right now it looks like just an internals dump, which is not a preferred style for jcstress output.

Or, we can just not do it at all, and just print the test names, without these verbose printouts. That would be my preference.

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

Changes requested by shade (Committer).

PR Review: https://git.openjdk.org/jcstress/pull/149#pullrequestreview-2162933112
PR Review Comment: https://git.openjdk.org/jcstress/pull/149#discussion_r1668422320
PR Review Comment: https://git.openjdk.org/jcstress/pull/149#discussion_r1668421718
PR Review Comment: https://git.openjdk.org/jcstress/pull/149#discussion_r1668421467
PR Review Comment: https://git.openjdk.org/jcstress/pull/149#discussion_r1668414286
PR Review Comment: https://git.openjdk.org/jcstress/pull/149#discussion_r1668404100
PR Review Comment: https://git.openjdk.org/jcstress/pull/149#discussion_r1668425396