Skip to content

Commit 9363ae1

Browse files
committed
Add testLatestDeps() helper method
1 parent d9fba53 commit 9363ae1

68 files changed

Lines changed: 331 additions & 134 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/agents/knowledge/general-rules.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
2727
| Semconv | Dual semconv testing | `SemconvStability`, `maybeStable`, semconv Gradle tasks | `testing-semconv-stability.md` |
2828
| Testing | General test patterns | Test files in scope — assertion style, test method signatures and throws clauses, resource cleanup, attribute assertions | `testing-general-patterns.md` |
2929
| Testing | Experimental flag tests | `testExperimental`, experimental attribute assertions, `experimental` flags in JVM args or system properties | `testing-experimental-flags.md` |
30+
| Testing | Flag-gated / mode-dependent assertion shape (experimental, `testLatestDeps`, semconv) — hoist flag into per-class constant, inline ternary, `if` for structural differences | Test classes branching on `EXPERIMENTAL_ATTRIBUTES`, `Boolean.getBoolean("testLatestDeps")`, or `emitOld*`/`emitStable*` | `testing-general-patterns.md` |
3031
| Library | TelemetryBuilder/getter/setter patterns | Library instrumentation classes | `library-patterns.md` |
3132
| API | Deprecation and breaking-change policy | Public API changes | `api-deprecation-policy.md` |
3233
| Config | Config property stability/renames/removals | `otel.instrumentation.*` property changes, `DeclarativeConfigUtil` or `ConfigProperties` usage | `config-property-stability.md` |

.github/agents/knowledge/testing-experimental-flags.md

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,30 +51,44 @@ val testExperimental by registering(Test::class) {
5151

5252
## Java Test Patterns
5353

54-
Read the flag once into a `private static final boolean` at class level:
54+
For the cross-cutting shape — inline ternary with `null` for "absent", when to use top-level
55+
`if` blocks, and `assumeTrue(...)` guidance — see
56+
[testing-general-patterns.md](testing-general-patterns.md#flag-gated--mode-dependent-assertions).
57+
The experimental-specific patterns below build on that shape.
58+
59+
### Hoist the flag into a per-class `EXPERIMENTAL_ATTRIBUTES` constant
60+
61+
Unlike `testLatestDeps()` / `emitStable*Semconv()`, there is no shared helper for
62+
experimental flags — the property name is module-specific. Read it once into a
63+
`private static final boolean` near the top of the class and reference the constant
64+
everywhere in the file:
5565

5666
```java
5767
private static final boolean EXPERIMENTAL_ATTRIBUTES =
5868
Boolean.getBoolean("otel.instrumentation.<module>.experimental-span-attributes");
5969
```
6070

61-
Use inline ternary in assertions — `null` means attribute expected absent:
71+
When multiple experimental flags coexist in one class, use a more specific name per flag.
72+
Replace every in-file occurrence of the raw `Boolean.getBoolean("…")` call with the
73+
constant — including plain `if`, `&&`, and `assumeTrue(...)`, not just inside ternaries.
6274

63-
```java
64-
equalTo(ExperimentalAttributes.SOME_ATTR, EXPERIMENTAL_ATTRIBUTES ? "value" : null)
65-
```
75+
### Single-arg `experimental(value)` helper
6676

67-
When many assertions share the flag, extract an `experimental()` helper:
77+
Experimental attributes are by definition absent when the flag is off, so the off-branch is
78+
always `null`. When several assertions in the same class gate attributes on
79+
`EXPERIMENTAL_ATTRIBUTES`, extract a tiny helper:
6880

6981
```java
7082
@Nullable
7183
private static <T> T experimental(T value) {
7284
return EXPERIMENTAL_ATTRIBUTES ? value : null;
7385
}
86+
87+
equalTo(SOME_KEY, experimental("value"))
7488
```
7589

7690
For multiple test classes sharing the same flag, move the helper into a shared
7791
`ExperimentalTestHelper` class and static-import it.
7892

79-
Use `assumeTrue(EXPERIMENTAL_ATTRIBUTES)` only when an entire test is meaningful in
80-
experimental mode only — prefer the ternary/helper pattern so both modes are exercised.
93+
Do **not** add a two-argument `experimental(on, off)` helper — for the rare case where the
94+
off-branch is non-null, an inline ternary is more readable.

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

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,3 +130,82 @@ unless the span ordering within a trace is genuinely non-deterministic (e.g., co
130130
producers/consumers, thread-pool fan-out, or channel interleaving). Sequential operations
131131
like `repeat {}` loops, single-child traces, and `flux` sequential emission produce spans
132132
in deterministic order — use `hasSpansSatisfyingExactly` for those.
133+
134+
## Flag-Gated / Mode-Dependent Assertions
135+
136+
Several test modes change which attributes, span names, status codes, or span indexes are
137+
expected:
138+
139+
- Experimental attributes (`-Dotel.instrumentation.<module>.experimental-*=true`) — see
140+
[testing-experimental-flags.md](testing-experimental-flags.md).
141+
- Semconv stability (`-Dotel.semconv-stability.opt-in=...`) — see
142+
[testing-semconv-stability.md](testing-semconv-stability.md).
143+
- `testLatestDeps` Gradle property — runs against the newest supported library versions
144+
instead of the pinned earliest-supported ones.
145+
146+
The subsections below cover the cross-cutting *shape* rules. Flag-specific tooling
147+
(helpers, constants, `/dup` handling) lives in the per-flag docs linked above.
148+
149+
### Read the flag through a shared static helper, not a per-class field
150+
151+
Each flag has a shared static accessor; static-import it and call it directly. Never call
152+
`Boolean.getBoolean("…")` inline, never duplicate the property-name string at the call
153+
site, and replace any per-class `TEST_LATEST_DEPS` constant with the shared accessor —
154+
including in plain `if`, `&&`, and `assumeTrue(...)`, not just inside ternaries.
155+
156+
```java
157+
import static io.opentelemetry.instrumentation.testing.util.TestLatestDeps.testLatestDeps;
158+
import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableDatabaseSemconv;
159+
160+
//
161+
162+
.hasName(testLatestDeps() ? "GET" : "HTTP GET")
163+
equalTo(DB_USER, emitStableDatabaseSemconv() ? null : USER_DB)
164+
```
165+
166+
| Flag | Shared accessor | Where it lives |
167+
| --- | --- | --- |
168+
| `-PtestLatestDeps=true` | `testLatestDeps()` | `io.opentelemetry.instrumentation.testing.util.TestLatestDeps` (testing-common) |
169+
| `otel.semconv-stability.opt-in=…` | `emitStableDatabaseSemconv()`, `emitOldDatabaseSemconv()`, `emitStableCodeSemconv()`, etc. | `io.opentelemetry.instrumentation.api.internal.SemconvStability` |
170+
| `otel.instrumentation.<module>.experimental-*` | per-module `EXPERIMENTAL_ATTRIBUTES` constant — see [testing-experimental-flags.md](testing-experimental-flags.md) | within the test class |
171+
172+
### Inline ternary in `equalTo(...)` with `null` for "absent"
173+
174+
Push the ternary as deep as possible — into the `equalTo` value or single attribute key —
175+
rather than duplicating two whole `hasAttributesSatisfyingExactly(...)` blocks under a
176+
`flag ? a : b`. The assertion API treats `null` as "expect attribute absent":
177+
178+
```java
179+
equalTo(DB_USER, emitStableDatabaseSemconv() ? null : USER_DB)
180+
equalTo(ERROR_TYPE, emitStableDatabaseSemconv() ? "42601" : null)
181+
equalTo(SOME_KEY, EXPERIMENTAL_ATTRIBUTES ? "value" : null)
182+
span.hasName(testLatestDeps() ? "GET" : "HTTP GET")
183+
.hasParent(trace.getSpan(testLatestDeps() ? 0 : 1))
184+
```
185+
186+
Do **not** introduce two-argument `flag(on, off)` helpers — the inline ternary is more
187+
readable. The only single-arg helper is `experimental(value)`, which only works because
188+
its off-branch is always `null` (see
189+
[testing-experimental-flags.md](testing-experimental-flags.md)).
190+
191+
### Use `if` (not `if/else`) when structure differs
192+
193+
When the *set* of attributes (count, keys) — not just values — differs between modes, use
194+
top-level `if` blocks rather than a ternary swapping the entire assertion list. For semconv
195+
this is **mandatory**: `/dup` mode must execute both branches, so never use `if/else`
196+
see [testing-semconv-stability.md](testing-semconv-stability.md) for details.
197+
198+
```java
199+
if (emitStableCodeSemconv()) {
200+
assertThat(attributes).containsEntry(CODE_FUNCTION_NAME, "MyClass.myMethod");
201+
}
202+
if (emitOldCodeSemconv()) {
203+
assertThat(attributes).containsEntry(CODE_NAMESPACE, "MyClass");
204+
}
205+
```
206+
207+
### `assumeTrue(...)` only when one mode is meaningless
208+
209+
Use `assumeTrue(EXPERIMENTAL_ATTRIBUTES)` / `assumeTrue(emitStable*())` /
210+
`assumeTrue(testLatestDeps())` only when an entire test is meaningful in one mode only —
211+
prefer the ternary/`if` pattern so both modes are exercised.

.github/agents/knowledge/testing-semconv-stability.md

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -76,26 +76,38 @@ for other domains `check { dependsOn(testStableSemconv) }`.
7676

7777
## Asserting Attributes in Tests
7878

79-
Preferred approach (most compact) — inline ternary with `emitStable*()`, where `null` means
80-
the attribute is expected absent:
79+
For the cross-cutting shape — inline ternary with `null` for "absent", static-imported flag
80+
accessors, and `assumeTrue(...)` guidance — see
81+
[testing-general-patterns.md](testing-general-patterns.md#flag-gated--mode-dependent-assertions).
82+
The semconv-specific patterns below build on that shape.
8183

82-
```java
83-
equalTo(DB_USER, emitStableDatabaseSemconv() ? null : USER_DB)
84-
equalTo(ERROR_TYPE, emitStableDatabaseSemconv() ? "42601" : null)
85-
span.hasName(emitStableDatabaseSemconv() ? "SELECT" : "SELECT dbname")
86-
```
84+
### Import `emitStable*Semconv()` directly
85+
86+
Static-import `emitStable*Semconv()` / `emitOld*Semconv()` from
87+
`io.opentelemetry.instrumentation.api.internal.SemconvStability` and call them inline. Do
88+
**not** wrap them in a per-class `static final boolean` field — they are already cheap
89+
static accessors.
8790

88-
Use `maybeStable(oldKey)` when only the attribute key changes and the value stays the same.
89-
`maybeStable()` does NOT cover `dup` mode — use separate `if` blocks for that.
91+
### `maybeStable(OLD_KEY)` for 1:1 key renames
92+
93+
Use `maybeStable(OLD_KEY)` when only the attribute *key* flips between old and stable
94+
semconv and the value is identical:
9095

9196
```java
9297
span.hasAttribute(equalTo(maybeStable(DB_STATEMENT), "SELECT ?"));
9398
```
9499

95-
Use separate `if` blocks (not `if/else`) when assertion structure differs significantly between
96-
modes — this ensures both branches execute in `dup` mode:
100+
`maybeStable()` does **not** cover `/dup` mode (it returns one key, not both), and does
101+
**not** apply where the mapping isn't 1:1 — for example `DB_RESPONSE_STATUS_CODE`
102+
`ERROR_TYPE`. Use `emitOld*()` / `emitStable*()` `if` blocks for those.
103+
104+
### `if` blocks (never `if/else`) for `/dup` mode
105+
106+
`/dup` mode emits **both** old and stable attributes, so each must be asserted in an
107+
independent top-level `if` block. Never use `if/else`:
97108

98109
```java
110+
// CORRECT — both branches run in /dup mode
99111
if (emitStableCodeSemconv()) {
100112
assertThat(attributes).containsEntry(CODE_FUNCTION_NAME, "MyClass.myMethod");
101113
}
@@ -104,7 +116,9 @@ if (emitOldCodeSemconv()) {
104116
}
105117
```
106118

107-
Use `assumeTrue(emitStable*())` only when an entire test is meaningful in one mode only.
119+
Do **not** add a single-arg `stable(value)` / `old(value)` helper — both modes typically
120+
emit real values (not `null`), and `maybeStable(...)` already covers the 1:1 key-rename
121+
case.
108122

109123
## Key Rules
110124

conventions/src/main/kotlin/io/opentelemetry/instrumentation/gradle/StaticImportFormatter.kt

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,11 @@ class StaticImportFormatter : FormatterFunc.NeedsFile, Serializable {
7777
"io.opentelemetry.instrumentation.api.internal.SemconvStability",
7878
"emit[a-zA-Z0-9]*",
7979
),
80+
Rule(
81+
"TestLatestDeps",
82+
"io.opentelemetry.instrumentation.testing.util.TestLatestDeps",
83+
"testLatestDeps",
84+
),
8085
Rule(
8186
"SqlDialect",
8287
"io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlDialect",

instrumentation/apache-dubbo-2.7/library-autoconfigure/src/test/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/DubboHeadersGetterTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package io.opentelemetry.instrumentation.apachedubbo.v2_7;
77

8+
import static io.opentelemetry.instrumentation.testing.util.TestLatestDeps.testLatestDeps;
89
import static java.util.Collections.singletonMap;
910
import static org.assertj.core.api.Assertions.assertThat;
1011
import static org.mockito.Mockito.when;
@@ -32,7 +33,7 @@ void testKeys() throws ReflectiveOperationException {
3233
when(context.getLocalAddress()).thenReturn(new InetSocketAddress(1));
3334

3435
// for latest dep tests call getObjectAttachments, otherwise call getAttachments
35-
if (Boolean.getBoolean("testLatestDeps")) {
36+
if (testLatestDeps()) {
3637
when(getObjectAttachments()).thenReturn(singletonMap("key", "value"));
3738
} else {
3839
when(rpcInvocation.getAttachments()).thenReturn(singletonMap("key", "value"));

instrumentation/apache-dubbo-2.7/testing/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/AbstractDubboTest.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableRpcSemconv;
1010
import static io.opentelemetry.instrumentation.testing.GlobalTraceUtil.runWithSpan;
1111
import static io.opentelemetry.instrumentation.testing.junit.service.SemconvServiceStabilityUtil.maybeStablePeerService;
12+
import static io.opentelemetry.instrumentation.testing.util.TestLatestDeps.testLatestDeps;
1213
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
1314
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
1415
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies;
@@ -186,7 +187,7 @@ void testApacheDubboBase() throws ReflectiveOperationException {
186187
: "hello"),
187188
equalTo(
188189
maybeStablePeerService(),
189-
hasServicePeerName() && Boolean.getBoolean("testLatestDeps")
190+
hasServicePeerName() && testLatestDeps()
190191
? "test-peer-service"
191192
: null),
192193
satisfies(
@@ -370,7 +371,7 @@ void testApacheDubboTest() throws ReflectiveOperationException {
370371
: "hello"),
371372
equalTo(
372373
maybeStablePeerService(),
373-
hasServicePeerName() && Boolean.getBoolean("testLatestDeps")
374+
hasServicePeerName() && testLatestDeps()
374375
? "test-peer-service"
375376
: null),
376377
satisfies(
@@ -484,7 +485,7 @@ static void assertNetworkType(AbstractStringAssert<?> stringAssert) {
484485

485486
static void assertLatestDeps(
486487
AbstractAssert<?, ?> assertion, Consumer<AbstractAssert<?, ?>> action) {
487-
if (Boolean.getBoolean("testLatestDeps")) {
488+
if (testLatestDeps()) {
488489
action.accept(assertion);
489490
} else {
490491
assertion.isNull();

instrumentation/apache-dubbo-2.7/testing/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/AbstractDubboTraceChainTest.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableRpcSemconv;
1111
import static io.opentelemetry.instrumentation.testing.GlobalTraceUtil.runWithSpan;
1212
import static io.opentelemetry.instrumentation.testing.junit.service.SemconvServiceStabilityUtil.maybeStablePeerService;
13+
import static io.opentelemetry.instrumentation.testing.util.TestLatestDeps.testLatestDeps;
1314
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
1415
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
1516
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies;
@@ -229,7 +230,7 @@ void testDubboChain() throws ReflectiveOperationException {
229230
: "hello"),
230231
equalTo(
231232
maybeStablePeerService(),
232-
hasServicePeerName() && Boolean.getBoolean("testLatestDeps")
233+
hasServicePeerName() && testLatestDeps()
233234
? "test-peer-service"
234235
: null),
235236
satisfies(
@@ -285,7 +286,7 @@ void testDubboChain() throws ReflectiveOperationException {
285286
: "hello"),
286287
equalTo(
287288
maybeStablePeerService(),
288-
hasServicePeerName() && Boolean.getBoolean("testLatestDeps")
289+
hasServicePeerName() && testLatestDeps()
289290
? "test-peer-service"
290291
: null),
291292
satisfies(
@@ -510,7 +511,7 @@ void testDubboChainInJvm() throws ReflectiveOperationException {
510511
: "hello"),
511512
equalTo(
512513
maybeStablePeerService(),
513-
hasServicePeerName() && Boolean.getBoolean("testLatestDeps")
514+
hasServicePeerName() && testLatestDeps()
514515
? "test-peer-service"
515516
: null),
516517
satisfies(

instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v5_0/ApacheHttpClientTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import static io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest.CONNECTION_TIMEOUT;
99
import static io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest.READ_TIMEOUT;
10+
import static io.opentelemetry.instrumentation.testing.util.TestLatestDeps.testLatestDeps;
1011

1112
import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
1213
import io.opentelemetry.instrumentation.testing.junit.http.HttpClientInstrumentationExtension;
@@ -224,7 +225,7 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
224225
// apparently apache http client does not report the 302 status code?
225226
optionsBuilder.setResponseCodeOnRedirectError(null);
226227

227-
if (Boolean.getBoolean("testLatestDeps")) {
228+
if (testLatestDeps()) {
228229
optionsBuilder.disableTestHttps();
229230
optionsBuilder.disableTestRemoteConnection();
230231
}

instrumentation/armeria/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttpServerTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55

66
package io.opentelemetry.javaagent.instrumentation.armeria.v1_3;
77

8+
import static io.opentelemetry.instrumentation.testing.util.TestLatestDeps.testLatestDeps;
9+
810
import com.linecorp.armeria.server.ServerBuilder;
911
import io.opentelemetry.instrumentation.armeria.v1_3.AbstractArmeriaHttpServerTest;
1012
import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
@@ -31,7 +33,7 @@ protected void configure(HttpServerTestOptions options) {
3133
options.setTestHttpPipelining(false);
3234
// span for non-standard request is created by netty instrumentation when not running latest
3335
// dep tests
34-
if (Boolean.getBoolean("testLatestDeps")) {
36+
if (testLatestDeps()) {
3537
options.disableTestNonStandardHttpMethod();
3638
} else {
3739
options.setResponseCodeOnNonStandardHttpMethod(405);

0 commit comments

Comments
 (0)