Migrate utils module Groovy tests to Java tests#11569
Conversation
sarahchen6
added
comp: testing
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("convertIterableMapAndBitSetArguments") |
There was a problem hiding this comment.
wasn't sure how to convert this to a TableTest in a simple manner since the method takes multiple types... let me know if you have recommendations!
There was a problem hiding this comment.
Just pass strings and do simple parsing? Or introduce convertion function?
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
| StableConfigSource stableConfigSource = | ||
| new StableConfigSource(filePath.toString(), ConfigOrigin.LOCAL_STABLE_CONFIG); | ||
|
|
||
| // Create ConfigProvider via reflection (constructor is private) |
There was a problem hiding this comment.
the alternative was having this test file a stableconfig subfolder so that package-private safeToString() can be accessed 🤔
| // spotless:on | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("containerInfoParsedFromFileContentArguments") |
There was a problem hiding this comment.
This MethodSource was kept for readability -- TableTest requires inputs to be on one line
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54ccc2aa75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| @TableTest({ | ||
| "configId | defaultConfigs ", | ||
| "'' | [:] ", |
There was a problem hiding this comment.
For the non-empty stable-config case, this test now only checks that constructing StableConfigSource returns an object, so it would pass even if StableConfigSource silently ignored every apm_configuration_default entry. The migrated Groovy test exercised stableCfg.get(...) for each default key; keeping that assertion is important because this is the case that protects stable-config defaults from being parsed but then dropped before consumers read them.
Useful? React with 👍 / 👎.
AlexeyKuznetsov-DD
left a comment
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
LGTM, left minor comments about code style
| import java.util.Map; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class CapturedEnvironmentTest extends DDJavaSpecification { |
There was a problem hiding this comment.
Do we really need to extends DDJavaSpecification here? Or am I missing where it used?
| @Test | ||
| void nonAutodetectedServiceNameWithNullCommand() throws IOException, InterruptedException { | ||
| String serviceName = forkAndRunProperties("null"); | ||
|
|
||
| assertNull(serviceName); | ||
| } | ||
|
|
||
| @Test | ||
| void nonAutodetectedServiceNameWithEmptyCommand() throws IOException, InterruptedException { | ||
| String serviceName = forkAndRunProperties(""); | ||
|
|
||
| assertNull(serviceName); | ||
| } | ||
|
|
||
| @Test | ||
| void nonAutodetectedServiceNameWithAllBlanksCommand() throws IOException, InterruptedException { | ||
| String serviceName = forkAndRunProperties(" "); | ||
|
|
||
| assertNull(serviceName); | ||
| } | ||
|
|
||
| @Test | ||
| void setServiceNameBySyspropSunJavaCommandWithClass() throws IOException, InterruptedException { | ||
| String serviceName = forkAndRunProperties("org.example.App -Dfoo=bar arg2 arg3"); | ||
|
|
||
| assertEquals("org.example.App", serviceName); | ||
| } | ||
|
|
||
| @Test | ||
| void setServiceNameBySyspropSunJavaCommandWithJar() throws IOException, InterruptedException { | ||
| String serviceName = forkAndRunProperties("foo/bar/example.jar -Dfoo=bar arg2 arg3"); | ||
|
|
||
| assertEquals("example", serviceName); | ||
| } | ||
|
|
||
| @Test | ||
| void setServiceNameWithRealSunJavaCommandProperty() throws IOException, InterruptedException { | ||
| String serviceName = forkAndRunProperties(null); | ||
|
|
||
| assertEquals(ServiceNamePrinter.class.getName(), serviceName); | ||
| } | ||
|
|
||
| @Test | ||
| void useAzureSiteNameInAzure() throws IOException, InterruptedException { | ||
| HashMap<String, String> azureEnvVars = new HashMap<>(); | ||
| azureEnvVars.put("DD_AZURE_APP_SERVICES", "1"); | ||
| azureEnvVars.put("WEBSITE_SITE_NAME", "siteService"); | ||
|
|
||
| String serviceName = | ||
| forkAndRunProperties("foo/bar/example.jar -Dfoo=bar arg2 arg3", azureEnvVars); | ||
|
|
||
| assertEquals("siteService", serviceName); | ||
| } | ||
|
|
||
| @Test | ||
| void dontUseSiteNameWhenNotInAzure() throws IOException, InterruptedException { | ||
| String serviceName = | ||
| forkAndRunProperties( | ||
| "foo/bar/example.jar -Dfoo=bar arg2 arg3", | ||
| Collections.singletonMap("WEBSITE_SITE_NAME", "siteService")); | ||
|
|
||
| assertEquals("example", serviceName); | ||
| } | ||
|
|
||
| @Test | ||
| void dontUseAzureSiteNameWhenNull() throws IOException, InterruptedException { | ||
| String serviceName = | ||
| forkAndRunProperties( | ||
| "foo/bar/example.jar -Dfoo=bar arg2 arg3", | ||
| Collections.singletonMap("DD_AZURE_APP_SERVICES", "true")); | ||
|
|
||
| assertEquals("example", serviceName); | ||
| } |
There was a problem hiding this comment.
I have feeling that all this tests can be refactored to @TableTest
| @TableTest({ | ||
| "scenario | key1 | key2 | value1 | value2 | origin1 | origin2 ", | ||
| "equal | key | key | value | value | DEFAULT | DEFAULT ", | ||
| "different key | key | key2 | value | value | ENV | ENV ", | ||
| "different value | key | key | value2 | value | JVM_PROP | JVM_PROP", | ||
| "different origin | key | key | value | value | ENV | DEFAULT " | ||
| }) | ||
| void supportsEqualityCheck( | ||
| String key1, | ||
| String key2, | ||
| Object value1, | ||
| Object value2, | ||
| ConfigOrigin origin1, | ||
| ConfigOrigin origin2) { |
There was a problem hiding this comment.
nit: may be re-group a bit: k1, v1, o1, k2, v2, o2? WDYT?
| // when | ||
| ConfigSetting cs1 = ConfigSetting.of(key1, value1, origin1); | ||
| ConfigSetting cs2 = ConfigSetting.of(key2, value2, origin2); | ||
|
|
||
| // then | ||
| if (key1.equals(key2) && value1.equals(value2) && origin1 == origin2) { | ||
| assertEquals(cs1.hashCode(), cs2.hashCode()); | ||
| assertEquals(cs1, cs2); | ||
| assertEquals(cs2, cs1); | ||
| assertEquals(cs1.toString(), cs2.toString()); | ||
| } else { | ||
| assertNotEquals(cs1.hashCode(), cs2.hashCode()); | ||
| assertNotEquals(cs1, cs2); | ||
| assertNotEquals(cs2, cs1); | ||
| assertNotEquals(cs1.toString(), cs2.toString()); | ||
| } |
There was a problem hiding this comment.
nit: 1. such //when & //then comments make almost no sense.
nit: 2. Probably instead of if (key1.equals(key2) && value1.equals(value2) && origin1 == origin2) {} else {}
better to introduce one more param smth. like expectedEqual and check it instead.
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("convertIterableMapAndBitSetArguments") |
There was a problem hiding this comment.
Just pass strings and do simple parsing? Or introduce convertion function?
| assertTrue(exception.getMessage().contains("Invalid boolean value:")); | ||
| } | ||
|
|
||
| // spotless:off |
There was a problem hiding this comment.
Spotless failed here? If yes, can you manually align table columns?
| expected, | ||
| ConfigConverter.parseMapWithOptionalMappings(mapString, "test", defaultPrefix, lowercaseKeys)); | ||
| } | ||
| // spotless:on |
There was a problem hiding this comment.
better to do on/off in small blocks as possible.
| import java.util.Map; | ||
| import org.tabletest.junit.TableTest; | ||
|
|
||
| public class ConfigConverterTest extends DDJavaSpecification { |
There was a problem hiding this comment.
Same DDJavaSpecification question... here and all similar places.
| } catch (NumberFormatException ignored) { | ||
| } | ||
| return s; | ||
| } |
There was a problem hiding this comment.
I would add empty lines between if... to improve readability...
if (source == null) return null;
String s = source.toString();
if (s.isEmpty()) return null;
if ("true".equals(s)) return Boolean.TRUE;
if ("false".equals(s)) return Boolean.FALSE;
if (s.endsWith("f")) {
...
|
|
||
| public class TimeUtilsTest extends DDJavaSpecification { | ||
|
|
||
| // spotless:off |
There was a problem hiding this comment.
Spotless should work with TableTest...
2 participants
What Does This Do
Migrate Groovy tests in the
utilsmodule to Java without changing any functionalityIntroduce TableTest converter for boxed values
Motivation
This is part of a broader initiative to migrate all testing in this repo to JUnit
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]