added e2e tests, some minor response parsing fixes #85

chrstnbwnkl · 2022-12-05T16:16:29Z

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 ?

…nto cb-test-environment

nilsnolde

Thanks, that's a good start!

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

nilsnolde · 2022-12-06T09:48:52Z

.github/workflows/ci-tests.yml

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

    - name: pytest and coverage
      run: |
+        docker-compose -f docker-compose.yml up -d


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.

nilsnolde · 2022-12-06T09:51:00Z

CHANGELOG.md

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


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.

nilsnolde · 2022-12-06T09:51:57Z

CONTRIBUTING.md

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


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

nilsnolde · 2022-12-06T09:53:44Z

CONTRIBUTING.md

+~~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


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

nilsnolde · 2022-12-06T09:58:19Z

routingpy/routers/graphhopper.py

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


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.

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.

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

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.

nilsnolde · 2022-12-06T10:14:02Z

routingpy/routers/valhalla.py

@@ -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],


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 · 2022-12-06T10:35:31Z

Also, seems like the PBF is not pushed up.

chrstnbwnkl · 2022-12-06T20:26:28Z

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

nilsnolde

sorry for the long wait!

nilsnolde · 2023-01-16T19:31:57Z

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

…ons for now

…nto cb-test-environment

nilsnolde · 2023-01-30T10:07:10Z

tests/docker-compose.yml

+    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


don't we also have to uncomment this?

chrstnbwnkl added 2 commits December 5, 2022 17:05

added e2e tests, some minor response parsing fixes

58b708a

changelog, readme's

317e64f

chrstnbwnkl requested a review from nilsnolde December 5, 2022 16:16

chrstnbwnkl and others added 3 commits December 5, 2022 17:17

Merge branch 'master' into cb-test-environment

3baad5c

ci: add docker compose cmd to test job

0e10090

Merge branch 'cb-test-environment' of github.com:gis-ops/routing-py i…

73bf55c

…nto cb-test-environment

nilsnolde requested changes Dec 6, 2022

View reviewed changes

nilsnolde mentioned this pull request Dec 6, 2022

Only support isochrone requests for single location #86

Open

chrstnbwnkl added 2 commits December 6, 2022 21:13

test: andorra osm file was ignored by git

df30e1b

test: review comments

75ed8e4

chrstnbwnkl requested a review from nilsnolde December 6, 2022 20:26

nilsnolde mentioned this pull request Dec 7, 2022

add alternates to valhalla #87

Open

last bits of the review

f22759d

nilsnolde approved these changes Jan 16, 2023

View reviewed changes

chrstnbwnkl added 10 commits January 26, 2023 17:37

test was failing, we should accept both lists of locations and locati…

66ff7f0

…ons for now

Merge branch 'cb-test-environment' of github.com:gis-ops/routing-py i…

da0c032

…nto cb-test-environment

graphhopper changed their vehicle parameter name to profile

3e21ba5

try different port

25561fb

try skipping only ors directions

00fecc5

debug with curl

84ba3fe

now pre-commit is failing

8ccebbc

Merge branch 'master' into cb-test-environment

e7108c5

debug with curl

05e9876

ignore ORS integration tests for now

514ad8e

nilsnolde requested changes Jan 30, 2023

View reviewed changes

forgot to comment out ORS service in docker-compose

764b20e

nilsnolde self-requested a review January 30, 2023 14:16

chrstnbwnkl mentioned this pull request Apr 3, 2023

fix: object creation only once nilsnolde/routingjs#15

Merged

nilsnolde mentioned this pull request Aug 2, 2023

Get more OTP itinerary info + fix wrong coords order #113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added e2e tests, some minor response parsing fixes #85

added e2e tests, some minor response parsing fixes #85

chrstnbwnkl commented Dec 5, 2022

nilsnolde left a comment

nilsnolde Dec 6, 2022

nilsnolde Dec 6, 2022

nilsnolde Dec 6, 2022

nilsnolde Dec 6, 2022

nilsnolde Dec 6, 2022

chrstnbwnkl Dec 6, 2022

chrstnbwnkl Dec 6, 2022

nilsnolde Dec 6, 2022 •

edited

Loading

nilsnolde Dec 6, 2022 •

edited

Loading

nilsnolde commented Dec 6, 2022

chrstnbwnkl commented Dec 6, 2022

nilsnolde left a comment

nilsnolde commented Jan 16, 2023

nilsnolde Jan 30, 2023

		~~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

added e2e tests, some minor response parsing fixes #85

Are you sure you want to change the base?

added e2e tests, some minor response parsing fixes #85

Conversation

chrstnbwnkl commented Dec 5, 2022

nilsnolde 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

nilsnolde Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

nilsnolde Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

nilsnolde commented Dec 6, 2022

chrstnbwnkl commented Dec 6, 2022

nilsnolde left a comment

Choose a reason for hiding this comment

nilsnolde commented Jan 16, 2023

Choose a reason for hiding this comment

nilsnolde Dec 6, 2022 •

edited

Loading

nilsnolde Dec 6, 2022 •

edited

Loading