RFR: 8341566: Add Reader.of(CharSequence) [v6]

[Markus KARG]

On Wed, 9 Oct 2024 07:21:48 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Markus KARG has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Improved wording: 'If the sequence changes while the reader is open, e.g. the length changes, the behavior is undefined.'
>
> test/jdk/java/io/Reader/Of.java line 40:
> 
>> 38:  * @bug 8341566
>> 39:  * @run testng Of
>> 40:  * @summary Check for expected behavior of Reader.of().
> 
> Nit - the jtreg documentation recommends the following order of these tags https://openjdk.org/jtreg/tag-spec.html#ORDER. For newly introduced tests, like this one, it will be good to follow that recommendation.

done

> test/jdk/java/io/Reader/Of.java line 42:
> 
>> 40:  * @summary Check for expected behavior of Reader.of().
>> 41:  */
>> 42: public class Of {
> 
> Would it be possible to include a test which creates and operates on a `Reader` which is backed by a `CharSequence` of zero length. Similarly, a test which creates and operates on a `Reader` which is backed by a `CharSequence` of `Integer.MAX_VALUE` length? I think that should give us good coverage for the implementation of the returned `Reader` for these corner cases.

Zero-case is partically covered by the existing test code. Your request looks like a real lot of work, as it implies a complete rewrite of the overall structure of the test - and frankly spoken, my budget for this issue is near to spent already. Does that *really* provide a justifying benefit, given the fact that these are literally edge cases (and given the fact, that the code is a 1:1 copy of `StringReader`, which we all used since decades, including those edge cases)? If you think it is really necessary, can you please post the modified version of `Of.java` here? I will then post it into the PR. Thanks.

> test/jdk/java/io/Reader/Of.java line 168:
> 
>> 166:     }
>> 167: 
>> 168:     @Test(dataProvider = "readers", expectedExceptions = IOException.class)
> 
> Instead of `expectedExceptions` at the method level, I think it would be better to use something like:
> 
> 
> org.testng.Assert.assertThrows(IOException.class,
>                                () -> {reader.read();});
> 
> That way it's clear which operation within the test method is expected to throw this exception and it also prevents unexpected `IOException` (for example, from reader.close()) from going unnoticed.

done.

Does it makes sense to keep the close tests as separate methods then? IMHO their sole existence was due to using `expectedExceptions` instead of `assertThrows`. So now that we *have* `assertThrows`, shall we move the test code itself at the end of the non-close test methods simply? 🤔

> test/jdk/java/io/Reader/Of.java line 174:
> 
>> 172:     }
>> 173: 
>> 174:     @Test(dataProvider = "readers", expectedExceptions = IOException.class)
> 
> Same comment as above, here and the other test methods, about using assertThrows instead of expectedExceptions at the method level.

done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1793220813
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1793313049
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1793236618
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1793236979