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

Conversation

chrstnbwnkl
Copy link
Contributor

I left the mocked request tests for now, because a) we still need them for non-FOSS routers and b) they do help with testing the parsing and request building. For now, the e2e tests are also just pretty basic, we can add more sophisticated ones as time moves on, what do you think @nilsnolde ?

Copy link
Owner

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

Thanks, that's a good start!

I'd just rename e2e to integration tests and reword the README stuff a bit.

@@ -42,8 +42,10 @@ jobs:

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

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.

CHANGELOG.md Outdated
@@ -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
Owner

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.

CONTRIBUTING.md Outdated
@@ -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
Owner

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?

CONTRIBUTING.md Outdated
Comment on lines 68 to 69
~~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
Owner

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.

@@ -510,7 +510,7 @@ def isochrones(
type,
intervals[0],
buckets,
center,
locations,
Copy link
Owner

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
Contributor 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:

https://github.com/gis-ops/routingpy/blob/73bf55cdcb8871c1c6b9886bc14a5ca8aef9a38f/routingpy/routers/graphhopper.py#L490

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
Contributor 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
Owner

@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.

@@ -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
Owner

@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

@nilsnolde
Copy link
Owner

Also, seems like the PBF is not pushed up.

@chrstnbwnkl
Copy link
Contributor Author

Adressed most comments now except the ones pertaining to the isochrone locations parameter, this should probably be adressed in a separate PR?

Copy link
Owner

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

sorry for the long wait!

@nilsnolde
Copy link
Owner

hm, some tests failing.. will need to wait a bit.

Comment on lines 3 to 19
ors:
container_name: ors-tests
ports:
- "8005: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
Copy link
Owner

Choose a reason for hiding this comment

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

don't we also have to uncomment this?

@nilsnolde nilsnolde self-requested a review January 30, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants