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

Increase kafka client test coverage #8237

Merged
merged 2 commits into from
Apr 24, 2020

Conversation

cescoffier
Copy link
Member

Fixes #8025

add a few tests related to the Kafka client.

Unfortunately, Avro does not work in native mode (fields written using unsafe).
I will create an issue to track this.

@cescoffier
Copy link
Member Author

Forgot to disable the test if docker is not enabled. Will fix that.

@cescoffier cescoffier self-assigned this Mar 31, 2020
@cescoffier cescoffier force-pushed the features/kafka-client-tests branch from 545c6bf to 3c88d1e Compare March 31, 2020 09:48
Copy link
Contributor

@gunnarmorling gunnarmorling left a comment

Choose a reason for hiding this comment

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

LGTM, a few minor comments inline.

@gsmet
Copy link
Member

gsmet commented Apr 10, 2020

@cescoffier can you check Gunnar's comments?

@cescoffier
Copy link
Member Author

@gunnarmorling I need to look at the Apicurio version of the Avro serde. So far the need was the "original" Avro. I can see an opportunity here if it works flawlessly in native.
I have created #8541.

@cescoffier cescoffier force-pushed the features/kafka-client-tests branch 3 times, most recently from dca5dfd to 98762cf Compare April 12, 2020 12:11
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I really don't like the fact that we have versions in the pom files. They should either be in the BOM if it makes sense or in the build-parent.

@cescoffier
Copy link
Member Author

As these artifacts are not available from Maven Central it's tricky to add them to the dependency management from the parent. That's why I put the version there on purpose.

Someone not aware of this would add the dependency that won't get resolved.
Note that we should NOT declare the repository in the parent either.

@gsmet gsmet force-pushed the features/kafka-client-tests branch from 98762cf to 937fa10 Compare April 24, 2020 08:21
@gsmet
Copy link
Member

gsmet commented Apr 24, 2020

I rebased and force pushed as it was an old PR. I'm a bit worried about the external repository. We'll see if it causes problems and reevaluate.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 24, 2020
@gsmet gsmet added this to the 1.5.0 milestone Apr 24, 2020
@gsmet gsmet merged commit c3c0b2f into quarkusio:master Apr 24, 2020
@cescoffier cescoffier deleted the features/kafka-client-tests branch April 24, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kafka triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase test coverage of the Kafka client
3 participants