Repository: graphhopper/graphhopper (java)

Issue:
Route pedestrians over highway=platform
Please allow pedestrians to be routed over `highway=platform`. (This tag should be treated the same as `highway=footway`.) Currently, they are diverted onto the street, see e.g. [here](https://graphhopper.com/maps/?point=46.94326%2C7.44836&point=46.943963%2C7.448967&locale=en-us&vehicle=foot&weighting=fastest&elevation=true&use_miles=false&layer=Omniscale).

Pull request:
Route pedestrians over highway=platform, define it as a pushing section for bikes. Unit testing considerations.
Fixes #1580 

#### Correct foot routing:
![gh-issue-1580-foot-correct](https://user-images.githubusercontent.com/10464482/57490638-89a49f80-72c2-11e9-8169-020460533e39.png)

#### Correct bike routing where gh prefers taking the road over entering the pushing section:
![gh-issue-1580-bike-correct](https://user-images.githubusercontent.com/10464482/57490669-a7720480-72c2-11e9-98e3-0edb5df23555.png)

#### Correct bike routing where an intermediate node forces gh to enter the pushing section:
![gh-issue-1580-bike-pushing-waypoints-correct](https://user-images.githubusercontent.com/10464482/57493282-eb6a0700-72cc-11e9-8569-b79e50f23104.png)
(Prior to the fix, gh would absolutely refuse to path to waypoint 2)

```
Continue onto Bernastrasse, get off the bike
Keep left, get off the bike
Waypoint 1
Continue, get off the bike
Waypoint 2
Continue onto Helvetiaplatz, get off the bike
Turn left, get off the bike
Arrive at destination
```

---
## The fix

The fix was relatively straightforward, not sure what much to say about it, just adding the correct definitions for highway=platform in foot and bike encoder classes.

Reading the documentation of [Tag:highway=platform](https://wiki.openstreetmap.org/wiki/Tag:highway%3Dplatform) did illustrate that `public_transport=platform` will also probably have to be dealt with at some point.

## Tests

I took the time to look at the test unit structure and think about improvements, since @karussell seemed interested in what I could do on the unit testing side, so I ended up writing the unit tests for fix in a different pattern.

I realized this should probably be in a different pull request altogether only after being three parts done. That's mea culpa. I wouldn't mind rewriting the tests in the current pattern for this PR and opening a separate one if that's needed, or just using the current pattern if the change is unwelcome. Just to be clear, I haven't touched any existing test logic or structure at all, just wrote new tests.

## Unit Testing pattern
I noticed the inheritance hierarchy in the test structure and the lack of parameterized tests and I think I get why. JUnit doesn't really let you parameterize on a per method basis, just on a per class basis and that can be really clumsy. Having to deal with that and the need to share code logic from abstract to concrete can  make things pretty hairy. However, one can make use of JUnit's TestRule and Parameterized to tackle both issues at once.

The two test classes I added are:
- BikeFlagEncoderRule.java
- BikeFlagEncoderDismountTest.java

BikeFlagEncoderRule sort of does what a before or setup annotation would do, except it encapsulates the same data that ``AbstractBikeFlagEncoderTester.java`` holds. It's more flexible in so far as we can use a Rule or ClassRule annotation to construct it once every test method run or once per class.

```java
@ClassRule 
public static BikeFlagEncoderRule encoderRule = new BikeFlagEncoderRule(new BikeFlagEncoder());
```

Now the entire test class can use any data in encoderRule and encoderRule can hold all the data AbstractBikeFlagEncoder used to hold. Note that the usage of abstract is no longer needed because the constructor to the concrete implementation is called in the constructor of BikeFlagEncoderRule. 

This opens the way to replacing the inheritance pattern with a composition pattern. That would allow the test logic in AbstractBikeFlagEncoder to moved into something more akin to a helper class, so the tests can be shared without needing inheritance.

Among other things, this has the significant benefit of making the usage of Parameterized easier. To deal with the difficulty of mixing disparate methods under a regime that requires that test parameterization happen class-wide, I propose using [org.junit.experimental.runners.Enclosed](https://junit.org/junit4/javadoc/latest/org/junit/experimental/runners/Enclosed.html). 

Enclosed allows static nested classes in a test class to be run properly. Why do we want static nested classes? They allow us to constrain Parameterized and have it be scoped only with the methods that need that parameter list. This allows unparameterized tests to just exist at the top level and parameterized tests on the second level, grouped by their parameter list.

Right now, the way ``BikeFlagEncoderDismountTest.java`` is set up, the tests look like this:

![bike-flag-encoder-dismount-test-test-explorer](https://user-images.githubusercontent.com/10464482/57492094-2b7abb00-72c8-11e9-99bf-0f2d309e84d7.png)

**tl;dr if this sounds interesting, I could make a new PR and rewrite:**
- ``AbstractBikeFlagEncoderTester.java``
- ``BikeFlagEncoderTest.java``
- ``MountainBikeFlagEncoderTest.java``
- ``RacingBikeFlagEncoderTest.java`` 

**to use a parameterized test pattern without inheritance.**

Patch:
diff --git a/core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java b/core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java
index 06bc1df0b..dfc9e5d59 100644
--- a/core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java
+++ b/core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java
@@ -167,6 +167,7 @@ abstract public class BikeCommonFlagEncoder extends AbstractFlagEncoder {
         setHighwaySpeed("cycleway", CYCLEWAY_SPEED);
         setHighwaySpeed("path", 10);
         setHighwaySpeed("footway", 6);
+        setHighwaySpeed("platform", 6);
         setHighwaySpeed("pedestrian", 6);
         setHighwaySpeed("track", 12);
         setHighwaySpeed("service", 14);
diff --git a/core/src/main/java/com/graphhopper/routing/util/BikeFlagEncoder.java b/core/src/main/java/com/graphhopper/routing/util/BikeFlagEncoder.java
index 4b6eee72a..9e4ed568b 100644
--- a/core/src/main/java/com/graphhopper/routing/util/BikeFlagEncoder.java
+++ b/core/src/main/java/com/graphhopper/routing/util/BikeFlagEncoder.java
@@ -49,6 +49,7 @@ public class BikeFlagEncoder extends BikeCommonFlagEncoder {
         addPushingSection("footway");
         addPushingSection("pedestrian");
         addPushingSection("steps");
+        addPushingSection("platform");
 
         avoidHighwayTags.add("trunk");
         avoidHighwayTags.add("trunk_link");
diff --git a/core/src/main/java/com/graphhopper/routing/util/FootFlagEncoder.java b/core/src/main/java/com/graphhopper/routing/util/FootFlagEncoder.java
index e41a0d0c6..ccdd70274 100644
--- a/core/src/main/java/com/graphhopper/routing/util/FootFlagEncoder.java
+++ b/core/src/main/java/com/graphhopper/routing/util/FootFlagEncoder.java
@@ -109,6 +109,7 @@ public class FootFlagEncoder extends AbstractFlagEncoder {
         safeHighwayTags.add("track");
         safeHighwayTags.add("residential");
         safeHighwayTags.add("service");
+        safeHighwayTags.add("platform");
 
         avoidHighwayTags.add("trunk");
         avoidHighwayTags.add("trunk_link");
diff --git a/core/src/main/java/com/graphhopper/routing/util/MountainBikeFlagEncoder.java b/core/src/main/java/com/graphhopper/routing/util/MountainBikeFlagEncoder.java
index f0e7965cb..b6f18a451 100644
--- a/core/src/main/java/com/graphhopper/routing/util/MountainBikeFlagEncoder.java
+++ b/core/src/main/java/com/graphhopper/routing/util/MountainBikeFlagEncoder.java
@@ -110,6 +110,7 @@ public class MountainBikeFlagEncoder extends BikeCommonFlagEncoder {
         setHighwaySpeed("tertiary_link", 18);
 
         addPushingSection("footway");
+        addPushingSection("platform");
         addPushingSection("pedestrian");
         addPushingSection("steps");
 
diff --git a/core/src/main/java/com/graphhopper/routing/util/RacingBikeFlagEncoder.java b/core/src/main/java/com/graphhopper/routing/util/RacingBikeFlagEncoder.java
index c2461c75c..b93e50fbd 100644
--- a/core/src/main/java/com/graphhopper/routing/util/RacingBikeFlagEncoder.java
+++ b/core/src/main/java/com/graphhopper/routing/util/RacingBikeFlagEncoder.java
@@ -113,6 +113,7 @@ public class RacingBikeFlagEncoder extends BikeCommonFlagEncoder {
 
         addPushingSection("path");
         addPushingSection("footway");
+        addPushingSection("platform");
         addPushingSection("pedestrian");
         addPushingSection("steps");
 


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/util/AbstractBikeFlagEncoderTester.java b/core/src/test/java/com/graphhopper/routing/util/AbstractBikeFlagEncoderTester.java
index ee1fae741..d112f0364 100644
--- a/core/src/test/java/com/graphhopper/routing/util/AbstractBikeFlagEncoderTester.java
+++ b/core/src/test/java/com/graphhopper/routing/util/AbstractBikeFlagEncoderTester.java
@@ -179,6 +179,20 @@ public abstract class AbstractBikeFlagEncoderTester {
         way.setTag("railway", "abandoned");
         assertTrue(encoder.getAccess(way).isWay());
 
+        way.clearTags();
+        way.setTag("highway", "platform");
+        assertTrue(encoder.getAccess(way).isWay());
+
+        way.clearTags();
+        way.setTag("highway", "platform");
+        way.setTag("bicycle", "dismount");
+        assertTrue(encoder.getAccess(way).isWay());
+
+        way.clearTags();
+        way.setTag("highway", "platform");
+        way.setTag("bicycle", "no");
+        assertTrue(encoder.getAccess(way).canSkip());
+
         DateFormat simpleDateFormat = Helper.createFormatter("yyyy MMM dd");
 
         way.clearTags();
@@ -354,6 +368,16 @@ public abstract class AbstractBikeFlagEncoderTester {
         wayType = getWayTypeFromFlags(way);
         assertEquals("get off the bike", wayType);
 
+        way.clearTags();
+        way.setTag("highway", "platform");
+        wayType = getWayTypeFromFlags(way);
+        assertEquals("get off the bike", wayType);
+
+        way.clearTags();
+        way.setTag("highway", "platform");
+        way.setTag("bicycle", "yes");
+        wayType = getWayTypeFromFlags(way);
+        assertEquals("", wayType);
     }
 
     @Test
diff --git a/core/src/test/java/com/graphhopper/routing/util/BikeFlagEncoderTest.java b/core/src/test/java/com/graphhopper/routing/util/BikeFlagEncoderTest.java
index 071c49626..7ade7832f 100644
--- a/core/src/test/java/com/graphhopper/routing/util/BikeFlagEncoderTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/BikeFlagEncoderTest.java
@@ -93,6 +93,16 @@ public class BikeFlagEncoderTest extends AbstractBikeFlagEncoderTester {
         assertEquals(PUSHING_SECTION_SPEED * 2, encoder.getSpeed(way));
         assertPriority(PREFER.getValue(), way);
 
+        way.clearTags();
+        way.setTag("highway", "platform");
+        way.setTag("surface", "paved");
+        way.setTag("bicycle", "yes");
+        assertEquals(PUSHING_SECTION_SPEED, encoder.getSpeed(way));
+        assertPriority(PREFER.getValue(), way);
+        way.setTag("segregated", "yes");
+        assertEquals(PUSHING_SECTION_SPEED * 2, encoder.getSpeed(way));
+        assertPriority(PREFER.getValue(), way);
+
         way.clearTags();
         way.setTag("highway", "cycleway");
         assertEquals(18, encoder.getSpeed(way));
@@ -161,6 +171,12 @@ public class BikeFlagEncoderTest extends AbstractBikeFlagEncoderTester {
         assertEquals(PUSHING_SECTION_SPEED, encoder.getSpeed(way));
         assertPriority(AVOID_IF_POSSIBLE.getValue(), way);
 
+        way.clearTags();
+        way.setTag("highway", "platform");
+        way.setTag("surface", "paved");
+        assertEquals(PUSHING_SECTION_SPEED, encoder.getSpeed(way));
+        assertPriority(AVOID_IF_POSSIBLE.getValue(), way);
+
         way.clearTags();
         way.setTag("highway", "footway");
         way.setTag("surface", "paved");
@@ -168,6 +184,13 @@ public class BikeFlagEncoderTest extends AbstractBikeFlagEncoderTester {
         assertEquals(cyclewaySpeed, encoder.getSpeed(way));
         assertPriority(VERY_NICE.getValue(), way);
 
+        way.clearTags();
+        way.setTag("highway", "platform");
+        way.setTag("surface", "paved");
+        way.setTag("bicycle", "designated");
+        assertEquals(cyclewaySpeed, encoder.getSpeed(way));
+        assertPriority(VERY_NICE.getValue(), way);
+
         way.clearTags();
         way.setTag("highway", "track");
         assertEquals(12, encoder.getSpeed(way));
@@ -469,6 +492,25 @@ public class BikeFlagEncoderTest extends AbstractBikeFlagEncoderTester {
         relFlags = encoder.handleRelationTags(0, osmRel);
         wayType = getWayTypeFromFlags(osmWay, relFlags);
         assertEquals("get off the bike", wayType);
+
+        // Test for highway=platform.
+        osmRel.clearTags();
+        osmWay.clearTags();
+        osmWay.setTag("highway", "platform");
+
+        // First tests without a cycle route relation, this is a get off the bike
+        relFlags = encoder.handleRelationTags(0, osmRel);
+        wayType = getWayTypeFromFlags(osmWay, relFlags);
+        assertEquals("get off the bike", wayType);
+
+        // now as part of a cycle route relation
+        osmRel.setTag("type", "route");
+        osmRel.setTag("route", "bicycle");
+        osmRel.setTag("network", "lcn");
+        relFlags = encoder.handleRelationTags(0, osmRel);
+        wayType = getWayTypeFromFlags(osmWay, relFlags);
+        assertEquals("", wayType);
+
     }
 
     @Test
diff --git a/core/src/test/java/com/graphhopper/routing/util/FootFlagEncoderTest.java b/core/src/test/java/com/graphhopper/routing/util/FootFlagEncoderTest.java
index 91f18ba95..1b74cbaa7 100644
--- a/core/src/test/java/com/graphhopper/routing/util/FootFlagEncoderTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/FootFlagEncoderTest.java
@@ -130,6 +130,9 @@ public class FootFlagEncoderTest {
         way.setTag("highway", "footway");
         assertTrue(footEncoder.getAccess(way).isWay());
 
+        way.setTag("highway", "platform");
+        assertTrue(footEncoder.getAccess(way).isWay());
+
         way.setTag("highway", "motorway");
         assertTrue(footEncoder.getAccess(way).canSkip());