Skip to content
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

convert undertow groovy test to java #12207

Merged
merged 10 commits into from
Oct 9, 2024
Merged

Conversation

shalk
Copy link
Contributor

@shalk shalk commented Sep 10, 2024

related to #7195

@shalk shalk marked this pull request as ready for review September 10, 2024 12:42
@shalk shalk requested a review from a team September 10, 2024 12:42

options.setHttpAttributes(
endpoint ->
Collections.unmodifiableSet(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to use static import here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it is better to use

options.setHttpAttributes(endpoint -> ImmutableSet.of(NetworkAttributes.NETWORK_PEER_PORT));

Comment on lines 41 to 49
.addExactPath(
SUCCESS.rawPath(),
exchange ->
exchange.dispatch(
k ->
testing.runWithSpan(
"controller",
(ThrowingRunnable<Exception>)
() -> k.getResponseSender().send(SUCCESS.getBody()))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the java AbstractHttpServerTest has a helper method for this, similar to the ones in groovy

I think it would look like:

Suggested change
.addExactPath(
SUCCESS.rawPath(),
exchange ->
exchange.dispatch(
k ->
testing.runWithSpan(
"controller",
(ThrowingRunnable<Exception>)
() -> k.getResponseSender().send(SUCCESS.getBody()))))
.addExactPath(
SUCCESS.rawPath(),
exchange -> controller(SUCCESS, () -> {
exchange.getResponseSender().send(SUCCESS.getBody());
return null;
}))

Copy link
Contributor Author

@shalk shalk Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tries this in my second commits 98c5f08#diff-b3a26e2313b3c2d5656873760131d5794cccfdda0263105ad63b83527d623981R47

But the test case failed.

emmm, Let me try again...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

val latestDepTest = findProperty("testLatestDeps") as Boolean
if (latestDepTest) {
tasks.withType<JavaCompile>().configureEach {
options.release.set(11)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you would need to add this. In CI we run latest dep tests with java 21 so even if the undertow libs require java 11 it would work.

Copy link
Contributor Author

@shalk shalk Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the ci can not pass if i do not change the build.gradle.kts

I verify the test case by ./gradlew :instrumentation:undertow-1.4:javaagent:compileTestJava -PtestLatestDeps=true --debug local after i change the file.

I think though the jdk is 21, but the --release 8 is in compile args. So the feature on JDK9 can not be used.
this can ref spring-boot3 module. What do you think ? Or any other fix suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use

if (latestDepTest) {
  otelJava {
    minJavaVersionSupported.set(JavaVersion.VERSION_11)
  }
}

exchange.dispatch(
k ->
controller(
CAPTURE_HEADERS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path and the endpoint used don't match

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

exchange.dispatch(
k ->
controller(
CAPTURE_HEADERS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path and the endpoint used don't match

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@laurit laurit merged commit 8f18c30 into open-telemetry:main Oct 9, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants