Skip to content

Convert camel-2.20 Groovy tests to Java#8813

Merged
mateuszrzeszutek merged 12 commits into
open-telemetry:mainfrom
jaydeluca:camel-2-20-tests-groovy-to-java
Jul 7, 2023
Merged

Convert camel-2.20 Groovy tests to Java#8813
mateuszrzeszutek merged 12 commits into
open-telemetry:mainfrom
jaydeluca:camel-2-20-tests-groovy-to-java

Conversation

@jaydeluca

Copy link
Copy Markdown
Member

Related to #7195

This PR converts the primary test package and decorators from groovy to Java for the camel-2.20 instrumentation.

A few notes:

  • I started converting the tests in the AWS package but almost all of them are marked as ignored, and for the one test that isn't ignored I am running into what seems like some concurrency issues. I will continue working on it but felt it might be acceptable to break that out as a separate PR in the future. Please let me know if you require me to address those in the same PR and I can continue hacking on it.
  • I had to pull in a few new dependencies, let me know if any of those are unacceptable or if I need to provide any additional context

Thank you for your consideration of my contribution

@jaydeluca jaydeluca requested a review from a team June 27, 2023 16:30
@linux-foundation-easycla

linux-foundation-easycla Bot commented Jun 27, 2023

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

import org.springframework.boot.SpringApplication;
import org.springframework.context.ConfigurableApplicationContext;

public class SingleServiceCamelTest extends RetryOnAddressAlreadyInUse {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In tests that use http server and need a client to test it with we usually use https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerUsingTest.java Up to you to decide whether it would help you.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thank you for this recommendation, i was able to remove the additional dependency on okhttp by switching to this

jaydeluca added 2 commits July 4, 2023 07:09
…race relation assertion, remove retryonaddressalreadyinuse
…race relation assertion, remove retryonaddressalreadyinuse
Comment thread instrumentation/camel-2.20/javaagent/build.gradle.kts Outdated
span.hasName("input")
.hasKind(SpanKind.INTERNAL)
.hasNoParent()
.hasAttribute(stringKey("camel.uri"), "direct://input"),

@laurit laurit Jul 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be better to use hasAttributesSatisfyingExactly instead of hasAttribute, groovy tests also verify that all attributes are asserted.

hasAttributesSatisfyingExactly(equalTo(stringKey("camel.uri"), "direct://input")))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thank you for the recommendation, I have updated accordingly

span.hasName("GET /api/{module}/unit/{unitId}")
.hasKind(SpanKind.SERVER)
.hasParent(trace.getSpan(1))
.hasAttributesSatisfying(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if possible use hasAttributesSatisfyingExactly

@laurit

laurit commented Jul 7, 2023

Copy link
Copy Markdown
Contributor

@jaydeluca looks good. See if you can replace the usages of hasAttribute and hasAttributesSatisfying with hasAttributesSatisfyingExactly.

@mateuszrzeszutek mateuszrzeszutek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @jaydeluca !

@mateuszrzeszutek mateuszrzeszutek merged commit e8da33c into open-telemetry:main Jul 7, 2023
@jaydeluca

Copy link
Copy Markdown
Member Author

thank you @laurit and @mateuszrzeszutek for taking the time to review and provide guidance, I appreciate it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants