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

Add router tests #55

Closed
nilsnolde opened this issue Jul 13, 2022 · 4 comments
Closed

Add router tests #55

nilsnolde opened this issue Jul 13, 2022 · 4 comments

Comments

@nilsnolde
Copy link
Collaborator

We’re resting already, which is great. But we only test the self contained example instances. It’d be worthwhile to test the integration with all routers with Andorra or so, so we get notified ahead of time when smth breaks. Best in a scheduled GitHub Action, e.g. every 2 weeks as there’s hardly any activity here unless we’re releasing.

@nilsnolde nilsnolde added the help wanted Extra attention is needed label Jul 13, 2022
@jcoupey
Copy link
Contributor

jcoupey commented Jul 14, 2022

there’s hardly any activity here unless we’re releasing

For that exact reason, I would not bother to test frequently. I mean if we only run tests on release PR, we'd still catch any problem before releasing.

@nilsnolde
Copy link
Collaborator Author

this would just test integration with the current versions of routers, not any code of this repo or vroom. we offer a docker-compose here which I kind of take responsibility for, since it's inside the repo. any of the routers could change their latest image at any point (that's kinda the same as depending on master branch instead of a release), breaking stuff in this repo's docker-compose.yml. so it'd be regular integration tests basically. few alternatives exist of course:

  1. remove the docker-compose.yml, which I'm pretty open to tbh. it does make startup so much easier and faster for new users, but it also attracts lots of people who have zero clue about software or docker (see recent issue..). people who can handle docker will only spend extra 20 mins setting up a docker-compose.yml. in this case I wouldn't feel like I'd need to test integrations
  2. pin the images we reference in the docker-compose.yml to a specific version of the routers (instead of latest) and let the community keep it up-to-date. in this case I would feel like we should test integation. it would be nice to have one place that tests valhalla & ors no? vroom tests osrm extensively, but not the others.

option 1 sounds tempting to me personally though :) what's your thoughts @jcoupey ?

@jcoupey
Copy link
Contributor

jcoupey commented Jul 14, 2022

I agree we should go for option 1 here since:

  • the scope for this repo is to provide a Docker setup for VROOM, not a full Docker infra working out of the box with any router;
  • the scope of this organization is to develop an optimization engine, so of course routing is related but users should be able to handle their own routing in any case (be it from source, using Docker or whatever).

Taking responsibility for maintaining working integration for all routers is just asking for trouble for a small benefit, especially if docker-composing is straightforward as you point out. And it's always possible to deal with the occasional questions in the issues.

We should probably adjust the current readme if removing docker-compose.yml?

@nilsnolde
Copy link
Collaborator Author

yeah, it's a thin line for these kind of wrapper projects..

We should probably adjust the current readme if removing docker-compose.yml

jep, I'll draft smth soon.

@nilsnolde nilsnolde removed the help wanted Extra attention is needed label Jul 14, 2022
@nilsnolde nilsnolde closed this as not planned Won't fix, can't repro, duplicate, stale Jul 14, 2022
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

No branches or pull requests

2 participants