Skip to content

Conversation

Monilnarang
Copy link

Summary:

  • Parameterized tests : type and changed name to testType

Elaboration:

  • The same method call (objects.get(x).type()) is repeated multiple times with different inputs, making it redundant and the test harder to maintain and extend.
  • When a test fails, JUnit only shows which assertion failed, but not which specific input caused the failure.
  • Adding new test cases requires copying and pasting another line of assertion statement instead of simply adding new data.

To accomplish this, I retrofitted this tests into a parameterized unit test. This reduces duplication, allows easy extension by simply adding new value sets, and makes debugging easier as it clearly indicates which test failed instead of requiring a search through individual assertions.
Also, when run each value set is shown its separate pass/fail status.

Copy link

@bchapuis
Copy link
Member

bchapuis commented Apr 16, 2025

Thanks a lot for taking the time to open this PR.

I usually wait to use parameterized tests until there are a lot of test cases and the repeated code starts to get messy. When there are only a few checks, I think regular test lines are easier to read and understand. Parameterized tests can make things harder to follow if they don't really help with keeping the code cleaner or easier to update. In this case, we're just checking the content of a single record at a few positions, so parametrized tests may be an overkill.

Here is another example of where we used a provider for parametried test in baremaps:

https://github.com/apache/incubator-baremaps/blob/main/baremaps-data/src/test/java/org/apache/baremaps/data/type/DataTypeProvider.java

If you are interested in contributing to apache baremaps, please do not hesitate to share your usecase on the mailing list or to start with one of the "good first issues". We will be happy to help.

https://github.com/apache/incubator-baremaps/issues?q=is%3Aissue%20state%3Aopen%20label%3A%22good%20first%20issue%22

@bchapuis bchapuis force-pushed the main branch 3 times, most recently from e070c9e to 2c93e18 Compare May 24, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants