RFR: 8292215: java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTest.java times out with slowdebug [v2]

[Aleksey Shipilev]

On Sat, 20 Aug 2022 02:41:50 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Split the java/util/stream/SpinedBufferTest.java test into two parts:
>> - java/util/stream/SpinedBufferTest1.java has the first 6 test cases
>> - java/util/stream/SpinedBufferTes2.java has the second 6 test cases
>> 
>> I couldn't figure out a way to set a larger timeout value for the entirety
>> of java/util/stream/SpinedBufferTest.java and I saw that other folks solved
>> this problem with testng tests by splitting the test into more parts.
>> 
>> This fix is being tested in my jdk-20+10 stress testing run.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Refactor SpinedBufferTest.java by type.

Looks much better, I have only a few Java stylistic nits.

test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestCommon.java line 27:

> 25: import java.util.*;
> 26: 
> 27: public class SpinedBufferTestCommon {

Usually done like this:

Suggestion:

public abstract class AbstractSpinedBufferTest {

test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestCommon.java line 30:

> 28: 
> 29:     // Create sizes around the boundary of spines
> 30:     static List<Integer> sizes;

Pre-existing, but code style:

Suggestion:

    static final List<Integer> SIZES;

test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestCommon.java line 43:

> 41:         catch (Exception e) {
> 42:             e.printStackTrace();
> 43:         }

The usual way to terminate the clinit "cleanly" is:

Suggestion:

        } catch (Exception e) {
            throw new IllegalStateException(e);
        }


This allows you to `final` out the `SIZES` field, as dataflow would definitely show the class initialization fails if field init did not happen.

test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestDouble.java line 35:

> 33: 
> 34: @Test
> 35: public class SpinedBufferTestDouble extends SpinedBufferTestCommon {

Here and later, I think the common naming convention is having the "Test" at the end, i.e.:

Suggestion:

public class SpinedBufferDoubleTest extends SpinedBufferTestCommon {

test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestDouble.java line 36:

> 34: @Test
> 35: public class SpinedBufferTestDouble extends SpinedBufferTestCommon {
> 36:     // DoubleSpinedBuffer

Unnecessary comment, I think?

test/jdk/java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTestInt.java line 94:

> 92:         PrimitiveIterator.OfInt it = sb.iterator();
> 93:         for (int i = 0; i < TEST_SIZE; i++)
> 94:             list2.add(it.nextInt());

Pre-existing? Here and later, let's keep the braces on:

Suggestion:

        for (int i = 0; i < TEST_SIZE; i++) {
            list2.add(it.nextInt());
        }

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

PR: https://git.openjdk.org/jdk/pull/9845