Skip to content

KAFKA-3612: Added structure for integration tests#1260

Closed
enothereska wants to merge 9 commits into
apache:trunkfrom
enothereska:KAFKA-3612-integration-tests
Closed

KAFKA-3612: Added structure for integration tests#1260
enothereska wants to merge 9 commits into
apache:trunkfrom
enothereska:KAFKA-3612-integration-tests

Conversation

@enothereska
Copy link
Copy Markdown
Contributor

No description provided.

@enothereska
Copy link
Copy Markdown
Contributor Author

@dguy @ijuma @guozhangwang @miguno please have a look if you can.

Comment thread gradle/dependencies.gradle Outdated
zkclient: "0.8",
zookeeper: "3.4.6",
curator: "2.9.0",
assertj: "3.3.0",
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.

This doesn't seem to be used.

@enothereska enothereska changed the title KAFKA-3612: Added structure for integration tests KAFKA-3612: Added structure for integration tests [WiP] Apr 23, 2016
/**
* Runs an in-memory, "embedded" Kafka cluster with 1 ZooKeeper instance and 1 Kafka broker.
*/
public class EmbeddedSingleNodeKafkaCluster {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd consider making this extend org.junit.rules.ExternalResource - it can then be used as a JUnit ClassRule or Rule. The benefits being that the JUnit framework takes care of startup and shutdown

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's a good idea.

Note: Kafka does not use this JUnit functionality yet (i.e. no use of ExternalResource, ClassRule, Rule as far as I can tell). @ijuma: Would it ok for us to introduce this? There's no additional dependency etc., it's just using a new JUnit feature that was introduced in 4.7 (we're on 4.12).

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.

Yes, it's fine to use that, I think. @guozhangwang is using TemporaryFolder in his PR, for example. However, when I looked at the TemporaryFolder implementation, the implementation is not great, see my comment here: #1258 (comment)

@enothereska
Copy link
Copy Markdown
Contributor Author

Thanks @dguy @ijuma @miguno

@enothereska enothereska changed the title KAFKA-3612: Added structure for integration tests [WiP] KAFKA-3612: Added structure for integration tests Apr 24, 2016
@miguno
Copy link
Copy Markdown
Contributor

miguno commented Apr 24, 2016

Thanks for your feedback, @dguy!

public void start() {
log.debug("Starting embedded Kafka broker at {} (with log.dirs={} and ZK ensemble at {}) ...",
brokerList(), logDir, zookeeperConnect());
// already started in constructore
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo -- or is that Italian? :-)

@enothereska
Copy link
Copy Markdown
Contributor Author

@miguno I brought in one more utility method (borrowed from you), as well as swapped the stores test, which is not great, with the one that tests compact topics which is a better starting point.

/**
* Tests related to internal topics in streams
*
* Note: This example uses lambda expressions and thus works with Java 8+ only.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This code doesn't use lambdas (anymore), so I'd remove this comment.

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.

5 participants