test: parameterize KubernetesRequestUtilsTest cases#18812
Open
zeitlinger wants to merge 14 commits into
Open
Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
… DTO shape Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…rTest The five sort tests share identical structure (build modules, sort, assert order). Collapse to a single @ParameterizedTest with @MethodSource for clearer scenario-by-scenario coverage. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…ingFactoryTest scalar/array cases Collapse 22 createAttributeBindingFor<Type> tests into a single @ParameterizedTest with @MethodSource. Each row carries a human-readable scenario name plus the type, input value, expected attribute key, and expected value. Default-binding and reflection-based List<T> tests are left as separate @test methods because their setup differs (TestClass toString fallback / TestFields generic Type lookup). Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Collapse the per-numeric-type Attribute / AttributeValue test pairs into two @ParameterizedTest sets keyed by attribute name: one for integral JMX attributes (byte/short/int/long, usesDoubleValues=false) and one for floating-point attributes (float/double, usesDoubleValues=true). Kept as separate methods (not one combined parameterized test) because the assertion shape differs — long vs double comparisons. String / Boolean / Enum tests stay separate because their info is null and the extracted value type is different. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…igurationTest Group the four cases by their assertion shape: two rows where the exporter is expected to be present (debug=true with traces.exporter=none or =logging) and two rows where it must be absent (debug unset or debug=false). Two @ParameterizedTest methods keep the assertion uniform within each group rather than forking on a boolean inside one method. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…-write cases Three of the snippet-injection tests share the same shape (single write of a full HTML document, expect injection success and matching after-html bytes). Collapse them into one @ParameterizedTest carrying the before / after resource file names. The split-write and slice-from-larger-buffer tests stay separate because their control flow is materially different. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…with shared case class Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Follow the parameterized-tests pattern: convert isResourceRequest to @ParameterizedTest with named rows, and add case-name first parameter to k8sRequestVerbs for consistent failure output. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…d-example' into k8s-param-test # Conflicts: # .github/agents/knowledge/testing-general-patterns.md # instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/internal/engine/AttributeExtractorTest.java # instrumentation/kubernetes-client-7.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/kubernetesclient/v7_0/KubernetesRequestUtilsTest.java
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors several JUnit tests to use @ParameterizedTest for clearer, more maintainable assertions and improved failure output, and adds a helper script to detect repeated test bodies that are good candidates for parameterization.
Changes:
- Converts multiple single-case tests into parameterized tests with named cases.
- Improves parameterized-test diagnostics by adding readable case names and using
arguments(...)in some providers. - Adds a heuristic Python scanner to find repeated non-parameterized test bodies.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LoggingExporterAutoConfigurationTest.java | Parameterizes exporter-present/absent scenarios with named cases. |
| instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/SnippetServletOutputStreamTest.java | Collapses repeated snippet-injection tests into a single parameterized test. |
| instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/OsResourceTest.java | Converts OS-type tests into a parameterized test, manually setting os.name per case. |
| instrumentation/kubernetes-client-7.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/kubernetesclient/v7_0/KubernetesRequestUtilsTest.java | Parameterizes isResourceRequest and improves k8sRequestVerbs case naming. |
| instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/internal/engine/AttributeExtractorTest.java | Parameterizes repeated attribute-info/value tests for numeric types. |
| instrumentation-docs/src/test/java/io/opentelemetry/instrumentation/docs/utils/InstrumentationNameComparatorTest.java | Parameterizes comparator test cases using input/expected lists. |
| instrumentation-annotations-support/src/test/java/io/opentelemetry/instrumentation/api/annotation/support/AttributeBindingFactoryTest.java | Parameterizes many attribute-binding cases into one typed test. |
| .github/scripts/code-review/find-parameterized-test-candidates.py | Adds a heuristic script to locate repeated non-parameterized JUnit test bodies. |
Comment on lines
+46
to
60
| @ParameterizedTest(name = "{0}") | ||
| @MethodSource("osTypeTestCases") | ||
| void osType(String name, String osName, String expectedOsType) { | ||
| String originalOsName = System.getProperty("os.name"); | ||
| try { | ||
| System.setProperty("os.name", osName); | ||
|
|
||
| Resource resource = OsResource.buildResource(); | ||
| assertThat(resource.getSchemaUrl()).isEqualTo(SchemaUrls.V1_24_0); | ||
| assertThat(resource.getAttribute(OS_TYPE)).isEqualTo(expectedOsType); | ||
| assertThat(resource.getAttribute(OS_DESCRIPTION)).isNotEmpty(); | ||
| } finally { | ||
| restoreSystemProperty("os.name", originalOsName); | ||
| } | ||
| } |
Comment on lines
+123
to
+147
| def find_matching_brace(text: str, open_idx: int) -> int: | ||
| depth = 0 | ||
| in_string: str | None = None | ||
| escaping = False | ||
| i = open_idx | ||
| while i < len(text): | ||
| char = text[i] | ||
| if in_string: | ||
| if escaping: | ||
| escaping = False | ||
| elif char == "\\": | ||
| escaping = True | ||
| elif char == in_string: | ||
| in_string = None | ||
| else: | ||
| if char in ('"', "'"): | ||
| in_string = char | ||
| elif char == "{": | ||
| depth += 1 | ||
| elif char == "}": | ||
| depth -= 1 | ||
| if depth == 0: | ||
| return i | ||
| i += 1 | ||
| return -1 |
Comment on lines
+147
to
154
| @ParameterizedTest(name = "{0}") | ||
| @MethodSource("longAttributes") | ||
| void longAttributeInfo(String attributeName, long expectedValue) { | ||
| BeanAttributeExtractor extractor = BeanAttributeExtractor.fromName(attributeName); | ||
| AttributeInfo info = extractor.getAttributeInfo(theServer, objectName); | ||
| assertThat(info).isNotNull(); | ||
| assertThat(info.usesDoubleValues()).isFalse(); | ||
| } |
Comment on lines
+6
to
+10
| """Find repeated non-parameterized test bodies that are candidates for @ParameterizedTest. | ||
|
|
||
| This is a heuristic scanner. It groups non-parameterized JUnit test methods inside the same file | ||
| by a normalized method-body shape and reports repeated groups. | ||
| """ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Apply the parameterized-tests pattern to
KubernetesRequestUtilsTest:isResourceRequestfrom a single@Testwith 14 inline assertions into a@ParameterizedTestwith named rows.k8sRequestVerbsfor clearer failure output, and switch its rows fromArguments.of(...)toarguments(...)to match the other providers in the file.Test plan
./gradlew :instrumentation:kubernetes-client-7.0:javaagent-unit-tests:testpasses./gradlew :instrumentation:kubernetes-client-7.0:javaagent-unit-tests:spotlessCheckpasses