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

Integration tests for Geofabrik Overpass API #959

Merged
merged 21 commits into from
Jun 25, 2024

Conversation

nataliejschultz
Copy link
Contributor

Addressing e-mission/e-mission-docs#1036

As of now, there are three tests in the TestOverpass.py module:

  1. test_overpass, which compares a call to the free version of overpass with a call to the geofabrik version
  2. test_get_stops_near, which passes a set of coordinates (as a list inside a dictionary) to get_stops_near in match_stops.py.
  3. test_get_predicted_transit_mode, which passes two sets of coordinates to get_predicted_transit_mode in match_stops.py.

Adding TestOverpass.py
Workflow file runs the shell script.

Shell script sets up emission environment and runs TestOverpass.py.
Adding commands to workflow to get a sense of why it's not running.
Running with the correct path this time (I think)
Adding runner permissions for setup script.
Didn't set correct path to file to add permissions.
@nataliejschultz
Copy link
Contributor Author

Currently, tests 2 and 3 only call the free version of the API to make calls. There are a few ways that come to mind to change this:

  1. Add hard-coded query to geofabrik as seen in test 1 and compare with the free call (not preferred)
  2. Add a similar block of code to match_stops.py as we did for nominatim.py using environment variables:
try:
    NOMINATIM_QUERY_URL = os.environ.get("NOMINATIM_QUERY_URL")
    logging.info(f"NOMINATIM_QUERY_URL: {NOMINATIM_QUERY_URL}")
    print("Nominatim Query URL Configured:", NOMINATIM_QUERY_URL)

    if NOMINATIM_QUERY_URL is None:
        raise Exception("Nominatim query url not configured")
except:
    print("Nominatim URL not configured, place decoding must happen on the client")
  1. Add a file called overpass_server.json with the geofabrik query URL and work with the current method of setting the query URL in match_stops.py
try:
    config_file = open('conf/net/ext_service/overpass_server.json')
except:
    print("overpass not configured, falling back to default overleaf.de")
    config_file = open('conf/net/ext_service/overpass_server.json.sample')
config_data = json.load(config_file)
url = config_data["url"]

I think the answer is a combination of 2 and 3, but I'd like some feedback from @shankari on my tests before deciding what to do

@nataliejschultz nataliejschultz marked this pull request as ready for review February 12, 2024 22:03
@shankari
Copy link
Contributor

shankari commented Mar 4, 2024

@nataliejschultz high-level question: why do we need to keep the configuration in a file? There is a single variable in the file.
Recall that for the nominatim use case, we switched the config from a file to an environment variable. I would suggest using the same pattern here.

Change that integrates the paid overpass API with not only the integration test, but the overall functionality of match_stops.py
Figuring out why tests are passing locally and failing on github actions with prints.
Adding some prints to see if I can pinpoint the issue.

Also removing more unnecessary fluff from setup script.
printing some logs to try to understand what's happening
Call isn't going through and I think it's because i'm using http instead of https. This might be a security restriction with GitHub Actions.
Forgot to change this in the match stops module, too.
Getting `module not found` error with __future__, so adding it as a direct import to see if that fixes the error.

Also adding more specificity to the echo in the .yml file
adding import just caused another error. Testing now that I updated the api key.
Increasing specificity for prints; removing unnecessary prints; renaming certain variables so they're more meaningful.
Basic cleanup + putting action on a schedule rather than a push.
@nataliejschultz
Copy link
Contributor Author

Coming back to this as I wait for review on other PRs.
The overpass url is now based on setting the OVERPASS_KEY environment variable in match_stops.py (rather than sample file), to be consistent with current direction of codebase:

try:
    OVERPASS_KEY = os.environ.get("OVERPASS_KEY")
    url = 'https://overpass.geofabrik.de/' + OVERPASS_KEY + '/'
    print("overpass configured")
except:
    print("overpass not configured, falling back to public overpass api")
    url = "https://lz4.overpass-api.de/"

I made sure that using unset OVERPASS_KEY in terminal triggered the "overpass not configured" print, and it did:

Screenshot 2024-06-17 at 1 28 05 PM

This readies the code to use the overpass key for calls in both the tests and any usage of match_stops.py.

Tests passing on GitHub Actions and locally:
Screenshot 2024-06-17 at 1 16 11 PM

Removing superfluous spaces.
Changing environment variable name and adding functionality for the new setup script name.
@shankari shankari merged commit 99d9c21 into e-mission:master Jun 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants