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

[Lance Andersen]

On Thu, 9 Sep 2021 06:26:29 GMT, Markus KARG <github.com+1701815+mkarg at openjdk.org> wrote:

>> Using TestNG makes it easier to maintain and extend the unit test of ChannelInputStream::transferTo.
>> 
>> This change was proposed by Brian Burkhalter @blbp and requested Alan Bateman @AlanBateman.
>> 
>> *Note: A further test addition (testing 2GB+ transfers) will be added by me in a subsequent PR using TestNG once *this* PR is merged. This will need a while due to personal scheduling, also the topics are not necessarily related, hence there are separate PRs.*
>
> 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).

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

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