On Sun, 3 Dec 2023 09:20:50 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> test/jdk/java/util/stream/BuiltInGatherersTest.java line 257:
>>
>>> 255: // Test cancellation after exception during processing
>>> 256: // Only use reasonably sized streams to avoid excessive thread
>>> creation
>>> 257: if (config.streamSize > 2 && config.streamSize < 100) {
>>
>> @AlanBateman Decided to not run the cancellation tests for the larger
>> streams as it creates a bunch of extra resource usage which makes GHA really
>> unhappy.
>
> Okay but it makes me wonder if this test should have its use method source
> with the stream sizes that are sensible to test.
Yeah, I have plans to split the tests out to separate files beyond the Preview.
>> test/jdk/java/util/stream/BuiltInGatherersTest.java line 331:
>>
>>> 329: case Integer n when n ==
>>> config.streamSize - 1 -> {
>>> 330: awaitSensibly(firstReady);
>>> 331:
>>> while(tasksWaiting.getQueueLength() < tasksToCancel) {
>>
>> @AlanBateman This is the only part of this I really don't like. Any better
>> suggestion as to "wait for N things waiting" in a test? 🤔
>
>> @AlanBateman This is the only part of this I really don't like. Any better
>> suggestion as to "wait for N things waiting" in a test? 🤔
>
> Semaphore::getQueueLength is more for monitoring purposes so the usage does
> seem a bit unusual here. There are a number of ways this could be done, maybe
> the simplest is for tasksWaiting to be an AtomicInteger and have the default
> just increment it and sleep-for-a-day.
@AlanBateman Unfortunately that doesn't really work as there's then a window
between incrementing the counter and going to sleep. So while it might execute
correctly in 99.9999% of cases I don't want to have a jbs bug filed which will
be one of those things where you can't recall it 6-12 months down the line 😓
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16928#discussion_r1413534692
PR Review Comment: https://git.openjdk.org/jdk/pull/16928#discussion_r1413537391
