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.

Sign up for GitHub

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

Jump to bottom

Convert camel-2.20 Groovy tests to Java #8813

Merged
merged 12 commits into from
Jul 7, 2023
Merged

Convert camel-2.20 Groovy tests to Java #8813

merged 12 commits into from
Jul 7, 2023

Conversation

jaydeluca
Copy link
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
Copy link

linux-foundation-easycla bot commented Jun 27, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

laurit
laurit reviewed Jul 4, 2023
View reviewed changes
...test/java/io/opentelemetry/javaagent/instrumentation/apachecamel/SingleServiceCamelTest.java Outdated
import org.springframework.boot.SpringApplication;
import org.springframework.context.ConfigurableApplicationContext;

public class SingleServiceCamelTest extends RetryOnAddressAlreadyInUse {
Copy link
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
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

@laurit laurit mentioned this pull request Jul 4, 2023
Open
@jaydeluca
fix visibility, use static import for AttributeKey, simplify parent t…
372c491 
…race relation assertion, remove retryonaddressalreadyinuse
@jaydeluca
fix visibility, use static import for AttributeKey, simplify parent t…
f825efa 
…race relation assertion, remove retryonaddressalreadyinuse
laurit
laurit reviewed Jul 7, 2023
View reviewed changes
...nt/src/test/java/io/opentelemetry/javaagent/instrumentation/apachecamel/DirectCamelTest.java Outdated
span.hasName("input")
.hasKind(SpanKind.INTERNAL)
.hasNoParent()
.hasAttribute(stringKey("camel.uri"), "direct://input"),
Copy link
Contributor

@laurit laurit Jul 7, 2023 •
edited
Loading

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
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

laurit
laurit reviewed Jul 7, 2023
View reviewed changes
...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/apachecamel/RestCamelTest.java Outdated
span.hasName("GET /api/{module}/unit/{unitId}")
.hasKind(SpanKind.SERVER)
.hasParent(trace.getSpan(1))
.hasAttributesSatisfying(
Copy link
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
Copy link
Contributor

laurit commented Jul 7, 2023

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

mateuszrzeszutek
mateuszrzeszutek approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

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
44 checks passed
@jaydeluca
Copy link
Member Author

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

@jaydeluca jaydeluca deleted the camel-2-20-tests-groovy-to-java branch July 9, 2023 14:07
@jaydeluca jaydeluca mentioned this pull request Sep 8, 2023
Merged
@jaydeluca jaydeluca mentioned this pull request Jul 1, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurit laurit laurit approved these changes

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jaydeluca @laurit @mateuszrzeszutek