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

Change test methods visibility. #556

Closed
wants to merge 2 commits into from
Closed

Change test methods visibility. #556

wants to merge 2 commits into from

Conversation

arturdryomov
Copy link
Contributor

Tests are passing locally: 187 of 187.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 4, 2019
@@ -47,10 +47,6 @@ public String getSnippet() {
}
}

public void setUp() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unused setUp blocks there and there.

@@ -24,10 +24,10 @@

public class QuadItemTest {

public class TestingItem implements ClusterItem {
private class TestingItem implements ClusterItem {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made everything fixture-related private.

@@ -116,6 +118,7 @@ public void testGeometry() {
Assert.assertEquals(lineString, feature.getGeometry());
}

@Test
public void testGetBoundingBox() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These three were not annotation-marked so were not running before.

jpoehnelt
jpoehnelt previously approved these changes Oct 4, 2019
Copy link
Contributor

@jpoehnelt jpoehnelt left a comment

Choose a reason for hiding this comment

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

LGTM. WIll take a look again after conflict resolution before merging.

@arturdryomov
Copy link
Contributor Author

@jpoehnelt, resolved. Please squash commits while merging.

@friederbluemle
Copy link
Contributor

Hi @ming13 - Would you be merging #560 first and rebasing your PR?

@friederbluemle
Copy link
Contributor

Hi @ming13 - So sorry, it looks like #560 created a bunch of conflicts with your branch. I've reapplied the remaining changes from your commit on top of the latest master, you can fetch it from here for your convenience: https://github.com/friederbluemle/android-maps-utils/tree/test-visibility-rebased
Once you've updated your branch, we can merge this PR. 👍

@jpoehnelt jpoehnelt added the triage me I really want to be triaged. label Oct 14, 2019
@jpoehnelt jpoehnelt dismissed their stale review October 15, 2019 18:31

waiting until conflict resolution

@friederbluemle
Copy link
Contributor

@jpoehnelt @ming13 - The changes here still make sense. I created a new PR #569 with the rebased commit (left the author as @ming13). Please check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. triage me I really want to be triaged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants