Skip to content

MINOR: add unit test for KGroupedTable.count#1255

Closed
dguy wants to merge 4 commits into
apache:trunkfrom
dguy:kgroupedtable-count-test
Closed

MINOR: add unit test for KGroupedTable.count#1255
dguy wants to merge 4 commits into
apache:trunkfrom
dguy:kgroupedtable-count-test

Conversation

@dguy
Copy link
Copy Markdown
Contributor

@dguy dguy commented Apr 22, 2016

No description provided.

import static org.junit.Assert.assertThat;


public class KGroupedTableCountTest {
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.

Could you rename this class file to KGroupedTableImplTest?

@guozhangwang
Copy link
Copy Markdown
Contributor

Checkstyle failure: streams:checkstyleTest[ant:checkstyle] /x1/jenkins/jenkins-slave/workspace/kafka-trunk-git-pr-jdk7/streams/src/test/java/org/apache/kafka/streams/kstream/internals/KGroupedTableImplCountTest.java:34:1: Disallowed import - org.hamcrest.core.Is.is.

Do we have to introduce this dependency? In general we want to have as less external dependencies as possible.

@dguy
Copy link
Copy Markdown
Contributor Author

dguy commented Apr 22, 2016

  1. It is a test dependency - so api users don't see it.
  2. it doesn't include any new jars.
  3. This has been a standard way of writing junit tests for a number of years

@miguno
Copy link
Copy Markdown
Contributor

miguno commented Apr 25, 2016

@dguy: Some background information regarding your earlier comment: The Kafka project has been pretty sensitive to introducing new dependencies, including test-scope dependencies. That said, I'm on your side. :-) This question is perhaps something we should discuss with other Kafka folks, because -- intentionally over-simplified -- I'd prefer us not to be stuck with assertEquals until eternity...

@miguno
Copy link
Copy Markdown
Contributor

miguno commented Apr 25, 2016

@guozhangwang : Can you take a look again? It'd be great to include this test improvement before the 0.10 release.

import static org.junit.Assert.assertEquals;


public class KGroupedTableImplCountTest {
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.

nit: we can rename it to KGroupedTableImplTest and add more test cases into this class in the future.

@guozhangwang
Copy link
Copy Markdown
Contributor

Jenkins failure is tracked in #1258. LGTM and merged to trunk. Thanks @dguy !

@asfgit asfgit closed this in 1b764c5 Apr 25, 2016
gfodor pushed a commit to AltspaceVR/kafka that referenced this pull request Jun 3, 2016
Author: Damian Guy <damian.guy@gmail.com>

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Michael G. Noll <michael@confluent.io>

Closes apache#1255 from dguy/kgroupedtable-count-test
@dguy dguy deleted the kgroupedtable-count-test branch August 3, 2016 08:25
efeg added a commit to efeg/kafka that referenced this pull request May 29, 2024
mumrah pushed a commit to mumrah/kafka that referenced this pull request Aug 14, 2024
* Find coordinator RPC impl for share group CRUD RPCs.
* Added PersisterStateManager to handle RPCs.
* Added PersisterConfig class to supply config for Persister.
* Updated Persister interface to add `stop` and `configure` methods.
* Updated broker server which now instantiates and manages the persister instance creation and pass it to appropriate classes.

Major update:
__share_group_state topic is getting created if not exisiting
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.

3 participants