Skip to content

MINOR: Depend on streams:test-utils for streams and examples tests#4760

Merged
guozhangwang merged 1 commit into
apache:trunkfrom
vvcephei:streams-test-utils-gradle-deps
Mar 28, 2018
Merged

MINOR: Depend on streams:test-utils for streams and examples tests#4760
guozhangwang merged 1 commit into
apache:trunkfrom
vvcephei:streams-test-utils-gradle-deps

Conversation

@vvcephei

Copy link
Copy Markdown
Contributor

Link :streams:test-utils as a test dependency for :streams and :streams:examples, avoiding a circular dependency.

Testing strategy: the project should build.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@mjsax mjsax requested review from guozhangwang and mjsax March 23, 2018 04:04
@mjsax mjsax added the streams label Mar 23, 2018
@mjsax

mjsax commented Mar 23, 2018

Copy link
Copy Markdown
Member

\cc @bbejeck

@mjsax

mjsax commented Mar 23, 2018

Copy link
Copy Markdown
Member

Retest this please.

@asfgit

asfgit commented Mar 23, 2018

Copy link
Copy Markdown

FAILURE
8649 tests run, 7 skipped, 1 failed.
--none--

@vvcephei

Copy link
Copy Markdown
Contributor Author

Failure was:

00:43:00.234 kafka.server.DynamicBrokerReconfigurationTest > testAddRemoveSslListener FAILED
00:43:00.234     java.lang.AssertionError: expected:<10> but was:<11>
00:43:00.234         at org.junit.Assert.fail(Assert.java:88)
00:43:00.234         at org.junit.Assert.failNotEquals(Assert.java:834)
00:43:00.234         at org.junit.Assert.assertEquals(Assert.java:645)
00:43:00.234         at org.junit.Assert.assertEquals(Assert.java:631)
00:43:00.234         at kafka.server.DynamicBrokerReconfigurationTest.verifyProduceConsume(DynamicBrokerReconfigurationTest.scala:956)
00:43:00.234         at kafka.server.DynamicBrokerReconfigurationTest.verifyListener(DynamicBrokerReconfigurationTest.scala:827)
00:43:00.234         at kafka.server.DynamicBrokerReconfigurationTest.testAddRemoveSslListener(DynamicBrokerReconfigurationTest.scala:703)

(https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/12005)

retest this please

@bbejeck bbejeck left a comment

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.

Thanks @vvcephei, LGTM

@vvcephei

Copy link
Copy Markdown
Contributor Author

The discussion on the mailing list has had no further comments recently. I went ahead and rebased it on current trunk.

@mjsax @guozhangwang assuming the tests pass, should we go ahead and merge this? Or just wait until there's a changeset that requires it?

@guozhangwang

Copy link
Copy Markdown
Contributor

retest this please

@vvcephei

Copy link
Copy Markdown
Contributor Author

@mjsax @guozhangwang : can we merge this?

@guozhangwang guozhangwang merged commit 659fbb0 into apache:trunk Mar 28, 2018

@guozhangwang guozhangwang left a comment

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.

LGTM!

@vvcephei vvcephei deleted the streams-test-utils-gradle-deps branch March 28, 2018 18:51
ewencp added a commit that referenced this pull request Apr 3, 2018
…tarballs

#4760 unintentionally included extra raw class files in the release tarballs by making the .class file output (instead of the jar) for a streams a dependency of the streams-test-utils. This fixes that issue by instead breaking the circular dependency by using a `compileOnly`/`provided` dependency on those sources and also including the dependency as a test dependency.

I verified by using `gradlew clean installAll releaseTarGzAll`, then checking that the release tarball doesn't have the extraneous files and the installed pom file has the expected dependencies. The dependency on kafka-streams is now in the `test` scope, but that should be fine since a streams application would only use this dependency if it already had a dependency on streams in `compile` (or in weird edge cases the user could handle specifying the right dependencies). This actually seems to even be an improvement over the previous situation where the actual dependency was not expressed in the pom at all (since the dependency was on the sourceSet output rather than the actual project).

Author: Ewen Cheslack-Postava <me@ewencp.org>

Reviewers: Matthias J. Sax <mjsax@apache.org>, Guozhang Wang <wangguoz@gmail.com>, John Roesler <vvcephei@users.noreply.github.com>

Closes #4816 from ewencp/fix-streams-dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants