refactor(tests): Use @ParameterizedTest instead of looping over test data

[FlorianKroiss]

Followup to #1403

[sebthom]

See inline comment

[sebthom]

I took another look at this, and I'm now hesitant to merge it. The parameterization doesn't seem to give us much benefit here:

• The overall amount of code is about the same as before.
• The test data is now decoupled from the test method and exposed as a separate public static field in the middle of the class.
• Using arguments() removes type safety - the old inline record feels safer and clearer.
• We now need to repeat the default arguments ("", 0, 0) for almost every test case, which adds noise and makes the test harder to read.

Overall, this feels like a step backward in readability and maintainability. I'm not convinced the migration to @ParameterizedTest buys us anything meaningful here.

@rubenporras WDYT?

[FlorianKroiss]

The overall amount of code is about the same as before.

Code reduction was not the goal here. I prefer parameterized tests over regular tests with loops because the former gives you much better test feedback. Imagine if the test fails for 3 out of 10 inputs. With the current setup, the test would fail with the first invalid input and you don't know if it fails for any other inputs. With parameterized tests you would immediately see which inputs have failed, because they are all executed, regardless of whether another test input has generated an error.

The test data is now decoupled from the test method and exposed as a separate public static field in the middle of the class.

Fair point, but that just the design of parameterized tests in JUnit. I usually place the test data right above the test method and give both the same name, which I think is not too bad for readability.

• Using arguments() removes type safety - the old inline record feels safer and clearer.
• We now need to repeat the default arguments ("", 0, 0) for almost every test case, which adds noise and makes the test harder to read.

We can use the existing record instead of Arguments, which keeps type safety and enables default values. I did intentionally not do this because this comes at the cost that a record, which is specific to a single test, is now visible to the test class, whereas before it was only visible in the test method itself.

[sebthom]

With parameterized tests you would immediately see which inputs have failed, because they are all executed, regardless of whether another test input has generated an error.

That is a good argument.

We can use the existing record instead of Arguments, which keeps type safety and enables default values. I did intentionally not do this because this comes at the cost that a record, which is specific to a single test, is now visible to the test class, whereas before it was only visible in the test method itself.

Yes I would prefer the type-safe record but I also would not like to have it visible to the other test methods. I think the problem here is that the test class is too large. Maybe if the parameterized tests should live in a dedicated test class with a single test method. then it doesn't matter to have the parameters in a static field and having a record for the parameters.

Let's wait what @rubenporras has to say.

[rubenporras]

Thanks for the idea of using parameterized test now that you upgraded to JUnit 5 and providing the implementation. I did not know the feature at all.

For me, since this is a test class, as long as the test runs nicely, I would be fine with the current implementation or having the type-safe record (in this on a new dedicated class). For me I would merge as it is. If @sebthom prefers the test in its own class so that we can have the record, I would also go along. @sebthom: do you think the extra round on the PR is worth it? If so, I would suggest we aim for that.