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

added e2e tests, some minor response parsing fixes #85

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ jobs:

- name: pytest and coverage
run: |
docker-compose -f docker-compose.yml up -d
Copy link
Member

Choose a reason for hiding this comment

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

Let's put the docker-compose.yml into the test folder. When it's in root people assume it somehow belongs to the project, which it doesn't, it's only a means to test the integration.

pip install -e .
coverage run --source=routingpy --module pytest
coverage lcov --include "routingpy/*"
docker-compose -f docker-compose.yml down
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,5 @@ test.py
*.pyc
*.idea/
.DS_Store
test_data
!test_data/andorra-latest.osm.pbf
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## **Unreleased**

### Added
- Added e2e tests using dockerized instances of Valhalla, OSRM, ORS and GraphHopper
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't call it "e2e" tests. While you might think of them like that, it's also what it was before, just with mocks. If you want to call it smth, you can say e.g. "integration" tests, which is more differentiating to previously.


### Fixed
- Unit conversion did not work properly in ors' directions method
- Docstring for `intersections` parameter in ors' isochrones method was missing
- `profile` parameter was unnecessarily passed to POST params in ors' isochrones and matrix methods
- Valhalla's Isochrones center property was unnecessarily wrapped in a list
- GraphHopper Isochrones center coordinates were strings, not floats


## [v1.1.0](https://pypi.org/project/routingpy/1.1.0/)

Expand Down
10 changes: 6 additions & 4 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ poetry install
```

3. Run tests to check if all goes well:

(In order to run the e2e tests, run the router instances with `docker-compose up` from the project root)
Copy link
Member

Choose a reason for hiding this comment

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

Let's add the command to the code snippet below, no?


```bash
# From the root of your git project
nosetests -v
Expand All @@ -62,10 +65,9 @@ pre-commit install

### Tests

We only do mocked tests as routing results heavily depend on underlying data, which, at least in the case of the FOSS routing
engines, changes on a very regular basis due to OpenStreetMap updates. All queries and most mocked responses are located in
`test/test_helper.py` in `dict`s. This unfortunately also means, that our tests are less API tests and more unit tests and can't catch
updates on providers' API changes.
~~We only do mocked tests as routing results heavily depend on underlying data, which, at least in the case of the FOSS routing
engines, changes on a very regular basis due to OpenStreetMap updates.~~ We also do end-to-end testing now! Run `docker-compose up` from
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove the old version and We also do end-to-end testing now. In my experience that doesn't age well.

the project root to set up local instances of Valhalla, OSRM, ORS and GraphHopper!

Run `nosetests` with a `coverage` flag, which shouldn't report < 90% coverage overall (the starting point at the
beginning of the project). A [`coveralls.io`](https://coveralls.io) instance will check for coverage when you submit a PR.
Expand Down
35 changes: 35 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
version: "3.8"
services:
ors:
container_name: ors-tests
ports:
- "8080:8080"
image: ghcr.io/gis-ops/openrouteservice:latest
volumes:
- ./test_data/andorra-latest.osm.pbf:/ors-core/data/osm_file.pbf
environment:
- "JAVA_OPTS=-Djava.awt.headless=true -server -XX:TargetSurvivorRatio=75 -XX:SurvivorRatio=64 -XX:MaxTenuringThreshold=3 -XX:+UseG1GC -XX:+ScavengeBeforeFullGC -XX:ParallelGCThreads=4 -Xms1g -Xmx2g"
- "CATALINA_OPTS=-Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.port=9001 -Dcom.sun.management.jmxremote.rmi.port=9001 -Dcom.sun.management.jmxremote.authenticate=false -Dcom.sun.management.jmxremote.ssl=false -Djava.rmi.server.hostname=localhost"
valhalla:
container_name: valhalla-tests
image: gisops/valhalla:latest
ports:
- "8002:8002"
volumes:
- ./test_data/:/custom_files
osrm:
container_name: osrm-tests
image: osrm/osrm-backend:latest
ports:
- "5000:5000"
volumes:
- ./test_data/:/data
command: '/bin/bash -c "osrm-extract -p /opt/car.lua /data/andorra-latest.osm.pbf && osrm-partition /data/andorra-latest.osrm && osrm-customize /data/andorra-latest.osrm && osrm-routed --algorithm=MLD /data/andorra-latest.osrm"'
graphhopper:
container_name: graphhopper-tests
image: israelhikingmap/graphhopper:latest
ports:
- "8989:8989"
volumes:
- ./test_data/:/graphhopper/data
command: "-i data/andorra-latest.osm.pbf --host 0.0.0.0"
2 changes: 1 addition & 1 deletion routingpy/routers/graphhopper.py
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ def isochrones(
type,
intervals[0],
buckets,
center,
locations,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand this (and was wrong before too, but don't think it's actually solved now): locations is an array right? If you look at parse_isochrone_json, it'll be put as center for an individual isochrone. So down there it's missing an indexing into the center array.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's a couple of things here: the parameter is locations when really, it's expecting one location as [lon, lat] so it already represents a single location. On top of that, center is really just that location but the coordinates are represented as strings:

center = [convert.format_float(f) for f in locations]

The real problem seems to be the confusion about the name of the locations parameter: Valhalla, ORS, GraphHopper and HERE all only accept one single location, yet that parameter is consistently named in the plural form. We could either consistently change them to location or we accept multiple and fire multiple requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

What adds even more to the confusion is that Valhalla takes one location but nested into an array, but GraphHopper and ORS take one location (unnested).

Copy link
Member

@nilsnolde nilsnolde Dec 6, 2022

Choose a reason for hiding this comment

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

Valhalla, ORS, GraphHopper and HERE all only accept one single location

Almost: Valhalla & ORS do accept multiple locations, but only ORS is doing what the code assumes, Valhalla does some weird union stuff with the multiple locations. Since we just came from ORS when we started routingpy, we tried to support that. In the end, I agree, let's just get rid of supporting multiple locations and accept only a single location for isochrones. Major version sort of thing, but no problem to push one, I don't really care tbh.

interval_type,
)

Expand Down
2 changes: 1 addition & 1 deletion routingpy/routers/valhalla.py
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ def parse_isochrone_json(response, intervals, locations, interval_type):
Isochrone(
geometry=feature["geometry"]["coordinates"],
interval=intervals[idx],
center=locations,
center=locations[0],
Copy link
Member

@nilsnolde nilsnolde Dec 6, 2022

Choose a reason for hiding this comment

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

this is fine for now. but actually, the way we do it here is not really possible with valhalla. it's never returning more than one isochrone, even with 5 input locations. basically it does a geometric union of the isochrones from multiple locations, so we'll always have only one Isochrone in Isochrones. which makes the center rather meaningless. but again, this is surely better than it was before.

for valhalla we would need to do a bit more to support actual center coords. we return them conditionally with show_locations. in fact we could remove show_locations from the input, always append it for isochrones and parse the MultiPoint snapped s

interval_type=interval_type,
)
)
Expand Down
145 changes: 145 additions & 0 deletions tests/test_e2e.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
import tests as _test
from routingpy import ORS, OSRM, Graphhopper, Valhalla


class TestRoutingpyE2e(_test.TestCase):
@classmethod
def setUpClass(cls) -> None:
cls.valhalla = Valhalla("http://localhost:8002")
cls.osrm = OSRM("http://localhost:5000")
cls.ors = ORS(base_url="http://localhost:8080/ors")
cls.gh = Graphhopper(base_url="http://localhost:8989")

def test_valhalla_directions(self):

directions = self.valhalla.directions(
[
[1.51886, 42.5063],
[1.53789, 42.51007],
],
"auto",
)

self.assertIsInstance(directions.geometry, list)
self.assertIsInstance(directions.duration, int)
self.assertIsInstance(directions.distance, int)

def test_valhalla_isochrones(self):

isochrones = self.valhalla.isochrones(
[
[1.51886, 42.5063],
],
"auto",
[20, 50],
)

self.assertIsInstance(isochrones[1].geometry, list)
self.assertEqual(isochrones[0].interval, 20)
self.assertEqual(isochrones[1].interval, 50)
self.assertEqual(isochrones[1].center, [1.51886, 42.5063])

def test_valhalla_matrix(self):
matrix = self.valhalla.matrix(
[
[1.51886, 42.5063],
[1.53789, 42.51007],
[1.53489, 42.52007],
[1.53189, 42.51607],
],
"auto",
)

self.assertEqual(len(matrix.distances), 4)
self.assertEqual(len(matrix.durations), 4)
self.assertEqual(len(matrix.durations[0]), 4)
self.assertEqual(len(matrix.distances[0]), 4)

def test_osrm_directions(self):
directions = self.osrm.directions(
[
[1.51886, 42.5063],
[1.53789, 42.51007],
]
)

self.assertIsInstance(directions.geometry, list)
self.assertIsInstance(directions.duration, int)
self.assertIsInstance(directions.distance, int)

def test_osrm_matrix(self):
matrix = self.osrm.matrix(
[
[1.51886, 42.5063],
[1.53789, 42.51007],
[1.53489, 42.52007],
[1.53189, 42.51607],
],
"auto",
)

self.assertEqual(len(matrix.distances), 4)
self.assertEqual(len(matrix.durations), 4)
self.assertEqual(len(matrix.durations[0]), 4)
self.assertEqual(len(matrix.distances[0]), 4)

def test_ors_directions(self):
directions = self.ors.directions(
[
[1.51886, 42.5063],
[1.53789, 42.51007],
],
"driving-car",
)

self.assertIsInstance(directions.geometry, list)
self.assertIsInstance(directions.duration, int)
self.assertIsInstance(directions.distance, int)

def test_ors_isochrones(self):
isochrones = self.ors.isochrones([1.51886, 42.5063], "driving-car", [20, 50])

self.assertIsInstance(isochrones[1].geometry, list)
self.assertEqual(isochrones[0].interval, 20)
self.assertEqual(isochrones[1].interval, 50)
self.assertAlmostEqual(isochrones[1].center[0], 1.51886, 1)
self.assertAlmostEqual(isochrones[1].center[1], 42.5063, 1)

def test_ors_matrix(self):
matrix = self.ors.matrix(
[
[1.51886, 42.5063],
[1.53789, 42.51007],
[1.53489, 42.52007],
[1.53189, 42.51607],
],
"driving-car",
metrics=["distance", "duration"],
)

self.assertEqual(len(matrix.distances), 4)
self.assertEqual(len(matrix.durations), 4)
self.assertEqual(len(matrix.durations[0]), 4)
self.assertEqual(len(matrix.distances[0]), 4)

def test_graphhopper_directions(self):
directions = self.gh.directions(
[
[1.51886, 42.5063],
[1.53789, 42.51007],
],
"car",
)

self.assertIsInstance(directions.geometry, list)
self.assertIsInstance(directions.duration, int)
self.assertIsInstance(directions.distance, int)

def test_graphhopper_isochrones(self):
isochrones = self.gh.isochrones([1.51886, 42.5063], "car", [50])

self.assertIsInstance(isochrones[0].geometry, list)
self.assertEqual(isochrones[0].interval, 50)
print(isochrones[0].center)
self.assertAlmostEqual(isochrones[0].center[0], 1.51886, 1)
self.assertAlmostEqual(isochrones[0].center[1], 42.5063, 1)