10BC0 adding kafka test container support by ash1425 · Pull Request #127 · testcontainers/testcontainers-python · GitHub
[go: up one dir, main page]
Skip to content

adding kafka test container support #127

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
Merged
tillahoffmann merged 3 commits into from
Mar 29, 2021
Merged

adding kafka test container support #127

tillahoffmann merged 3 commits into from
Mar 29, 2021

Conversation

@ash1425
Copy link
Contributor
@ash1425 ash1425 commented Jan 5, 2021 •
edited
Loading

fix #121 This PR adds support for kafka container. Heavily inspired from java testcontainer's Kafka module.

@ash1425 ash1425 changed the title adding kafka test container support #121 adding kafka test container support Jan 5, 2021
@ash1425
Copy link
Contributor Author
ash1425 commented Jan 7, 2021

I tried these failing tests locally, They work in all python versions (used venv) but were flaky and I had to try multiple time.
mssql test didnt work locally too.

@SrinivasanTarget
Copy link
SrinivasanTarget commented Jan 8, 2021

@tillahoffmann Can you please review this change?

tillahoffmann
tillahoffmann reviewed Jan 8, 2021
View reviewed changes
Copy link
Contributor
@tillahoffmann tillahoffmann 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 addition! Some comments inline.

@ash1425
Copy link
Contributor Author
ash1425 commented Jan 16, 2021

@tillahoffmann Thanks for the review comments. I have accommodated those in latest commit.

mamachanko
mamachanko reviewed Jan 17, 2021
View reviewed changes
@mamachanko mamachanko mentioned this pull request Jan 19, 2021
Open
3 tasks
mamachanko
mamachanko reviewed Jan 21, 2021
View reviewed changes
mamachanko
mamachanko suggested changes Jan 26, 2021
View reviewed changes
mamachanko
mamachanko suggested changes Jan 27, 2021
View reviewed changes
@ash1425
Copy link
Contributor Author
ash1425 commented Jan 27, 2021 •
edited
Loading

assert consumer.end_offsets([tp])[tp] == 1, "Expected exactly one test message to be present on test topic !"

This is going to ensure that we will have one and only one message in the consumer in integration test.

@mamachanko
Copy link
mamachanko commented Jan 28, 2021

assert consumer.end_offsets([tp])[tp] == 1, "Expected exactly one test message to be present on test topic !"

This is going to ensure that we will have one and only one message in the consumer in integration test.

I understand. Then it looks like the for loop and the assertion within don't add confidence to the test. Would you consider to scrap them?

@t0t07
Copy link
t0t07 commented Mar 22, 2021

@ash1425 Hey Ashay, do you have bandwidth working on the PR? If not, maybe I can help to cross the finish line?

@ash1425
Copy link
Contributor Author
ash1425 commented Mar 24, 2021

@YikSanChan
Thanks for offering to help. I have invited you as collaborator to my fork.

For now, I have incorporated all the review comments and merged upstream changes to resolve the conflicts. I think we should be good to approve and merge if all tests pass.

@t0t07
Copy link
t0t07 commented Mar 24, 2021

@ash1425 Great thanks! If there is any more comment from testcontainers members, I can try to resolve

@t0t07
Copy link
t0t07 commented Mar 24, 2021

@tillahoffmann Could you please taking a look? This is the feature I am looking for. Thank you!

@SrinivasanTarget
Copy link
SrinivasanTarget commented Mar 25, 2021

@tillahoffmann CI seems happy. Are we good to get these changes in?

@tillahoffmann tillahoffmann merged commit bc969c5 into testcontainers:master Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@tillahoffmann tillahoffmann tillahoffmann approved these changes

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

How to create a Kafka test Container

5 participants

@ash1425 @SrinivasanTarget @mamachanko @t0t07 @tillahoffmann
0