-
Notifications
You must be signed in to change notification settings - Fork 331
Add Config and Context JUnit extensions #11076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,6 +159,7 @@ include( | |
| ":dd-java-agent:testing", | ||
| ":utils:config-utils", | ||
| ":utils:container-utils", | ||
| ":utils:junit-utils", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought (non-blocking): I'm thinking that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think so yes.
Agreed
That could be a solution yes. I only wanted to do a follow up of @jpbempel PR and tried to avoid changing too many things (I did change a lot in this PR overall 😬 ) because I know he has some more changes wrt to this migration. But yes, I want to get rid of groovy from the default testing module 😉 |
||
| ":utils:filesystem-utils", | ||
| ":utils:flare-utils", | ||
| ":utils:logging-utils", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| plugins { | ||
| `java-library` | ||
| } | ||
|
|
||
| apply(from = "$rootDir/gradle/java.gradle") | ||
|
|
||
| dependencies { | ||
| api(libs.bytebuddy) | ||
| api(libs.bytebuddyagent) | ||
| api(libs.forbiddenapis) | ||
| api(project(":components:environment")) | ||
|
|
||
| compileOnly(libs.junit.jupiter) | ||
| compileOnly(libs.tabletest) | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||
| package datadog.trace.junit.utils.config; | ||||||
|
|
||||||
| import java.lang.annotation.ElementType; | ||||||
| import java.lang.annotation.Repeatable; | ||||||
| import java.lang.annotation.Retention; | ||||||
| import java.lang.annotation.RetentionPolicy; | ||||||
| import java.lang.annotation.Target; | ||||||
| import org.junit.jupiter.api.extension.ExtendWith; | ||||||
|
|
||||||
| /** | ||||||
| * Declares a configuration override for a test. Can be placed on a test class (applies to all | ||||||
| * tests) or on individual test methods. | ||||||
| * | ||||||
| * <p>By default, injects a system property with the {@code dd.} prefix. Use {@code env = true} for | ||||||
| * environment variables (prefix {@code DD_}). | ||||||
|
Comment on lines
+14
to
+15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought (non-blocking): Maybe the code can be smart enough to detect when there's the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it but I did not because our config keys should not have |
||||||
| * | ||||||
| * <p>Examples: | ||||||
| * | ||||||
| * <pre>{@code | ||||||
| * @WithConfig(key = "service", value = "my_service") | ||||||
| * @WithConfig(key = "trace.resolver.enabled", value = "false") | ||||||
| * class MyTest extends DDJavaSpecification { | ||||||
| * | ||||||
| * @Test | ||||||
| * @WithConfig(key = "AGENT_HOST", value = "localhost", env = true) | ||||||
| * void testWithEnv() { ... } | ||||||
| * } | ||||||
| * }</pre> | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is one to class the class definition, and another to close the |
||||||
| */ | ||||||
| @Retention(RetentionPolicy.RUNTIME) | ||||||
| @Target({ElementType.TYPE, ElementType.METHOD}) | ||||||
| @Repeatable(WithConfigs.class) | ||||||
| @ExtendWith(WithConfigExtension.class) | ||||||
| public @interface WithConfig { | ||||||
| /** | ||||||
| * Config key (e.g. {@code "trace.resolver.enabled"}). The {@code dd.}/{@code DD_} prefix is | ||||||
| * auto-added unless {@link #addPrefix()} is {@code false}. | ||||||
| */ | ||||||
| String key(); | ||||||
|
|
||||||
| /** Config value. */ | ||||||
| String value(); | ||||||
|
|
||||||
| /** If {@code true}, sets an environment variable instead of a system property. */ | ||||||
| boolean env() default false; | ||||||
|
|
||||||
| /** If {@code false}, the key is used as-is without adding the {@code dd.}/{@code DD_} prefix. */ | ||||||
| boolean addPrefix() default true; | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Shouldn't we introduce a source level annotation
VisibleForTesting?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should come up with something like this yes. Maybe have our own?