Migrate tests to JUnit5

[strangelookingnerd]

This PR aims to migrate all tests to JUnit5.

Migration includes:

• Using JUnit5 annotations
• Using Assertions over Assert
• Remove public modifiers from test classes and methods
• Clean up assertions (switching expected and actual, simplyifing assertions...)
• Trivial code cleanup

I validated my changes using mvn verify which passes just fine.

[garydgregory]

Hello @strangelookingnerd

I appreciate the intent of this PR but half of these changes are not required to port to JUnit 5 and I stopped reviewing after a while. You'll find some comments in the PR though.

Please be aware that in our post-XZ Utils world of supply chain attacks, a maintainer MUST review every single line of code, not just eyeball a file. A PR like one of 45 files and hundreds if not thousands of changes puts a painful burden on a maintainer like myself.

With this in mind, please:

• Only change what's required
• There is no need to edit every single test file to change the visibility of classes of methods.
• There is no need to edit JDBC implementations of methods to remove SQLExceptions from method signatures, this has nothing to do with JUnit 5.

Thank you again for your contribution and your time.

Please:

• Review my comments
• Review your own work, don't apply search and replace without examining the results, I think you would have caught the indentation issues.
• Provide the smallest PR that does the job
• Run 'mvn' (by itself) to run all build checks (the default Maven goal); running 'mvn clean verify' is not enough ;-)

TY !

[strangelookingnerd]

Please be aware that in our post-XZ Utils world of supply chain attacks, a maintainer MUST review every single line of code, not just eyeball a file. A PR like one of 45 files and hundreds if not thousands of changes puts a painful burden on a maintainer like myself.

100% understand.

There is no need to edit every single test file to change the visibility of classes of methods.

It's considered best practice for JUnit5 and I find it useful as it for example excludes those classes and methods from auto-completion and removes the burden to add Javadoc to them. As most IDEs / code analysis tools require Javadoc on public classes and methods, we often time end up with useless, verbose information. Or at least I personally do not see the need to add something like this for every class

/**
 * Test the BasicRowProcessor class.
 */
public class BasicRowProcessorTest extends BaseTestCase {

/**
 * ColumnListHandlerTest
 */
public class ColumnListHandlerTest extends BaseTestCase {

My proposal would be to follow the best practice of reducing the visibility of test classes and methods and remove superfluous Javadoc as well. WDYT? Totally fine if you disagree, just let me know.

There is no need to edit JDBC implementations of methods to remove SQLExceptions from method signatures, this has nothing to do with JUnit 5.

Fair point, I put it under "trivial cleanup" since I was touching these methods anyway. I'll revert it.

Review your own work, don't apply search and replace without examining the results, I think you would have caught the indentation issues.

I am mostly applying OpenRewrite recipes and validate / adapt the results manually. No clue how the indention got messed up here but I will re-check for sure. Maybe adding spotless to the build would help.

Run 'mvn' (by itself) to run all build checks (the default Maven goal); running 'mvn clean verify' is not enough ;-)

In my defence, CONTRIBUTING.md actually states to run mvn clean verify.

[garydgregory]

Hello @strangelookingnerd

Thank you for the thoughtful reply.

The README, CONTRIBUTING, and other files are generated when we create a release candidate. These files are out of date, so I'll offer my apologies for that. I just pushed re-generated versions of the README and CONTRIBUTING files.

Let me offer you different POVs:

• This is a PR, it needs to be minimal, what JUnit calls a "best-practice" is really a style change from my POV and is not as important as maintainer time and sanity.
• If a maintained wants to, later, make all these noisy changes, then can do so, but, IMO it doesn't belong in a PR. If this project used RTC instead of CTR, then we'd have no choice, but we use CTR, so here we are ;-)
• A class Javadoc on a test class is important for several reasons IMO, and this is what I do all the time: I am navigating test classes, I hop into a "TestFoo" class and I want to jump to the subject class "Foo", all I have to do in my IDE (Eclipse), is click on the class name in the Javadoc comment that says "Tests {@link Foo}." and I'm there.
• Using the link tag makes it obvious to the reader what is under test, for example, "Tests {@link Foo} when Bar has been fiddled".
• The class Javadoc also offers a placeholder for other comments when a class may have dependencies on some interesting environment like "Uses Docker for this and that", or "Temporarily sets environment variables this and that".
• Having just a minimal Javadoc offers a place for maintainers and contributors to say something. I want to encourage: "Oh, yes, that's the place I should add my tidbit of knowledge about testing this class." As opposed to: "Well, there is no Javadoc, since no one seems to care about documenting how this works, I won't say anything either about the bell and whistle."
• It doesn't matter what tool is used (well, it matters if you use something that uses AI-generated stuff based on copyrighted material or sources that are incompatible with the Apache license), what matters is the PR and the changes it contains. I expect contributors to review their PRs, magically generated or hand-crafted. As a maintainer, I must review each line of code (XZ Utils). It's only fair to ask contributors to do the same ;-)

[strangelookingnerd]

Reverted the changes as you requested and left the bare minumum in.

[garydgregory]

Hello @strangelookingnerd

Thank you for updating the PR. Everything looks good.