Use parameterized tests instead of repeated methods

metrics-lib should use parameterized tests (cf. an example in CollecTor)

Benefits are a clear separation of API structure tests and varying data, a reduction of the number of test methods, which leads to easier and more readable test classes. This also opens the possibility of introducing randomized testing (later).

added component::metrics/library metrics-2018 owner::metrics-team priority::medium severity::normal status::new type::enhancement labels

Capitalize summary.

Trac:
Summary: use parameterized tests instead of repeated methods to Use parameterized tests instead of repeated methods

Trac:
Keywords: N/A deleted, metrics-2018 added

Here is an example from CollecTor's code base using parametrized tests.

And, the most recent change there shows that for a new descriptor type only new test data needed to be added:

diff --git a/src/test/java/org/torproject/collector/sync/SyncPersistenceTest.java b/src/test/java/org/torproject/collector/sync/SyncPersistenceTest.java
index fe47f26..f0d30bc 100644
--- a/src/test/java/org/torproject/collector/sync/SyncPersistenceTest.java
+++ b/src/test/java/org/torproject/collector/sync/SyncPersistenceTest.java
@@ -43,6 +43,12 @@ public class SyncPersistenceTest {
          Integer.valueOf(1), // expected recent count of descs files
          Integer.valueOf(1)}, // expected output count of descs files
 
+        {"torperf/op-nl-1048576-2017-04-11.tpf",
+         new String[]{"torperf/2017/04/11/op-nl-1048576-2017-04-11.tpf"},
+         "op-nl-1048576-2017-04-11.tpf",
+         Integer.valueOf(1),
+         Integer.valueOf(1)},
+

I mostly agree that parameterized tests make the code more readable. However, there's one thing that makes them a bit less readable: without a test method name, whoever reads the code needs to guess why given test input produces given test output. That's not always obvious just by looking at the data.

As a (truly) random example, look at the various testSearchBase64* methods here. And now imagine how this would look like without test method names. Sure, it would be shorter. But would you still understand that the input data for testSearchBase64HashedFingerprintTorkaZ is the hashed fingerprint of one of the example relays? Stated differently, from looking at the data alone, would you be able to tell whether a given test case is already covered by existing tests or not?

I guess when we move to parameterized tests we should try to preserve the information that is currently contained in test method names. It could be in Java comments, though they might not be used or updated in practice. It could also be a String in yet another parameter (maybe first?) which might be printed out by JUnit in case of a failing test. Are there best practices for this issue, or how do others solve this?

Another potential problem is that, AFAIU, we'll have to split each (or at least most) of the current test classes into one or more that share parameters and another one that is non-parameterized. Should be okay, but we should ideally have a plan for this before moving around all existing unit test classes.

Replying to karsten:

I mostly agree that parameterized tests make the code more readable. However, there's one thing that makes them a bit less readable: without a test method name, whoever reads the code needs to guess why given test input produces given test output. That's not always obvious just by looking at the data.

Well, the comments/descriptions can easily be added to the test data.

As a (truly) random example, look at the various testSearchBase64* methods here. And now imagine how this would look like without test method names. Sure, it would be shorter. But would you still understand that the input data for testSearchBase64HashedFingerprintTorkaZ is the hashed fingerprint of one of the example relays? Stated differently, from looking at the data alone, would you be able to tell whether a given test case is already covered by existing tests or not?

The methods names hint about the test's purpose, but are not really very helpful for the outside reader, being one word camel case.

I guess when we move to parameterized tests we should try to preserve the information that is currently contained in test method names. It could be in Java comments, though they might not be used or updated in practice. It could also be a String in yet another parameter (maybe first?) which might be printed out by JUnit in case of a failing test. Are there best practices for this issue, or how do others solve this?

(answered above, add the description string to the data)

Another potential problem is that, AFAIU, we'll have to split each (or at least most) of the current test classes into one or more that share parameters and another one that is non-parameterized. Should be okay, but we should ideally have a plan for this before moving around all existing unit test classes.

This is a valid point and will take some time structuring the changes. One immediate improvement would be adding a test description to the @Test annotation. These could be re-used later.