CODEC-285 JUnit v5 parallel assertions

[nhojpatrick]

Save debugging headache or re-execution if something is failing.

[kinow]

Merged your other PR's, so git conflicts now slightly_smiling_face

Ah! Spoke too soon!

[garydgregory]

I think we should pass on these types of changes. It makes debugging and maintenance harder IMO. The tests are harder to comprehend. -1 from me.

[nhojpatrick]

I think we should pass on these types of changes. It makes debugging and maintenance harder IMO. The tests are harder to comprehend. -1 from me.

Okay gary is this is another improvement you don't wish, I'll stop suggesting these. You can refactor this suggestion again if you want or I can just close the PR. But it does make fixing tests easier for most people once you up skill in JUnit v5.

[aherbert]

@garydgregory I disagree with the test being harder to comprehend. This actually seems like a useful addition as all assertions will run and you get a report on which failed. The classic pattern is to fix the first failing test, rerun, fix the next failing test, rerun, etc until all pass in serial fashion.

With this change you can apply a fix knowing what is currently failing from all test criteria, not just the first one.

In terms of source readability this just puts all assertions inside a block.

[garydgregory]

The current style we use is to have one method fail fast, if the original author of that test wanted to have one potential failure for different parts of that method, then they could have split the method with meaningful names, which is nice as it also provides some level of documentation. Using assertAll escapes a method from failing fast which then means we have two styles of reporting and causes failing builds to take longer, which uses up more resources and ends up producing no jars.

If you really want to help Apache Commons use more modern Junit, then I would suggest looking at Commons VFS where some tests are still stuck on Junit 3.

[aherbert]

As usual, there is more than one way to do things. A lot of these tests are repeat tests with different arguments thus more suitable for a ParameterizedTest with only one assertion per test method. Legacy tests with multiple assertions per test method were obviously limited to the support in the test framework when written. It is noisy to write classes using:

testConditionWithArgument1
testConditionWithArgument2
testConditionWithArgument3

So they are all in a single testCondition method, sometimes with the same assertion in a loop.

If the assertions are not serially conditional on the previous statements then using some level of parallelism should be preferred to isolate exactly what works and does not work. I would argue that fail fast is not as useful as a complete listing of what is broken. This is why we run all test classes and all test methods instead of failing on the first assertion that errors.

[garydgregory]

A parameterized test seems like the way to go here instead of anything "parallel" which if truly parallel is very bad as it lacks determinism. We want reproducible builds.