Skip to content

Commit 85ef84a

Browse files
trasktrask
committed
Prefer parameterized tests when they are a good fit
1 parent 3b2944f commit 85ef84a

4 files changed

Lines changed: 362 additions & 617 deletions

File tree

‎.github/agents/knowledge/testing-general-patterns.md‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,36 @@
1212
- Do not use AssertJ `.as(...)` descriptions or `.withFailMessage(...)` in tests.
1313
Prefer direct assertions whose failure output shows the unexpected values.
1414

15+
## Parameterized Tests
16+
17+
- Prefer `@ParameterizedTest` when multiple tests in the same file exercise the same behavior
18+
with small input or expectation changes. This is a good fit when setup, action, and assertion
19+
shape are identical or nearly identical.
20+
- Do **not** force unrelated scenarios into a parameterized test when the setup diverges
21+
materially, the assertion logic branches heavily, or separate test names document distinct
22+
behavior more clearly.
23+
- Prefer the narrowest parameter source that keeps the test readable:
24+
`@ValueSource` / `@EnumSource` for a single simple input, `@CsvSource` for a few scalar input
25+
and expectation tuples, and `@MethodSource` for richer inputs such as objects, lambdas,
26+
callbacks, or larger expectation structures.
27+
- Prefer readable invocation names. When the default argument rendering would be noisy or
28+
unhelpful, use `Named` values in the method source and reference them from
29+
`@ParameterizedTest(name = "{0}")` instead of threading a separate display-name `String`
30+
through the test method signature.
31+
- If the raw argument values are already concise and readable — for example simple strings,
32+
enums, or small scalar tuples — prefer plain arguments over `Named`, and prefer the default
33+
parameterized-test invocation rendering unless a custom `name = ...` materially improves
34+
readability.
35+
- In mixed cases within the same file, apply that rule per source: keep `Named` for opaque
36+
values such as lambdas or method references, and use plain arguments for readable enums,
37+
strings, or other self-describing values.
38+
- Keep the parameterized test body focused on the shared behavior. Push case-specific data into
39+
the method source or a small helper instead of branching inside the test body.
40+
- When a method source is only used by one adjacent test or test cluster, place it immediately
41+
after that test cluster. When the same source is reused by several separated tests, or when a
42+
file has several sources that would interrupt the main test flow, keep the sources together at
43+
the bottom of the file.
44+
1545
## Test Resource Cleanup
1646

1747
- In JUnit tests, when an `AutoCloseable` is intended to remain live for most or all of the test

‎instrumentation/executors/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/executors/LambdaContextPropagationTest.java‎

Lines changed: 58 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,20 @@
88
import static java.util.Collections.singletonList;
99
import static java.util.concurrent.TimeUnit.SECONDS;
1010
import static org.assertj.core.api.Assertions.assertThat;
11+
import static org.junit.jupiter.api.Named.named;
1112

1213
import io.opentelemetry.api.baggage.Baggage;
1314
import io.opentelemetry.context.Scope;
14-
import java.util.ArrayList;
15-
import java.util.List;
15+
import java.util.Collections;
1616
import java.util.concurrent.Callable;
17-
import java.util.concurrent.ExecutionException;
1817
import java.util.concurrent.ExecutorService;
1918
import java.util.concurrent.Executors;
2019
import java.util.concurrent.atomic.AtomicInteger;
20+
import java.util.stream.Stream;
2121
import org.junit.jupiter.api.BeforeEach;
22-
import org.junit.jupiter.api.Test;
22+
import org.junit.jupiter.params.ParameterizedTest;
23+
import org.junit.jupiter.params.provider.Arguments;
24+
import org.junit.jupiter.params.provider.MethodSource;
2325

2426
// regression test for:
2527
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/9175
@@ -34,125 +36,70 @@ void reset() {
3436
failureCounter.set(0);
3537
}
3638

37-
@Test
38-
void propagateContextExecuteRunnable() throws InterruptedException {
39+
@ParameterizedTest(name = "{0}")
40+
@MethodSource("executorInvocations")
41+
void propagateContext(ExecutorInvocation invocation) throws Exception {
3942
ExecutorService executor = Executors.newSingleThreadExecutor();
4043

41-
Baggage baggage = Baggage.builder().put("test", "test").build();
42-
try (Scope ignored = baggage.makeCurrent()) {
43-
for (int i = 0; i < 20; i++) {
44-
executor.execute(LambdaContextPropagationTest::assertBaggage);
45-
}
46-
}
47-
48-
executor.shutdown();
49-
executor.awaitTermination(30, SECONDS);
50-
51-
assertThat(failureCounter).hasValue(0);
52-
}
53-
54-
@Test
55-
void propagateContextSubmitRunnable() throws InterruptedException {
56-
ExecutorService executor = Executors.newSingleThreadExecutor();
57-
58-
Baggage baggage = Baggage.builder().put("test", "test").build();
59-
try (Scope ignored = baggage.makeCurrent()) {
60-
for (int i = 0; i < 20; i++) {
61-
executor.submit(LambdaContextPropagationTest::assertBaggage);
62-
}
63-
}
64-
65-
executor.shutdown();
66-
executor.awaitTermination(30, SECONDS);
67-
68-
assertThat(failureCounter).hasValue(0);
69-
}
70-
71-
@Test
72-
void propagateContextSubmitRunnableAndResult() throws InterruptedException {
73-
ExecutorService executor = Executors.newSingleThreadExecutor();
74-
75-
Baggage baggage = Baggage.builder().put("test", "test").build();
76-
try (Scope ignored = baggage.makeCurrent()) {
77-
for (int i = 0; i < 20; i++) {
78-
executor.submit(LambdaContextPropagationTest::assertBaggage, null);
44+
try {
45+
Baggage baggage = Baggage.builder().put("test", "test").build();
46+
try (Scope ignored = baggage.makeCurrent()) {
47+
for (int i = 0; i < 20; i++) {
48+
invocation.invoke(executor);
49+
}
7950
}
51+
} finally {
52+
executor.shutdown();
53+
executor.awaitTermination(30, SECONDS);
8054
}
8155

82-
executor.shutdown();
83-
executor.awaitTermination(30, SECONDS);
84-
8556
assertThat(failureCounter).hasValue(0);
8657
}
8758

88-
@Test
89-
void propagateContextSubmitCallable() throws InterruptedException {
90-
ExecutorService executor = Executors.newSingleThreadExecutor();
91-
92-
Baggage baggage = Baggage.builder().put("test", "test").build();
93-
try (Scope ignored = baggage.makeCurrent()) {
94-
for (int i = 0; i < 20; i++) {
95-
Callable<?> callable =
96-
() -> {
97-
assertBaggage();
98-
return null;
99-
};
100-
executor.submit(callable);
101-
}
102-
}
103-
104-
executor.shutdown();
105-
executor.awaitTermination(30, SECONDS);
106-
107-
assertThat(failureCounter).hasValue(0);
59+
private static Stream<Arguments> executorInvocations() {
60+
return Stream.of(
61+
Arguments.of(
62+
named(
63+
"execute runnable",
64+
(ExecutorInvocation)
65+
executor -> executor.execute(LambdaContextPropagationTest::assertBaggage))),
66+
Arguments.of(
67+
named(
68+
"submit runnable",
69+
(ExecutorInvocation)
70+
executor -> executor.submit(LambdaContextPropagationTest::assertBaggage))),
71+
Arguments.of(
72+
named(
73+
"submit runnable and result",
74+
(ExecutorInvocation)
75+
executor ->
76+
executor.submit(LambdaContextPropagationTest::assertBaggage, null))),
77+
Arguments.of(
78+
named(
79+
"submit callable",
80+
(ExecutorInvocation) executor -> executor.submit(baggageCallable()))),
81+
Arguments.of(
82+
named(
83+
"invoke all",
84+
(ExecutorInvocation)
85+
executor -> executor.invokeAll(Collections.nCopies(20, baggageCallable())))),
86+
Arguments.of(
87+
named(
88+
"invoke any",
89+
(ExecutorInvocation)
90+
executor -> executor.invokeAny(singletonList(baggageCallable())))));
10891
}
10992

110-
@Test
111-
void propagateContextInvokeAll() throws InterruptedException {
112-
ExecutorService executor = Executors.newSingleThreadExecutor();
113-
114-
Baggage baggage = Baggage.builder().put("test", "test").build();
115-
try (Scope ignored = baggage.makeCurrent()) {
116-
for (int i = 0; i < 20; i++) {
117-
Callable<Void> callable =
118-
() -> {
119-
assertBaggage();
120-
return null;
121-
};
122-
List<Callable<Void>> callables = new ArrayList<>();
123-
for (int j = 0; j < 20; j++) {
124-
callables.add(callable);
125-
}
126-
executor.invokeAll(callables);
127-
}
128-
}
129-
130-
executor.shutdown();
131-
executor.awaitTermination(30, SECONDS);
132-
133-
assertThat(failureCounter).hasValue(0);
93+
private static Callable<Void> baggageCallable() {
94+
return () -> {
95+
assertBaggage();
96+
return null;
97+
};
13498
}
13599

136-
@Test
137-
void propagateContextInvokeAny() throws InterruptedException, ExecutionException {
138-
ExecutorService executor = Executors.newSingleThreadExecutor();
139-
140-
Baggage baggage = Baggage.builder().put("test", "test").build();
141-
try (Scope ignored = baggage.makeCurrent()) {
142-
for (int i = 0; i < 20; i++) {
143-
Callable<?> callable =
144-
() -> {
145-
assertBaggage();
146-
return null;
147-
};
148-
executor.invokeAny(singletonList(callable));
149-
}
150-
}
151-
152-
executor.shutdown();
153-
executor.awaitTermination(30, SECONDS);
154-
155-
assertThat(failureCounter).hasValue(0);
100+
@FunctionalInterface
101+
private interface ExecutorInvocation {
102+
void invoke(ExecutorService executor) throws Exception;
156103
}
157104

158105
private static void assertBaggage() {

‎instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/internal/engine/AttributeExtractorTest.java‎

Lines changed: 29 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import static org.assertj.core.api.Assertions.assertThat;
99

1010
import java.util.Set;
11+
import java.util.stream.Stream;
1112
import javax.management.MBeanServer;
1213
import javax.management.MBeanServerFactory;
1314
import javax.management.ObjectName;
@@ -16,6 +17,8 @@
1617
import org.junit.jupiter.api.BeforeEach;
1718
import org.junit.jupiter.api.Test;
1819
import org.junit.jupiter.params.ParameterizedTest;
20+
import org.junit.jupiter.params.provider.Arguments;
21+
import org.junit.jupiter.params.provider.MethodSource;
1922
import org.junit.jupiter.params.provider.ValueSource;
2023

2124
class AttributeExtractorTest {
@@ -127,100 +130,30 @@ void testSetup() {
127130
assertThat(set).isNotNull().hasSize(1).contains(objectName);
128131
}
129132

130-
@Test
131-
void testByteAttribute() {
132-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("ByteAttribute");
133-
AttributeInfo info = extractor.getAttributeInfo(theServer, objectName);
134-
assertThat(info).isNotNull();
135-
assertThat(info.usesDoubleValues()).isFalse();
136-
}
137-
138-
@Test
139-
void testByteAttributeValue() {
140-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("ByteAttribute");
141-
Number number = extractor.extractNumericalAttribute(theServer, objectName);
142-
assertThat(number).isNotNull();
143-
assertThat(number.longValue()).isEqualTo(10);
144-
}
145-
146-
@Test
147-
void testShortAttribute() {
148-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("ShortAttribute");
149-
AttributeInfo info = extractor.getAttributeInfo(theServer, objectName);
150-
assertThat(info).isNotNull();
151-
assertThat(info.usesDoubleValues()).isFalse();
152-
}
153-
154-
@Test
155-
void testShortAttributeValue() {
156-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("ShortAttribute");
157-
Number number = extractor.extractNumericalAttribute(theServer, objectName);
158-
assertThat(number).isNotNull();
159-
assertThat(number.longValue()).isEqualTo(11);
160-
}
161-
162-
@Test
163-
void testIntAttribute() {
164-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("IntAttribute");
165-
AttributeInfo info = extractor.getAttributeInfo(theServer, objectName);
166-
assertThat(info).isNotNull();
167-
assertThat(info.usesDoubleValues()).isFalse();
168-
}
169-
170-
@Test
171-
void testIntAttributeValue() {
172-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("IntAttribute");
173-
Number number = extractor.extractNumericalAttribute(theServer, objectName);
174-
assertThat(number).isNotNull();
175-
assertThat(number.longValue()).isEqualTo(12);
176-
}
177-
178-
@Test
179-
void testLongAttribute() {
180-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("LongAttribute");
133+
@ParameterizedTest
134+
@MethodSource("numericAttributeArgs")
135+
void testNumericAttributeInfo(
136+
String attributeName, boolean usesDoubleValues, Number expectedValue) {
137+
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName(attributeName);
181138
AttributeInfo info = extractor.getAttributeInfo(theServer, objectName);
182-
assertThat(info).isNotNull();
183-
assertThat(info.usesDoubleValues()).isFalse();
184-
}
185-
186-
@Test
187-
void testLongAttributeValue() {
188-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("LongAttribute");
189-
Number number = extractor.extractNumericalAttribute(theServer, objectName);
190-
assertThat(number).isNotNull();
191-
assertThat(number.longValue()).isEqualTo(13);
192-
}
193139

194-
@Test
195-
void testFloatAttribute() {
196-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("FloatAttribute");
197-
AttributeInfo info = extractor.getAttributeInfo(theServer, objectName);
198140
assertThat(info).isNotNull();
199-
assertThat(info.usesDoubleValues()).isTrue();
141+
assertThat(info.usesDoubleValues()).isEqualTo(usesDoubleValues);
200142
}
201143

202-
@Test
203-
void testFloatAttributeValue() {
204-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("FloatAttribute");
144+
@ParameterizedTest
145+
@MethodSource("numericAttributeArgs")
146+
void testNumericAttributeValue(
147+
String attributeName, boolean usesDoubleValues, Number expectedValue) {
148+
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName(attributeName);
205149
Number number = extractor.extractNumericalAttribute(theServer, objectName);
206-
assertThat(number).isNotNull();
207-
assertThat(number.doubleValue()).isEqualTo(14.0); // accurate representation
208-
}
209150

210-
@Test
211-
void testDoubleAttribute() {
212-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("DoubleAttribute");
213-
AttributeInfo info = extractor.getAttributeInfo(theServer, objectName);
214-
assertThat(info).isNotNull();
215-
assertThat(info.usesDoubleValues()).isTrue();
216-
}
217-
218-
@Test
219-
void testDoubleAttributeValue() {
220-
BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName("DoubleAttribute");
221-
Number number = extractor.extractNumericalAttribute(theServer, objectName);
222151
assertThat(number).isNotNull();
223-
assertThat(number.doubleValue()).isEqualTo(15.0); // accurate representation
152+
if (usesDoubleValues) {
153+
assertThat(number.doubleValue()).isEqualTo(expectedValue.doubleValue());
154+
} else {
155+
assertThat(number.longValue()).isEqualTo(expectedValue.longValue());
156+
}
224157
}
225158

226159
@Test
@@ -266,4 +199,14 @@ void testNegativeFilter(String attributeName) {
266199
.describedAs("negative value should be filtered")
267200
.isNull();
268201
}
202+
203+
private static Stream<Arguments> numericAttributeArgs() {
204+
return Stream.of(
205+
Arguments.of("ByteAttribute", false, 10L),
206+
Arguments.of("ShortAttribute", false, 11L),
207+
Arguments.of("IntAttribute", false, 12L),
208+
Arguments.of("LongAttribute", false, 13L),
209+
Arguments.of("FloatAttribute", true, 14.0),
210+
Arguments.of("DoubleAttribute", true, 15.0));
211+
}
269212
}

0 commit comments

Comments
 (0)