RFR: 8273507: Convert test/jdk/java/nio/channels/Channels/TransferTo.java to TestNG test [v2]

[Markus KARG]

On Thu, 9 Sep 2021 14:13:14 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> Markus KARG has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Using assert* methods instead of fail() and expectedExceptions
>>   
>>   Signed-off-by: Markus Karg <markus at headcrashing.eu>
>
>> > I would add a comment for for each DataProvider/Test to give future maintainers an overview of the intent of the methods
>> 
>> I support the general idea of making things more comprehensible, but I will abstain from adding comments in _this_ PR due to three reasons:
>> 
>> * This PR _solely_ has the target of technically introducing **TestNG** and I really do not want to mix concerns like technical merits with documental merits.
>> * The current PR is a blocker in the _critical path_ of other, higher prioritized PRs. We should try not to slow it down _unnecessarily_.
>> * I dislike code comments and would instead prefer to turn this source code into **BDD** style, i. e. changing the test name into "shouldDoThisInThatCase" style and introducing "given", "when" and "then" sections inside the tests. As this source code is a copy of two other original files, I will do it for all three files in one shot to keep their inherent relationship.
>> 
>> So in the best sense of KISS, SoC and "release early, release often", it makes sense that I will create an (or mutiple) explicit, subsequent PR(s) for introducing BDD instead of adding code comments in the current (slow and big) PR.
> 
> We are going to have to agree to disagree here.   Comments are/should be part of the development process including for tests.   There is no reason to create additional issues or new PRs given the tests is already being modified.  
> 
>> > I would add a comment for for each DataProvider/Test to give future maintainers an overview of the intent of the methods
>> 
>> I support the general idea of making things more comprehensible, but I will abstain from adding comments in _this_ PR due to three reasons:
>> 
>> * This PR _solely_ has the target of technically introducing **TestNG** and I really do not want to mix concerns like technical merits with documental merits.
>> * The current PR is a blocker in the _critical path_ of other, higher prioritized PRs. We should try not to slow it down _unnecessarily_.
>> * I dislike code comments and would instead prefer to turn this source code into **BDD** style, i. e. changing the test name into "shouldDoThisInThatCase" style and introducing "given", "when" and "then" sections inside the tests. As this source code is a copy of two other original files, I will do it for all three files in one shot to keep their inherent relationship.
>> 
>> So in the best sense of KISS, SoC and "release early, release often", it makes sense that I will create an (or mutiple) explicit, subsequent PR(s) for introducing BDD instead of adding code comments in the current (slow and big) PR.
> 
> We are going to agree to disagree on this.  There is no reason to create additional issues/PRs for improving the readability/documentation.  Comments do not have to be extensive but enough to introduce the intent of the test.    Now is the right time to add comments given the test is being updated.   
> 
> As we evolve/maintain OpenJDK, we want to keep maintainability for current and future developers in mind.  Adding documentation/comments while we are updating/adding to the project will help in this endeavor (and I know it would have helped me while navigating areas of the codebase I was less familiar with).

@LanceAndersen requested:
> I would add a comment for for each DataProvider/Test to give future maintainers an overview of the intent of the methods

Done by https://github.com/openjdk/jdk/pull/5421/commits/b08c5a0a545bad9168ac0afae3d46551658f6ee1.

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

PR: https://git.openjdk.java.net/jdk/pull/5421