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

KAFKA-4657: Improve test coverage of CompositeReadOnlyWindowStore #2672

Closed
wants to merge 3 commits into from
Closed

Conversation

adyach
Copy link
Contributor

@adyach adyach commented Mar 10, 2017

This commmit brings improved test coverage for window store fetch method
and WindowStoreIterator

This commmit brings improved test coverage for window store fetch method
and WindowStoreIterator
@adyach
Copy link
Contributor Author

adyach commented Mar 10, 2017

@guozhangwang @dguy please review when you have time

@asfbot
Copy link

asfbot commented Mar 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2117/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Mar 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2120/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Mar 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2118/
Test FAILed (JDK 8 and Scala 2.12).

@guozhangwang
Copy link
Contributor

thanks @adyach , @dguy @mjsax could you take a look?

Copy link
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @adyach. Appreciated. I just left one comment.

}

@Test
public void testWindowStoreIterator() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split this out into 2 tests please?. One for each method that is being tested. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dguy done

@asfbot
Copy link

asfbot commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2131/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2128/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2129/
Test FAILed (JDK 8 and Scala 2.12).

}

@Test
public void testWindowStoreIteratorHasNext() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

it is preferable to have descriptive test names, i.e.,
emptyIteratorAlwaysReturnsFalse
emptyIteratorPeekNextKeyShouldThrowNoSuchElementException
etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 90e6816

@asfbot
Copy link

asfbot commented Mar 13, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2138/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Mar 13, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2140/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Mar 13, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2137/
Test PASSed (JDK 7 and Scala 2.10).

Copy link
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

Thanks @adyach - appreciated. LGTM

@asfgit asfgit closed this in 85e7a0a Mar 13, 2017
@guozhangwang
Copy link
Contributor

LGTM and merged to trunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants