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

Add integration tests for BraveTracingProvider #93

Merged
merged 5 commits into from
Mar 4, 2021

Conversation

m50d
Copy link
Contributor

@m50d m50d commented Feb 25, 2021

We already have an integration test for tracing in general, but it's good to confirm that the specific integration with Brave also works

Copy link
Member

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

Yeah, tracing feature works in end-to-end from producer to processor, so integration test would be meaningful to check if it works correctly.

I added some feedbacks. Please take a look and give your opinions.

}
}))
.producerSupplier(
bootstrapServers -> kafkaTracing.producer(TestUtils.producer(bootstrapServers)))
Copy link
Member

Choose a reason for hiding this comment

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

Is that necessary to pass producerSupplier ?

.producerDecorator(kafkaTracing::producer)

more natural?

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 thought there might be future tests where we wanted to supply a completely new producer (e.g. a mock/stub, different configuration parameters) rather than just wrap the existing one.

@m50d m50d requested a review from ocadaruma March 1, 2021 06:13
Copy link
Member

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

Added minor comment, but almost looks good.

@m50d m50d requested a review from ocadaruma March 3, 2021 01:05
@m50d
Copy link
Contributor Author

m50d commented Mar 3, 2021

I think build failure is a known flaky test?

@ocadaruma
Copy link
Member

Oh, it's not known but shouldn't be related to this PR's change...
I created a ticket about that. #94

Copy link
Member

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

@ocadaruma ocadaruma merged commit aa40e0c into line:master Mar 4, 2021
@m50d m50d deleted the trace-propagation branch March 4, 2021 06:01
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.

2 participants