Skip to content

Convert kafka-clients unit test from groovy to java#7770

Merged
mateuszrzeszutek merged 7 commits into
open-telemetry:mainfrom
cuichenli:kafka-unit-test
Feb 13, 2023
Merged

Convert kafka-clients unit test from groovy to java#7770
mateuszrzeszutek merged 7 commits into
open-telemetry:mainfrom
cuichenli:kafka-unit-test

Conversation

@cuichenli

@cuichenli cuichenli commented Feb 9, 2023

Copy link
Copy Markdown
Member

No description provided.

wip
771e3e8 
wip

wip

clean
@cuichenli cuichenli requested a review from a team February 9, 2023 07:05
mateuszrzeszutek
mateuszrzeszutek reviewed Feb 9, 2023
View reviewed changes

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

I left a couple of comments, they're mostly about minor style issues. Overall it looks great, thanks @cuichenli !

Comment thread ...sting/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaClientBaseTest.java Outdated
Comment thread ...sting/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaClientBaseTest.java Outdated
Comment thread ...sting/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaClientBaseTest.java Outdated
Comment thread ...sting/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaClientBaseTest.java Outdated
Comment thread ...sting/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaClientBaseTest.java Outdated
Comment thread ...gent/src/test/java/io/opentelemetry/instrumentation/kafkaclients/KafkaClientDefaultTest.java Outdated
Comment thread ...gent/src/test/java/io/opentelemetry/instrumentation/kafkaclients/KafkaClientDefaultTest.java Outdated
Comment thread ...gent/src/test/java/io/opentelemetry/instrumentation/kafkaclients/KafkaClientDefaultTest.java Outdated
Comment thread ...t/java/io/opentelemetry/instrumentation/kafkaclients/KafkaClientPropagationDisabledTest.java
Comment thread .../java/io/opentelemetry/instrumentation/kafkaclients/KafkaClientSuppressReceiveSpansTest.java Outdated
Comment on lines +66 to +67
testing.waitForTraces(1);
testing.waitAndAssertTraces(

@mateuszrzeszutek mateuszrzeszutek Feb 9, 2023

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.

The waitAndAssertTraces() method already waits for 1 completed trace, there should be no need to call waitForTraces() explicitly

Suggested change
testing.waitForTraces(1);
testing.waitAndAssertTraces(
testing.waitAndAssertTraces(

@cuichenli cuichenli Feb 10, 2023

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.

removed most testing.waitForTraces but kept two where there is no testing.waitAndAssertTraces

cuichenli and others added 3 commits February 10, 2023 13:42
@cuichenli
Apply suggestions from code review
4594778 
Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
remove redundant public annotation
cbfcf16
update
43899d7
@cuichenli

cuichenli commented Feb 10, 2023

Copy link
Copy Markdown
Member Author

@mateuszrzeszutek thanks for your comments, updated accordingly. please take a look again. thanks!

mateuszrzeszutek
mateuszrzeszutek approved these changes Feb 10, 2023
View reviewed changes

@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 @cuichenli !

Comment thread ...t/java/io/opentelemetry/instrumentation/kafkaclients/KafkaClientPropagationDisabledTest.java Outdated
Comment thread .../java/io/opentelemetry/instrumentation/kafkaclients/KafkaClientSuppressReceiveSpansTest.java Outdated
Comment thread ...sting/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaClientBaseTest.java Outdated
@mateuszrzeszutek

mateuszrzeszutek commented Feb 10, 2023

Copy link
Copy Markdown
Member

The latest dep version tests are failing against an errorprone warning

/home/runner/work/opentelemetry-java-instrumentation/opentelemetry-java-instrumentation/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/test/java/io/opentelemetry/instrumentation/kafkaclients/KafkaClientDefaultTest.java:74: warning: [PreferJavaTimeOverload] If the numeric primitive (Duration.ofSeconds(5).toMillis()) represents a Duration, please call consumer.poll(Duration) instead.
    ConsumerRecords<?, ?> records = consumer.poll(Duration.ofSeconds(5).toMillis());
                                                 ^
    (see https://errorprone.info/bugpattern/PreferJavaTimeOverload)

You might want to add @SuppressWarnings("PreferJavaTimeOverload") whenever poll(long) is called

@cuichenli

cuichenli commented Feb 13, 2023

Copy link
Copy Markdown
Member Author

@mateuszrzeszutek thanks, all the tests passed

@mateuszrzeszutek mateuszrzeszutek merged commit d3326db into open-telemetry:main Feb 13, 2023
@trask trask mentioned this pull request Feb 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cuichenli @mateuszrzeszutek