Opened 2 years ago

Last modified 2 years ago

#22733 new enhancement

Use parameterized tests instead of repeated methods

Reported by: iwakeh Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords: metrics-2018
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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).

Child Tickets

Change History (5)

comment:1 Changed 2 years ago by karsten

Summary: use parameterized tests instead of repeated methodsUse parameterized tests instead of repeated methods

Capitalize summary.

comment:2 Changed 2 years ago by karsten

Keywords: metrics-2018 added

comment:3 Changed 2 years ago by iwakeh

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)},
+

comment:4 Changed 2 years ago by 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.

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.

comment:5 in reply to:  4 Changed 2 years ago by iwakeh

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.

Last edited 2 years ago by iwakeh (previous) (diff)
Note: See TracTickets for help on using tickets.