-
Notifications
You must be signed in to change notification settings - Fork 1k
Spring boot 4 support for RestClient #15684
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
Conversation
instrumentation/spring/spring-boot-autoconfigure/testing/build.gradle.kts
Outdated
Show resolved
Hide resolved
|
|
||
| @Test | ||
| void restClientBuilder() { | ||
| testing.clearAllExportedData(); |
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.
why do you need to call clearAllExportedData here? In tests that use our InstrumentationExtension this gets automatically called before each test. If these smoke tests don't use it then perhaps it could be called from a @BeforeEach method so it wouldn't need to be called in each tests.
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.
one of the tests is asserting the telemetry emitted during the test setup, so BeforeEach couldn't be used, but AfterEach does
| .mutate() | ||
| .requestInterceptors( | ||
| interceptors -> { | ||
| if (interceptors.stream() |
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.
perhaps we should use something like in #15546 to avoid rebuilding the client when interceptor wasn't added?
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.
updated. depending on which PR is merged first, we can move the test to the AbstractRestClientInstrumentationAutoConfigurationTest class in the second one
…mentation into spring-restclient
0901e86 to
39960c1
Compare
ede1190 to
e57ab9d
Compare
...on/spring/autoconfigure/internal/instrumentation/web/RestClientBeanPostProcessorSpring4.java
Outdated
Show resolved
Hide resolved
|
|
||
| testImplementation("org.springframework:spring-test:7.0.2") | ||
| testImplementation("org.springframework.boot:spring-boot-resttestclient") | ||
| testImplementation("org.springframework.boot:spring-boot-restclient") |
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.
this is unrelated to the other changes, they released 4.0.1 yesterday which broke stuff. See spring-projects/spring-boot#48253
| // classes in test setup require 5.0+ | ||
| testLibrary("org.springframework.cloud:spring-cloud-starter-gateway-server-webflux:5.0.0") | ||
| testLibrary("org.springframework.boot:spring-boot-starter-test:4.0.0") | ||
| testLibrary("org.springframework.boot:spring-boot-webflux") |
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.
this is also unrelated to restClient but instead related to the 4.0.1 version bump overnight which resulted in some missing classes.
Part of #14906
Addresses: #14906 (comment)