RFR: 8321124: java/util/stream/GatherersTest.java times out [v4]

[Viktor Klang]

On Tue, 5 Dec 2023 14:07:53 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Viktor Klang has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Removing extraneous built-in gatherer test output
>
> test/jdk/java/util/stream/GatherersMapConcurrentTest.java line 106:
> 
>> 104:             );
>> 105:         assertEquals("expected", exception.getMessage());
>> 106:         assertNull(exception.getCause());
> 
> I assume it would be possible to add a comrade to this test for the wrapping case too, e.g. if the mapper throws an Error.

@AlanBateman Yes, good point. I'll add that as well now 👍

> test/jdk/java/util/stream/GatherersMapConcurrentTest.java line 245:
> 
>> 243:     public void behavesAsExpected(Config config) {
>> 244:         for (var concurrency : List.of(1, 2, 3, 10, 1000)) {
>> 245:             final var expectedResult = config.stream()
> 
> I assume the loop is here because you can't have both a MethodSource and ValueSource. I think I'd probably create a new method source for this, e.g.
> 
> 
>     static Stream<Arguments> configutatonAndConcurrency() {
>         return IntStream.of(1, 2, 3, 10, 1000)
>                 .mapToObj(i -> i)
>                 .flatMap(i -> configurations().map(c -> new Arguments(c, i)));
>     }
> 
> 
> The advantage is that you'll have the parameter in the .jtr in the event of a failure.

@AlanBateman Yeah, it's an unfortunate limitation of JUnit. Ideally I'd like to have an annotation per argument and being able to specify that I'd like the framework to execute it as a cartesian product.

Do you want me to make this change as a part of this PR or does it make more sense to get this one integrated first?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16928#discussion_r1415691074
PR Review Comment: https://git.openjdk.org/jdk/pull/16928#discussion_r1415690167