-
Notifications
You must be signed in to change notification settings - Fork 984
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
Move test code depending on Spring to separate module #5582
Conversation
It is difficult to depend on Spring 6 in test code in our published modules because we maintain a Java 8 baseline while Spring 6 requires Java 17. Moving this code to a separate module avoids that issue. There is still one usage of Spring in our normal test code for ModifiedClassPathClassLoader.
testImplementation 'com.google.inject:guice' | ||
|
||
// Uncomment these if you are interested in testing injection with dagger in MeterRegistryInjectionTest | ||
// testImplementation 'com.google.dagger:dagger' | ||
// testAnnotationProcessor 'com.google.dagger:dagger-compiler' | ||
|
||
// Only needed for ModifiedClassPathClassLoader | ||
testImplementation(libs.spring5.core) |
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.
I'd like to get rid of this test dependency as well, but that's a more involved change than I want to make now to solve the issue of test failures on bumping the Spring version.
* Tests for {@link MeterTagSupport}. | ||
* Tests for {@link MeterTagSupport} have been moved to the module | ||
* micrometer-samples-spring-framework6 to test with the current version of Spring that | ||
* requires JDK 17+.. | ||
* | ||
* @author Marcin Grzejszczak | ||
* @author Johnny Lim | ||
*/ | ||
class MeterTagSupportTests { |
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 could just delete this file, but I thought it might help to have a reference in the same project to the test class for the main code.
// Spring 6 requires Java 17+ | ||
// skip this module when building with jdk <17 | ||
if (!javaLanguageVersion.canCompileOrRun(17)) { | ||
project.tasks.configureEach { task -> task.enabled = false } |
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.
I assume this is okay for the docs module since we run it with JDK 17 (according to this), but let me know if I'm missing any reason this would be a problem.
|
||
compileTestJava { | ||
// need to override the config applied in the root build.gradle to all subprojects | ||
// TODO can we not do this with Gradle's toolchains instead? |
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.
My hope was the above java
block would take care of setting the Java target/source for the test compile task as well, but it didn't. I'm not sure if there's a better way to do it than I did here, but I ran out of patience looking into ways to configure Gradle.
micrometer-core/src/test/java/io/micrometer/core/aop/MeterTagSupportTests.java
Outdated
Show resolved
Hide resolved
…upportTests.java Co-authored-by: Jonatan Ivanov <[email protected]>
It is difficult to depend on Spring 6 in test code in our published modules because we maintain a Java 8 baseline while Spring 6 requires Java 17. Moving this code to a separate module avoids that issue. There is still one usage of Spring in our normal test code for ModifiedClassPathClassLoader.
This will unblock #5432 because the same tests do not fail on the latest Spring 6.1.