From 2e58e27e6e37212002acee2c7d000819961b12e9 Mon Sep 17 00:00:00 2001 From: Sam Stern Date: Thu, 21 Dec 2017 10:54:20 -0800 Subject: [PATCH 1/2] Add basic tests, failing --- .../java/com/firebase/geofire/GeoQuery.java | 31 ++++--- .../geofire/GeoQueryDataEventListener.java | 5 +- .../GeoQueryDataEventTestListener.java | 80 +++++++++++++++++++ .../com/firebase/geofire/GeoQueryTest.java | 36 +++++++++ 4 files changed, 142 insertions(+), 10 deletions(-) create mode 100644 src/test/java/com/firebase/geofire/GeoQueryDataEventTestListener.java diff --git a/src/main/java/com/firebase/geofire/GeoQuery.java b/src/main/java/com/firebase/geofire/GeoQuery.java index 3fc290f..d17d33c 100644 --- a/src/main/java/com/firebase/geofire/GeoQuery.java +++ b/src/main/java/com/firebase/geofire/GeoQuery.java @@ -128,7 +128,8 @@ private void updateLocationInfo(final DataSnapshot dataSnapshot, final GeoLocati String key = dataSnapshot.getKey(); LocationInfo oldInfo = this.locationInfos.get(key); boolean isNew = oldInfo == null; - final boolean changedLocation = oldInfo != null && !oldInfo.location.equals(location); + boolean changedLocation = oldInfo != null && !oldInfo.location.equals(location); + boolean changedData = oldInfo != null && !oldInfo.dataSnapshot.equals(dataSnapshot); boolean wasInQuery = oldInfo != null && oldInfo.inGeoQuery; boolean isInQuery = this.locationIsInQuery(location); @@ -142,17 +143,29 @@ public void run() { }); } } else if (!isNew && isInQuery) { - for (final GeoQueryDataEventListener listener: this.eventListeners) { - this.geoFire.raiseEvent(new Runnable() { - @Override - public void run() { - if (changedLocation) { + // Data moved + if (changedLocation) { + for (final GeoQueryDataEventListener listener: this.eventListeners) { + this.geoFire.raiseEvent(new Runnable() { + @Override + public void run() { listener.onDataMoved(dataSnapshot, location); } + }); + } + } - listener.onDataChanged(dataSnapshot, location); - } - }); + // Data changed + // TODO: Check this + if (changedData) { + for (final GeoQueryDataEventListener listener: this.eventListeners) { + this.geoFire.raiseEvent(new Runnable() { + @Override + public void run() { + listener.onDataChanged(dataSnapshot, location); + } + }); + } } } else if (wasInQuery && !isInQuery) { for (final GeoQueryDataEventListener listener: this.eventListeners) { diff --git a/src/main/java/com/firebase/geofire/GeoQueryDataEventListener.java b/src/main/java/com/firebase/geofire/GeoQueryDataEventListener.java index a8b4e39..a8399da 100644 --- a/src/main/java/com/firebase/geofire/GeoQueryDataEventListener.java +++ b/src/main/java/com/firebase/geofire/GeoQueryDataEventListener.java @@ -67,7 +67,10 @@ public interface GeoQueryDataEventListener { /** * Called if a dataSnapshot changed within the search area. - * An onDataMoved() event would always be preceded by onDataChanged() but it would be possible to see onDataChanged() without an antecedent onDataMoved(). + * An onDataMoved() is always followed by onDataChanged() but it is be possible to see + * onDataChanged() without an preceding onDataMoved(). + * + * Note: this method is not related to ValueEventListener#onDataChange(DataSnapshot). * * This method can be called multiple times. * diff --git a/src/test/java/com/firebase/geofire/GeoQueryDataEventTestListener.java b/src/test/java/com/firebase/geofire/GeoQueryDataEventTestListener.java new file mode 100644 index 0000000..923b5aa --- /dev/null +++ b/src/test/java/com/firebase/geofire/GeoQueryDataEventTestListener.java @@ -0,0 +1,80 @@ +package com.firebase.geofire; + +import static java.util.Locale.US; + +import com.google.firebase.database.DatabaseError; +import com.google.firebase.database.DataSnapshot; + +public class GeoQueryDataEventTestListener extends TestListener implements GeoQueryDataEventListener { + + private boolean recordEntered; + private boolean recordMoved; + private boolean recordChanged; + private boolean recordExited; + + public static String dataEntered(String key, double latitude, double longitude) { + return String.format(US, "DATA_ENTERED(%s,%f,%f)", key, latitude, longitude); + } + + public static String dataExited(String key) { + return String.format("DATA_EXITED(%s)", key); + } + + public static String dataMoved(String key, double latitude, double longitude) { + return String.format(US, "DATA_MOVED(%s,%f,%f)", key, latitude, longitude); + } + + public static String dataChanged(String key, double latitude, double longitude) { + return String.format(US, "DATA_CHANGED(%s,%f,%f)", key, latitude, longitude); + } + + public GeoQueryDataEventTestListener(boolean recordEntered, + boolean recordMoved, + boolean recordChanged, + boolean recordExited) { + this.recordEntered = recordEntered; + this.recordMoved = recordMoved; + this.recordChanged = recordChanged; + this.recordExited = recordExited; + + } + + @Override + public void onDataEntered(DataSnapshot dataSnapshot, GeoLocation location) { + if (recordEntered) { + this.addEvent(dataEntered(dataSnapshot.getKey(), location.latitude, location.longitude)); + } + } + + @Override + public void onDataExited(DataSnapshot dataSnapshot) { + if (recordExited) { + this.addEvent(dataExited(dataSnapshot.getKey())); + } + } + + @Override + public void onDataMoved(DataSnapshot dataSnapshot, GeoLocation location) { + if (recordMoved) { + this.addEvent(dataMoved(dataSnapshot.getKey(), location.latitude, location.longitude)); + } + } + + @Override + public void onDataChanged(DataSnapshot dataSnapshot, GeoLocation location) { + if (recordChanged) { + this.addEvent(dataChanged(dataSnapshot.getKey(), location.latitude, location.longitude)); + } + } + + @Override + public void onGeoQueryReady() { + + } + + @Override + public void onGeoQueryError(DatabaseError error) { + + } + +} diff --git a/src/test/java/com/firebase/geofire/GeoQueryTest.java b/src/test/java/com/firebase/geofire/GeoQueryTest.java index d9978f3..db86e8c 100644 --- a/src/test/java/com/firebase/geofire/GeoQueryTest.java +++ b/src/test/java/com/firebase/geofire/GeoQueryTest.java @@ -109,6 +109,42 @@ public void keyMoved() throws InterruptedException { testListener.expectEvents(events); } + @Test + public void dataChanged() throws InterruptedException { + GeoFire geoFire = newTestGeoFire(); + setLoc(geoFire, "0", 0, 0); + setLoc(geoFire, "1", 37.0000, -122.0000); + setLoc(geoFire, "2", 37.0001, -122.0001); + setLoc(geoFire, "3", 37.1000, -122.0000); + setLoc(geoFire, "4", 37.0002, -121.9998, true); + + GeoQuery query = geoFire.queryAtLocation(new GeoLocation(37, -122), 0.5); + + GeoQueryDataEventTestListener testListener = new GeoQueryDataEventTestListener( + false, true, true, false); + query.addGeoQueryDataEventListener(testListener); + + waitForGeoFireReady(geoFire); + + setLoc(geoFire, "0", 1, 1); // outside of query + setLoc(geoFire, "1", 37.0001, -122.0000); // moved + setLoc(geoFire, "2", 37.0001, -122.0001); // location stayed the same + setLoc(geoFire, "4", 37.0002, -122.0000, true); // moved + setValueAndWait(geoFire.getDatabaseRefForKey("2").child("some_child"), "some_value"); // data changed + + + List events = new LinkedList<>(); + events.add(GeoQueryDataEventTestListener.dataMoved("1", 37.0001, -122.0000)); + events.add(GeoQueryDataEventTestListener.dataChanged("1", 37.0001, -122.0000)); + + events.add(GeoQueryDataEventTestListener.dataMoved("4", 37.0002, -122.0000)); + events.add(GeoQueryDataEventTestListener.dataChanged("4", 37.0002, -122.0000)); + + events.add(GeoQueryDataEventTestListener.dataChanged("2", 37.0001, -122.0001)); + + testListener.expectEvents(events); + } + @Test public void subQueryTriggersKeyMoved() throws InterruptedException { GeoFire geoFire = newTestGeoFire(); From 1b2be354f02f88f6d7bb4dab4ad047d263317304 Mon Sep 17 00:00:00 2001 From: Sam Stern Date: Thu, 21 Dec 2017 15:09:52 -0800 Subject: [PATCH 2/2] Make tests pass, discovered floating point issue. --- .../java/com/firebase/geofire/GeoQuery.java | 31 ++++++------------- .../geofire/GeoQueryDataEventListener.java | 6 ++-- .../com/firebase/geofire/GeoQueryTest.java | 22 +++++++------ 3 files changed, 25 insertions(+), 34 deletions(-) diff --git a/src/main/java/com/firebase/geofire/GeoQuery.java b/src/main/java/com/firebase/geofire/GeoQuery.java index d17d33c..3fc290f 100644 --- a/src/main/java/com/firebase/geofire/GeoQuery.java +++ b/src/main/java/com/firebase/geofire/GeoQuery.java @@ -128,8 +128,7 @@ private void updateLocationInfo(final DataSnapshot dataSnapshot, final GeoLocati String key = dataSnapshot.getKey(); LocationInfo oldInfo = this.locationInfos.get(key); boolean isNew = oldInfo == null; - boolean changedLocation = oldInfo != null && !oldInfo.location.equals(location); - boolean changedData = oldInfo != null && !oldInfo.dataSnapshot.equals(dataSnapshot); + final boolean changedLocation = oldInfo != null && !oldInfo.location.equals(location); boolean wasInQuery = oldInfo != null && oldInfo.inGeoQuery; boolean isInQuery = this.locationIsInQuery(location); @@ -143,29 +142,17 @@ public void run() { }); } } else if (!isNew && isInQuery) { - // Data moved - if (changedLocation) { - for (final GeoQueryDataEventListener listener: this.eventListeners) { - this.geoFire.raiseEvent(new Runnable() { - @Override - public void run() { + for (final GeoQueryDataEventListener listener: this.eventListeners) { + this.geoFire.raiseEvent(new Runnable() { + @Override + public void run() { + if (changedLocation) { listener.onDataMoved(dataSnapshot, location); } - }); - } - } - // Data changed - // TODO: Check this - if (changedData) { - for (final GeoQueryDataEventListener listener: this.eventListeners) { - this.geoFire.raiseEvent(new Runnable() { - @Override - public void run() { - listener.onDataChanged(dataSnapshot, location); - } - }); - } + listener.onDataChanged(dataSnapshot, location); + } + }); } } else if (wasInQuery && !isInQuery) { for (final GeoQueryDataEventListener listener: this.eventListeners) { diff --git a/src/main/java/com/firebase/geofire/GeoQueryDataEventListener.java b/src/main/java/com/firebase/geofire/GeoQueryDataEventListener.java index a8399da..0537380 100644 --- a/src/main/java/com/firebase/geofire/GeoQueryDataEventListener.java +++ b/src/main/java/com/firebase/geofire/GeoQueryDataEventListener.java @@ -67,12 +67,14 @@ public interface GeoQueryDataEventListener { /** * Called if a dataSnapshot changed within the search area. + * * An onDataMoved() is always followed by onDataChanged() but it is be possible to see * onDataChanged() without an preceding onDataMoved(). * - * Note: this method is not related to ValueEventListener#onDataChange(DataSnapshot). + * This method can be called multiple times for a single location change, due to the way + * the Realtime Database handles floating point numbers. * - * This method can be called multiple times. + * Note: this method is not related to ValueEventListener#onDataChange(DataSnapshot). * * @param dataSnapshot The associated dataSnapshot that moved within the search area * @param location The location for this dataSnapshot as a GeoLocation object diff --git a/src/test/java/com/firebase/geofire/GeoQueryTest.java b/src/test/java/com/firebase/geofire/GeoQueryTest.java index db86e8c..3c76a9e 100644 --- a/src/test/java/com/firebase/geofire/GeoQueryTest.java +++ b/src/test/java/com/firebase/geofire/GeoQueryTest.java @@ -1,6 +1,7 @@ package com.firebase.geofire; import com.google.firebase.database.DatabaseError; +import com.google.firebase.database.DatabaseReference; import junit.framework.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -113,7 +114,7 @@ public void keyMoved() throws InterruptedException { public void dataChanged() throws InterruptedException { GeoFire geoFire = newTestGeoFire(); setLoc(geoFire, "0", 0, 0); - setLoc(geoFire, "1", 37.0000, -122.0000); + setLoc(geoFire, "1", 37.0000, -122.0001); setLoc(geoFire, "2", 37.0001, -122.0001); setLoc(geoFire, "3", 37.1000, -122.0000); setLoc(geoFire, "4", 37.0002, -121.9998, true); @@ -126,19 +127,20 @@ public void dataChanged() throws InterruptedException { waitForGeoFireReady(geoFire); - setLoc(geoFire, "0", 1, 1); // outside of query - setLoc(geoFire, "1", 37.0001, -122.0000); // moved - setLoc(geoFire, "2", 37.0001, -122.0001); // location stayed the same - setLoc(geoFire, "4", 37.0002, -122.0000, true); // moved - setValueAndWait(geoFire.getDatabaseRefForKey("2").child("some_child"), "some_value"); // data changed + setLoc(geoFire, "0", 1, 1, true); // outside of query + setLoc(geoFire, "1", 37.0001, -122.0001, true); // moved + setLoc(geoFire, "2", 37.0001, -122.0001, true); // location stayed the same + setLoc(geoFire, "4", 37.0002, -121.9999, true); // moved + DatabaseReference childRef = geoFire.getDatabaseRefForKey("2").child("some_child"); + setValueAndWait(childRef, "some_value"); // data changed List events = new LinkedList<>(); - events.add(GeoQueryDataEventTestListener.dataMoved("1", 37.0001, -122.0000)); - events.add(GeoQueryDataEventTestListener.dataChanged("1", 37.0001, -122.0000)); + events.add(GeoQueryDataEventTestListener.dataMoved("1", 37.0001, -122.0001)); + events.add(GeoQueryDataEventTestListener.dataChanged("1", 37.0001, -122.0001)); - events.add(GeoQueryDataEventTestListener.dataMoved("4", 37.0002, -122.0000)); - events.add(GeoQueryDataEventTestListener.dataChanged("4", 37.0002, -122.0000)); + events.add(GeoQueryDataEventTestListener.dataMoved("4", 37.0002, -121.9999)); + events.add(GeoQueryDataEventTestListener.dataChanged("4", 37.0002, -121.9999)); events.add(GeoQueryDataEventTestListener.dataChanged("2", 37.0001, -122.0001));