Update contributing docs to mention AssertJ / JUnit5

[nastra]

No description provided.

[emkornfield]

I think one thing is not clear to me there is a goal if we want to rewrite all existing tests to use AssertJ (my impression is this is probably low value in terms of contributor/reviewer time).

[wgtmac]

I think this is a common issue for Java projects that migrate to JUnit 5. Rewriting all existing tests takes a lot of effort even using AI.

[nastra]

yes, this is definitely a common issue when migrating. What we did in the Iceberg project is that we mandated new tests to be written purely with JUnit5 + AssertJ and incrementally converted things over from legacy Junit4 assertions to AssertJ assertions. Once that was done, the move from Junit4 to JUnit5 was much easier.

While this might seem as low value it is actually a great place where new people can contribute to the project. On the Iceberg side I was mostly involved in reviewing and guiding the migration while new contributors helped with all of the code changes.

I think as long as there's interest from the project to modernize the codebase and there's a reviewer willing to invest some time, the contributions will just happen naturally over time.

[alamb]

Thank you for this PR @nastra and the review @emkornfield

Rather than adding documentation for the java implementation to this site, I think they would be more discoverable if they lived with the code / github flow for that other repo.

So what do you think about putting the details of contributing to parquet-java into a CONTRIBUTING.md or similar page in the parquet-java repo https://github.com/apache/parquet-java ?

[nastra]

Rather than adding documentation for the java implementation to this site, I think they would be more discoverable if they lived with the code / github flow for that other repo.

So what do you think about putting the details of contributing to parquet-java into a CONTRIBUTING.md or similar page in the parquet-java repo https://github.com/apache/parquet-java ?

I was checking the parquet-java codebase but this file was the one that is shown on the website and the title of the file says Contributing to Parquet-Java (https://github.com/apache/parquet-site/pull/190/changes#diff-941e9b19876340449cc889dd6c8e8ad84f98598c1b70a2a5682bdbcf5fcdb172R2-R3) so this seemed to be the right place to add this

[alamb]

Rather than adding documentation for the java implementation to this site, I think they would be more discoverable if they lived with the code / github flow for that other repo.
So what do you think about putting the details of contributing to parquet-java into a CONTRIBUTING.md or similar page in the parquet-java repo https://github.com/apache/parquet-java ?

I was checking the parquet-java codebase but this file was the one that is shown on the website and the title of the file says Contributing to Parquet-Java (https://github.com/apache/parquet-site/pull/190/changes#diff-941e9b19876340449cc889dd6c8e8ad84f98598c1b70a2a5682bdbcf5fcdb172R2-R3) so this seemed to be the right place to add this

I agree with the current repo setup this is the best place -- we can potentially move the content to the parquet-java repo as a follow on PR

[wgtmac]

Should we just merge this PR as is since the contributing doc is already for parquet-java?

[alamb]

Should we just merge this PR as is since the contributing doc is already for parquet-java?

Yes I think that is a good idea. I'll make a follow on PR / proposal to move the content to the parquet-java repo

Update:

• tracking in Move parquet-java specific contributing content to parquet-java #194

[steveloughran]

FWIW I don't understand why team junit set out to break every single assert(), especially how assertEquals(message, expected, actual) still compiles but now the args become something else like assert(actual, expected, message).

assertJ just takes all the pain away.

Does break all pending PRs with tests though, such as my own #3562; forced an update and a rebase there...

[nastra]

FWIW I don't understand why team junit set out to break every single assert(), especially how assertEquals(message, expected, actual) still compiles but now the args become something else like assert(actual, expected, message).

my guess is because they realized that it's more natural to mentally define actual and then expected. I've also seen numerous cases across the parquet-java codebase where the actual/expected were flipped, which is easy to do JUnit assertions. With AssertJ assertions you're always defining the actual first and then verify that certain things hold on that object

assertJ just takes all the pain away.

Does break all pending PRs with tests though, such as my own #3562; forced an update and a rebase there...

This is true unfortunately that this will cause conflicts on pending PRs and I don't have a good solution on how to avoid that. In Iceberg we migrated module by module and had to deal with merge conflicts as well for a while, but the Iceberg codebase is larger (especially multiple versions of Flink + Spark modules). For parquet-java the migration should be much easier/quicker, so we could technically migrate the codebase to AssertJ in a short amount of time to reduce future conflicts on newer PRs. I already have most of the work done but haven't published all PRs yet, because I don't want to overwhelm reviewers.

[wgtmac]

I agree with @alamb's plan. Let's move on. Thanks @nastra!