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

Enable test code sharing #197

Merged
merged 1 commit into from Jul 16, 2015

Conversation

Projects
None yet
5 participants
@larsp
Copy link
Contributor

larsp commented Jul 11, 2015

Becomes useful e.g. when integration testing with the schema-registry (RestApp)

Enable test code sharing
Becomes useful e.g. when integration testing with the schema-registry (RestApp)
@ConfluentJenkins

This comment has been minimized.

Copy link
Contributor

ConfluentJenkins commented Jul 11, 2015

Can one of the admins verify this patch?

@ConfluentCLABot

This comment has been minimized.

Copy link

ConfluentCLABot commented Jul 11, 2015

Hey @larsp,
thank you for your Pull Request.

It looks like you haven't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@larsp

This comment has been minimized.

Copy link
Contributor Author

larsp commented Jul 11, 2015

[clabot:check]

@ConfluentCLABot

This comment has been minimized.

Copy link

ConfluentCLABot commented Jul 11, 2015

@confluentinc It looks like @larsp just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@ewencp

This comment has been minimized.

Copy link
Member

ewencp commented Jul 11, 2015

add to whitelist

@ConfluentJenkins

This comment has been minimized.

Copy link
Contributor

ConfluentJenkins commented Jul 11, 2015

Refer to this link for build results (access rights to CI server needed):
http://jenkins.confluent.io/job/schema-registry-pr/25/
Test PASSed.

@ewencp

This comment has been minimized.

Copy link
Member

ewencp commented Jul 12, 2015

@larsp I don't think this patch results in any test classes getting packaged in the jar -- it creates a test-jar for the parent project (since you added the test-jar command to the top-level pom.xml), but the parent project has no test classes so you just get a jar file doesn't contain anything useful.

I'm also not sure we want to use the maven-jar-plugin's test-jar -- their own documentation suggests moving the code to a separate project so you get transitive dependencies: https://maven.apache.org/plugins/maven-jar-plugin/examples/create-test-jar.html (under "the preferred way").

Which set of classes are you trying to reuse elsewhere? I'm guessing you want ClusterTestHarness since I don't see much else that could be reused. If it's a very limited set of classes from the kafka-schema-registry (a.k.a. core/) project, maybe we can refactor those into a kafka-schema-registry-test project as the maven-jar-plugin docs suggest?

@larsp

This comment has been minimized.

Copy link
Contributor Author

larsp commented Jul 12, 2015

Hi @ewencp, you are right that it will create a test jar for the parent pom. Nevertheless is the parent pom referenced as parent in the other module poms and therefore inherits the plugin execution:
original:
$ find . -name "*-tests.jar"

fork:

$ find . -name "*-tests.jar"
./avro-serializer/target/kafka-avro-serializer-2.0-SNAPSHOT-tests.jar
./client/target/kafka-schema-registry-client-2.0-SNAPSHOT-tests.jar
./core/target/kafka-schema-registry-2.0-SNAPSHOT-tests.jar
./json-serializer/target/kafka-json-serializer-2.0-SNAPSHOT-tests.jar
./target/kafka-schema-registry-parent-2.0-SNAPSHOT-tests.jar

afterwards each individual test jar can be loaded in maven using the classifier tests such as:

<dependency>
    <groupId>io.confluent</groupId>
    <artifactId>kafka-schema-registry</artifactId>
    <version>2.0-SNAPSHOT</version>
    <classifier>tests</classifier>
    <scope>test</scope>
</dependency>

Or in gradle via ["io.confluent:kafka-schema-registry:$confluentVersion:tests"]

I wanted to use RestApp from the core tests and this seemed the easiest way of doing it. I am also happy if this test code gets refactored to some other location when transitive dependencies of the usual test libraries such as JUnit, hamcrest, etc. can be avoided. Which shouldn't be a problem for this particular class, but I am not sure if this is also applicable for the rest of the test utilities?

@ewencp

This comment has been minimized.

Copy link
Member

ewencp commented Jul 16, 2015

@larsp Of course you are correct, I must have been working from a dirty build and missed those test jars.

I'm going to merge this patch. Since everything is in test-jars, I'd still consider them private and therefore unsupported, no compatibility guarantees etc. I think if we want to provide something that will be consistently reusable, even across releases, we should probably try to refactor just those pieces into a separate module. But we can also just do that in a follow up patch.

I've also followed this up with equivalent commits in confluentinc/common, confluentinc/rest-utils, and confluentinc/kafka-rest just so the jars will get published. confluentinc/camus already had the relevant config in the pom file.

@ewencp

This comment has been minimized.

Copy link
Member

ewencp commented Jul 16, 2015

@larsp And thanks for the patch!

ewencp added a commit that referenced this pull request Jul 16, 2015

@ewencp ewencp merged commit e3366d0 into confluentinc:master Jul 16, 2015

1 check passed

default Build finished.
Details
@sharego

This comment has been minimized.

Copy link

sharego commented Aug 22, 2016

[clabot:check]

@ConfluentCLABot

This comment has been minimized.

Copy link

ConfluentCLABot commented Aug 22, 2016

@confluentinc It looks like @larsp just signed our Contributor License Agreement. 👍

Always at your service,

clabot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.