8364213: (bf) Improve java/nio/Buffer/GetChars.java test comments #26512
Conversation
Address code review comments which came in post integration/merge. Update/clarify test summary tag Document the values generated for arguments Move the endian-ness qualifier in the test descriptor string to not be first Change test method names to more closely match arg names of method under test
|
👋 Welcome back bokken! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
Webrevs
|
|
@AlanBateman - this is addressing your comments in prior review |
| * <li>int stop - index (exclusive) into char[] where the CharBuffer should be limited</li> | ||
| * <li>String - description of the test scenario</li> | ||
| * </ul> | ||
| */ |
There was a problem hiding this comment.
That helps a bit but I think we need to break up charBufferArguments into simpler/maintainable methods that have clear comments to document the test cases that they generate. Right now it's just way too hard to see what is tested and not tested.
There was a problem hiding this comment.
I have started by adding more documentation to the "random" tests.
I agree this makes it much easier to reason about what scenarios are covered.
It is actually making me wonder if the "static" tests are even worth keeping.
fix for random range generation (JDK-8364345)
|
/issue 8364345 |
|
@bokken |
| cb.position(start); | ||
| cb.limit(end); | ||
| return cb; | ||
| } | ||
|
|
||
| /** | ||
| * Generates random content to use for populating <i>cb</i> then calling through to {@code addCases(String, char[], CharBuffer, List)} |
There was a problem hiding this comment.
Try to keep the line lengths under 100 chars (80 is better).
It makes it possible to do size-by-size reviews that some reviewers prefer.
Address code review comments which came in post integration/merge of #26282.
Update/clarify test summary tag
Document the values generated for arguments
Move the endian-ness qualifier in the test descriptor string to not be first
Change test method names to more closely match arg names of method under test
Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26512/head:pull/26512$ git checkout pull/26512Update a local copy of the PR:
$ git checkout pull/26512$ git pull https://git.openjdk.org/jdk.git pull/26512/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26512View PR using the GUI difftool:
$ git pr show -t 26512Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26512.diff
Using Webrev
Link to Webrev Comment