Increase SDK test coverage #8261

Guardiola31337 · 2017-03-02T22:47:43Z

There are a bunch of classes not already tested in the SDK module.

tobrun · 2017-03-03T16:09:07Z

With this PR we are moving the tests to the SDK module, this change needs to be reflected in the Makefile for the different unit test tasks we expose there.

tobrun

Changes are looking great overal, feel free to tackle method implementations themselves as well. one thing we were thinking about is removing the List by separating them into different collections, long term we want to simplify a lot of this code using the peer model in #6912.

tobrun · 2017-03-14T11:39:05Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java

-
-  static {
+  @Override
+  public void loadNativeLibrary() {


Don't think this is a valid change, loading the native library is a static operation. cc @ivovandongen

I would prefer to use Interfaces in any case for testability. Instead of depending on classes directly.

For the library loading, I would like to centralise it, regardless of testability. For example, create a class LibraryLoader, with a single static method load() which can be called from the classes containing methods from a static initialisation block still. This would centralise the knowledge about the library name at least. And also, you could mock this method with PowerMock.

@ivovandongen I like the idea to create a class LibraryLoader to centralise all related stuff in the same class (as you mentioned) but I don't understand the necessity of a static block and a static method. Do they have to be static for any particular reason that I don't know yet? cc/ @tobrun

ivovandongen

The devision of responsibilities is a good idea. However, the decomposition is not entirely clear. This might mostly be naming, but the devision between concepts as AnnotationManager, Annotations and then an implementation called AnnotationsFunctions seems a little arbitrary. It seems like Annotations is meant to manage the collection of annotations, but thats also the responsibility of AnnotationManager.

ivovandongen · 2017-03-31T12:49:47Z

...rm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java

  private final List<Marker> selectedMarkers = new ArrayList<>();

  private MapboxMap mapboxMap;
  private MapboxMap.OnMarkerClickListener onMarkerClickListener;
-
-  AnnotationManager(NativeMapView view, MapView mapView, MarkerViewManager markerViewManager) {
+  private Annotations annotationsManager;


This is a weird naming, an Annotations called annotationsManager inside AnnotationManager

ivovandongen · 2017-03-31T12:51:58Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Annotations.java

+
+import java.util.List;
+
+interface Annotations {


Again, naming here. Can we give this a somewhat more descriptive name? Also, how is this different from the responsibilities of AnnotationManager itself?

JavaDoc on interfaces is greatly appreciated

ivovandongen · 2017-03-31T12:52:51Z

...android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationsFunctions.java

+  private final NativeMapView nativeMapView;
+  private final LongSparseArray<Annotation> annotations;
+
+  AnnotationsFunctions(NativeMapView nativeMapView, LongSparseArray<Annotation> annotations) {


Naming; AnnotationFunctions?

ivovandongen · 2017-03-31T12:53:29Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/LibraryLoader.java

+
+
+class LibraryLoader {
+  static void load() {


NIT; empty lines / a hint of JavaDoc

ivovandongen · 2017-03-31T12:55:39Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Markers.java

+
+import java.util.List;
+
+interface Markers {


JavaDoc on interfaces is greatly appreciated

Naming here, again sorry, is not ideal. My main question again is how this differs from related classes

ivovandongen · 2017-03-31T13:00:17Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java

  static {
-    System.loadLibrary("mapbox-gl");
+    LibraryLoader.load();


Could you replace all occurrences of System.loadLibrary("mapbox-gl");:

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/net/NativeConnectivityListener.java

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineRegion.java

ivovandongen · 2017-03-31T13:01:04Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Polygons.java

+
+import java.util.List;
+
+interface Polygons {


JavaDoc + Naming

ivovandongen · 2017-03-31T13:01:35Z

...orm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MarkersFunctions.java

+  private final IconManager iconManager;
+  private final MarkerViewManager markerViewManager;
+
+  MarkersFunctions(NativeMapView nativeMapView, MapView mapView, LongSparseArray<Annotation> annotations, IconManager


JavaDoc + Naming

In this case the plurality of Markers is also a bit weird here I think.

ivovandongen · 2017-03-31T13:01:58Z

...rm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/PolygonsFunctions.java

+import java.util.ArrayList;
+import java.util.List;
+
+class PolygonsFunctions implements Polygons {


JavaDoc + Naming (incl plurality)

ivovandongen · 2017-03-31T13:02:07Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Polylines.java

+
+import java.util.List;
+
+interface Polylines {


JavaDoc + Naming

tobrun · 2017-04-20T07:59:06Z

I would like to get this PR moving forward. The refactored code + having a way to unit test library loading classes are valuable additions to the next release. Though there is consensus on having unit testable native library loading classes, there is some disagreement on the way it's currently setup.
I feel the best way forward is to merge current approach and ticketing out a follow up ticket with a focus on having more local and isolated setup.

Before we merge @Guardiola31337, can we address the naming comments? Most of these were fixed with using the -able suffix but the plural names remained untouched. AFAIK plural names are reserved for Collection (sub)classes with the exception of utility classes.

Guardiola31337 · 2017-05-11T07:49:28Z

@tobrun I fixed suggested comments 👍
Could you give it a second look?

…annotation manager tests (add marker and add markers)

Guardiola31337 added Android Mapbox Maps SDK for Android tests ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 2, 2017

Guardiola31337 requested review from zugaldia and tobrun March 2, 2017 22:47

Guardiola31337 force-pushed the move_unit_tests_into_sdk branch from 306ac5a to d988a7d Compare March 2, 2017 22:54

zugaldia approved these changes Mar 3, 2017

View reviewed changes

Guardiola31337 force-pushed the move_unit_tests_into_sdk branch from bf18178 to 6f30973 Compare March 6, 2017 21:14

tobrun suggested changes Mar 14, 2017

View reviewed changes

Guardiola31337 force-pushed the move_unit_tests_into_sdk branch 3 times, most recently from 3634de4 to 2549f23 Compare March 31, 2017 11:51

ivovandongen suggested changes Mar 31, 2017

View reviewed changes

Guardiola31337 force-pushed the move_unit_tests_into_sdk branch from 0b3003d to ab0dd00 Compare April 11, 2017 06:38

Guardiola31337 added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Apr 11, 2017

Guardiola31337 changed the title ~~[WIP] Increase SDK test coverage~~ Increase SDK test coverage Apr 11, 2017

Guardiola31337 force-pushed the move_unit_tests_into_sdk branch 3 times, most recently from 8bd2166 to 482b84e Compare April 13, 2017 17:11

Guardiola31337 force-pushed the move_unit_tests_into_sdk branch 2 times, most recently from e45f800 to 2eaa757 Compare May 10, 2017 18:32

tobrun approved these changes May 11, 2017

View reviewed changes

[android] refactor move unit tests from test app to sdk and add some …

75b78ab

…annotation manager tests (add marker and add markers)

Guardiola31337 force-pushed the move_unit_tests_into_sdk branch from 2eaa757 to 75b78ab Compare May 11, 2017 09:03

ivovandongen approved these changes May 11, 2017

View reviewed changes

Guardiola31337 merged commit 5290d7e into master May 11, 2017

Guardiola31337 deleted the move_unit_tests_into_sdk branch May 11, 2017 10:06

This was referenced May 15, 2017

MarkerView deselect events with OnMarkerViewClickListener #8996

Merged

[android] - don't leak selected markers when removing #9047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase SDK test coverage #8261

Increase SDK test coverage #8261

Guardiola31337 commented Mar 2, 2017

tobrun commented Mar 3, 2017

tobrun left a comment

tobrun Mar 14, 2017

ivovandongen Mar 14, 2017

Guardiola31337 Mar 14, 2017

ivovandongen left a comment

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

ivovandongen Mar 31, 2017

tobrun commented Apr 20, 2017 •

edited

Loading

Guardiola31337 commented May 11, 2017


		import java.util.List;

		interface Annotations {


		import java.util.List;

		interface Markers {


		import java.util.List;

		interface Polygons {


		import java.util.List;

		interface Polylines {

Increase SDK test coverage #8261

Increase SDK test coverage #8261

Conversation

Guardiola31337 commented Mar 2, 2017

tobrun commented Mar 3, 2017

tobrun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivovandongen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobrun commented Apr 20, 2017 • edited Loading

Guardiola31337 commented May 11, 2017

tobrun commented Apr 20, 2017 •

edited

Loading