search

firebase / geofire-java

GeoFire for Java - Realtime location queries with Firebase
MIT License
670 stars 271 forks source link

Create test and common modules #126

Closed samtstern closed 6 years ago

samtstern commented 6 years ago

Makes two new modules:

Also removes all use of deprecated APIs.

samtstern commented 6 years ago

This addresses #101

samtstern commented 6 years ago

@vanniktech which test classes did you envision exposing in a module? So far I just moved TestHelpers, GeoFireTestingRule and SimpleFuture

samtstern commented 6 years ago

A dep from test on the normal module leads to a circular dep error from maven.

On Wed, Jan 10, 2018, 12:54 PM Niklas Baudy notifications@github.com wrote:

@vanniktech commented on this pull request.

What do we need the common package for? I'd make the test package just have a dependency on the normal geofire unless this is not possible due to the java / android split.

I'll check tomorrow which other things we want to expose. I'd also, in a separate PR polish the APIs and add a bit of documentation so that this one won't blow up.

In java/src/test/java/com/firebase/geofire/GeoLocationTest.java https://github.com/firebase/geofire-java/pull/126#discussion_r160794799:

 @Test

public void geoLocationHasCorrectValues() {

  • Assert.assertEquals(new GeoLocation(1,2).latitude, 1.0);
  • Assert.assertEquals(new GeoLocation(1,2).longitude, 2.0);
  • Assert.assertEquals(new GeoLocation(0.000001,2).latitude, 0.000001);
  • Assert.assertEquals(new GeoLocation(0,0.000001).longitude, 0.000001);
  • assertEquals(new GeoLocation(1,2).latitude, 1.0, EPSILON);

you're using 2 spaces here now

In test/src/main/java/com/firebase/geofire/GeoFireTestingRule.java https://github.com/firebase/geofire-java/pull/126#discussion_r160794923:

     if (FirebaseApp.getApps().isEmpty()) {
  • final FirebaseCredential credentials;
  • final GoogleCredentials credentials;

Nice 👍

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/firebase/geofire-java/pull/126#pullrequestreview-87975675, or mute the thread https://github.com/notifications/unsubscribe-auth/AIEw6pdIX-It5DG9LewnVJejYEBaeXNjks5tJSOggaJpZM4RZwsL .

vanniktech commented 6 years ago

Huh that's interesting. In Gradle it just works when you use compile project(':geofire') from the test/build.gradle file.

samtstern commented 6 years ago

@vanniktech yeah I was surprised to have it not work but I just can't get it to behave without separating out common. But I don't think it's much of a problem, do you?

And agree, a separate PR to actually document all the test helpers and such. I just want to lay a foundation.

vanniktech commented 6 years ago

But I don't think it's much of a problem, do you?

Not it should not.

vanniktech commented 6 years ago

When executing mvn -U idea:idea for generating the idea related files - Otherwise the project does not work from AS in my case - yields 

Could not resolve dependencies for project com.firebase:geofire-android:aar:2.2.1-SNAPSHOT: Failure to find com.firebase:geofire-common:jar:2.2.1-SNAPSHOT in file:///Users/nik/android/sdk/extras/google/m2repository was cached in the local repository, resolution will not be reattempted until the update interval of google-extras has elapsed or updates are forced
vanniktech commented 6 years ago

Why is com.firebase.geofire.example.Example in the common package? Seems like it's not belonging there.

vanniktech commented 6 years ago

Let me know how we can proceed. We could make the changes here or later in separate PRs.

samtstern commented 6 years ago

@vanniktech I think I addressed all of your comments except for the failure about mvn -U idea:idea. Not quite sure about that one.

It works if I run mvn install first but what I want is for the modules to depend on each other like you can get in gradle, not depending on JARs in the maven local repo.

vanniktech commented 6 years ago

@samtstern I haven't forgotten about this. I'll pick this up next week due to some priority changes.