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

Replace EmbeddedKafkaCluster with Testcontainers #129

Merged
merged 3 commits into from
Apr 22, 2020
Merged

Replace EmbeddedKafkaCluster with Testcontainers #129

merged 3 commits into from
Apr 22, 2020

Conversation

bsideup
Copy link
Contributor

@bsideup bsideup commented Apr 15, 2020

No description provided.

@bsideup bsideup added the type/chore A task not related to code (build, formatting, process, ...) label Apr 15, 2020
@bsideup bsideup marked this pull request as draft April 15, 2020 15:49
aneveu
aneveu approved these changes Apr 21, 2020
View reviewed changes
src/test/java/reactor/kafka/AbstractKafkaTest.java Outdated
embeddedKafka.startBroker(brokerId);
protected void shutdownKafkaBroker() {
assumeBrokerRestartSupport();
// TODO embeddedKafka.shutdownBroker(brokerId);
Copy link

Choose a reason for hiding this comment

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

not sure this TODO is still needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Replaced with Assert.fail so that (if we ever decide to add the broker restart scenario) it will remind us that we need to implement these methods 👍

src/test/java/reactor/kafka/AbstractKafkaTest.java Outdated
waitForTopic(topic, partitions, false);
protected void startKafkaBroker() {
assumeBrokerRestartSupport();
// TODO embeddedKafka.restartBroker(brokerId);
Copy link

Choose a reason for hiding this comment

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

same here

@bsideup bsideup marked this pull request as ready for review April 21, 2020 14:21
@bsideup bsideup requested a review from aneveu April 21, 2020 14:26
@bsideup bsideup added this to the 1.2.1.RELEASE milestone Apr 21, 2020
@codecov-io
Copy link

codecov-io commented Apr 21, 2020 •
edited
Loading

Codecov Report

Merging #129 into master will decrease coverage by 1.24%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #129      +/-   ##
============================================
- Coverage     81.98%   80.74%   -1.25%     
+ Complexity      232      230       -2     
============================================
  Files            19       19              
  Lines          1127     1127              
  Branches        114      114              
============================================
- Hits            924      910      -14     
- Misses          167      180      +13     
- Partials         36       37       +1
Impacted Files Coverage Δ Complexity Δ
...ctor/kafka/receiver/internals/KafkaSchedulers.java 76.74% <0.00%> (-4.66%) 2.00% <0.00%> (-1.00%)
...kafka/receiver/internals/DefaultKafkaReceiver.java 87.87% <0.00%> (-2.80%) 57.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8341b49...af671a7. Read the comment docs.

simonbasle
simonbasle reviewed Apr 21, 2020
View reviewed changes
.editorconfig Outdated
@@ -0,0 +1,22 @@
# EditorConfig is awesome: http://EditorConfig.org
Copy link
Member

@simonbasle simonbasle Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this could be made part of a separate commit / PR, wdyt @bsideup ?

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 could, but had a bit of a trouble fighting with IDEA's desire to collapse all imports into .* :D

Ok, will extract the change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted:
#134

@bsideup bsideup requested a review from OlegDokuka April 21, 2020 15:07
@bsideup bsideup requested a review from simonbasle April 22, 2020 12:45
@bsideup bsideup merged commit 3230d2d into master Apr 22, 2020
@bsideup bsideup deleted the migrate_to_testcontainers branch April 22, 2020 15:17
@bsideup bsideup modified the milestones: 1.2.1.RELEASE, 1.3.0-M1 Apr 30, 2020
@bsideup bsideup mentioned this pull request Apr 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbasle simonbasle simonbasle approved these changes

@aneveu aneveu Awaiting requested review from aneveu

@OlegDokuka OlegDokuka Awaiting requested review from OlegDokuka

Assignees
No one assigned
Labels
type/chore A task not related to code (build, formatting, process, ...)
Projects
None yet
Milestone
1.3.0-M1
Development

Successfully merging this pull request may close these issues.
4 participants
@bsideup @codecov-io @aneveu @simonbasle