Creating TestNominatim.py #929

nataliejschultz · 2023-07-21T06:23:59Z

Creating an integration test to make sure nominatim calls work properly. The tests call various classes from Nominatim.py and ensure that the format of the returned values are as expected with the current calling format.

Creating an integration test to make sure nominatim calls work properly. The tests call various classes from Nominatim.py and ensures that the format of the responses are as expected with the current calling format.

shankari

@nataliejschultz

where are the cleanup changes to the module? Removing the google APIs, reading in the config as an environment variable....
this test is not actually being run right now - we should move it to integrationTests and run it the same way as the other integration tests, against the docker container for nominatim. Note that the exact strings returned here might also change as the OSM data changes

emission/individual_tests/TestNominatim.py

save_ground_truth.py: added parser argument line so that ground truth can be saved successfully using "diary" argument. Generated a ground truth file while testing save_ground_truth.py. Modified nominatim.py to remove all google based functionality. Created new docker-compose.yml, Dockerfile, and a few shell scripts in emission/integrationTests to run the integration tests while starting Nominatim server through docker container. nominatim-docker-test.yml sets up the servers in GH actions and runs docker-compose. runIntegrationTests.sh uses the discover method to run all tests in emission/integrationTests. TestNominatimBeta.py compares the result of a reverse geocoding call to nominatim with an expected output. removed ports from docker-compose.tests.yml per Shankari's instruction.

shankari

Apparently this does not include your most recent changes.
Can you update and I will re-review?

bin/debug/save_ground_truth.py

emission/integrationTests/TestNominatimBeta.py

emission/integrationTests/docker-compose.yml

emission/integrationTests/nominatimTests/nominatim-docker-test.yml

Moved nominatim-docker-test.yml to github workflows directory and set path to docker compose. Edited docker-compose.yml to reflect rhode island data instead of norcal; updated ports. Added environment variable to dockerfile (not quite polished yet) so that nominatim query can be run with localhost when appropriate.

shankari

This is better, but I still think that there are errors with the relative paths.
Can you indicate how (if?) you tested this?

"and to put in outputs periodically when you have finished "one set of changes""
and: #929 (comment)

.github/workflows/nominatim-docker-test.yml

emission/integrationTests/docker-compose.yml

emission/integrationTests/Dockerfile

Trying to get nominatim-docker-test.yml to run in GH actions with small syntax changes. Also changing the repo from which docker builds to current directory.

Removed unused imports. Added a line that temporarily sets the query URL in eco to that of the environment variable, just for the purpose of testing. This way, nominatim.json can stay the same, while the testing environment variable can still be used and modified easily. Also resolved hardcoding of nominatim URL per Shankari's request.

Added os import, removed print statement.

shankari

e-mission/e-mission-docs#937 (comment)
is wrong. Please see comments below.

Again, I would encourage you to test as you go along so you know that the direction that you are taking is correct.

emission/individual_tests/TestNominatim.py

emission/integrationTests/Dockerfile

emission/net/ext_service/geocoder/nominatim.py

Changed file to run on PR (temporarily) to see if it will run.

- Added nominatim network in docker-compose so that db and nominatim are separated. This should allow them to run on different ports, while communicating with the web server. - Testing to see where pythonpath is set in GH actions/if we need to specify in the runner specs.

Dockerfile: updated query url to access the nominatim container while running on web server. start_integration_tests: moved a commented out section. Nominatim.py: Added environment variable to replace nominatim.json file. added multiple prints to (hopefully) see what's going on with the tests.

…atimBeta. Nominatim.py: removing prints TestNominatimBeta: removed and combined with testnominatim.py runIntegrationTests.sh: modified to only call TestNominatim.py, since the other integration tests don't work yet. TestNominatim.py: updated environment variable functionality to work with both AWS and the testing environment. Modified test_geocode to only call geocode function once. Changed test query for test_get_json_geo so that it would return a list.

Directly testing two different potential solutions to the URL query issue in GH actions. Will change the environment variable for NOMINATIM_QUERY_URL when I find the right way to call the container in the GH actions network.

TestNominatim.py: commented out irrelevant tests that won't help with troubleshooting Nominatim.py: Added port to nominatim call. Trying out calling by image name instead of name of container. runIntegrationTests.sh: Added command to list out processes on 8080

Did not recognize this command; trying again.

Specifying nominatim-network is separated from the service name, nominatim. It is possible that this was causing an issue with accessing the network.

Trying to remove universal network at the bottom of the file, and removing http.

Modifying network to add a bridge driver, specifying network names, adding link to nominatim network from web server, and assigning web server its own ports. All of these changes are in an attempt to allow web server to access the nominatim container and make an API call.

Since my network was working properly locally, I'm now thinking that the issue has to do with timing. I don't think the rhodeisland-nominatim container had enough time to setup and wait/listen, which caused the connection to be refused. Changes: start_integration_tests: removing echoes dockerfile: reverting to old environment variable compose: removing version(hopefully this is allowed) and reverting everything except adding condition check. This should cause web-server to wait for the nominatim service to be up and running before trying to connect. We will see!

emission/integrationTests/Dockerfile

docker-compose: Added healthcheck to nominatim service, in tandem with restart policy for web server. Hopefully, this gives nominatim enough time to initialize. Also added environment variable for query url. Dockerfile: removed environment variable, and added to compose.

Added --wait to docker-compose command, which supposedly will cause the other containers to wait for my nominatim service to pass its health check. Removed restart on failure, which isn't necessary with wait.

start_integration_tests: Added sleep command to directly delay starting the tests, since docker methods weren't working. This worked locally, and I could see where the health checks were taking place. Will be helpful to, at the very least, see how long nominatim needs to get started up. nominatim-docker-test.yml: Reverted to old command. TestNominatim.py: Un-commented other tests.

Giving nominatim twice as long to initialize. I cannot get my local container to not build with old image data, despite deleting the old images.

A small change while I'm working on implementing dockerize. Mock should bypass the issue with the place_id, which should get the tests to work!! :)

Testing dockerize. Added docker install to dockerfile, and added a line in our start_integration_tests.sh to wait for a 200 from the rhodeisland-nominatim container. Here's what it looks like on my machine: ``` integrationtests-web-server-1 | 2023/08/28 18:27:10 Waiting for: http://rhodeisland-nominatim:8080 integrationtests-web-server-1 | 2023/08/28 18:27:10 Received 200 from http://rhodeisland-nominatim:8080 ``` Additionally, my tests passed locally: ``` integrationtests-web-server-1 | Ran 6 tests in 0.344s integrationtests-web-server-1 | integrationtests-web-server-1 | OK ```

shankari · 2023-10-18T22:10:57Z

I configured the GEOFABRIK_API secret and the tests are still failing
I don't see the printout for NOMINATIM_QUERY_URL

There is also another failure due to

AttributeError: 'Cleanedplace' instance has no attribute 'display_name'

shankari · 2023-10-18T22:14:23Z

I'm also seeing at least one location where the nominatim URL is apparently not available.

web-server_1  | Nominatim URL not configured, place decoding must happen on the client

nataliejschultz · 2023-10-18T22:15:56Z

I configured the GEOFABRIK_API secret and the tests are still failing I don't see the printout for NOMINATIM_QUERY_URL

I'll add a print for the query urls to see what's going on

nataliejschultz · 2023-10-18T22:18:03Z

I'm also seeing at least one location where the nominatim URL is apparently not available.
web-server_1  | Nominatim URL not configured, place decoding must happen on the client

Hmmm. Some prints are in order. Let's see what happens when I revert the last change and add a print

Despite being on one network, calling the container by name might have messed things up. Also adding a print for debugging.

nataliejschultz · 2023-10-18T22:35:07Z

It looks like it's not pulling the secret successfully. Did you put it in repository secrets, or environment secrets @shankari ? I have it in repository secrets on my fork.

shankari · 2023-10-19T17:22:49Z

Port 8080 should not have anything to do with it. Now that you are on the same network, you should be able to use the local network to connect

It looks like it's not pulling the secret successfully. Did you put it in repository secrets, or environment secrets @shankari ? I have it in repository secrets on my fork.

Repository secrets

nataliejschultz · 2023-10-19T17:25:31Z

Port 8080 should not have anything to do with it.

It looks like it's not pulling the secret successfully. Did you put it in repository secrets, or environment secrets @shankari ? I have it in repository secrets on my fork.

Repository secrets

Do you know if you input it as a string? I don't know offhand what the problem could be, since it was pulling just fine on my fork.

Going to try one of the solutions from this link

Slightly modifying the workflow to see if the API key can be pulled to the workflow successfully.

shankari · 2023-10-19T17:46:57Z

Do you know if you input it as a string? I don't know offhand what the problem could be, since it was pulling just fine on my fork.

I can't see the existing secret - only override or delete it. I didn't do anything specific to mark it as a string - I just copied and pasted the API key. Was I supposed to put it in quotes or something?

Can you access one of other repo secrets, like the DOCKER_USER, for example?

Replacing the API key with a known working repo secret to see if the problem is GH accessing the repo secrets, or something with the way the secret was put in.

nataliejschultz · 2023-10-19T20:16:10Z

Can you access one of other repo secrets, like the DOCKER_USER, for example?

I replaced the GEOFABRIK_API secret with the DOCKER_USER (a known-to-work secret?) and it did not carry through to the test. So, there must be some other reason that the secret is not being pulled from GH actions secrets properly. I'm going to try to set the secret as an environment variable in the workflow file outside of the docker compose command and see if that works :)

Setting the geofabrik api key as an environment variable prior to running compose. Hopefully, this will pull the secret properly and allow it to be used in the workflow.

shankari · 2023-10-19T20:21:54Z

It is known to work on https://github.com/e-mission/e-mission-server/blob/master/.github/workflows/image_build_push.yml
You might want to use that as a template

Using DOCKER_USER in place of GEOFABRIK_API until the secret is successfully carried through to the test.

echoing the secret to see if it's being pulled at all

Maybe it's because the quotes are not there?

nataliejschultz · 2023-10-24T17:24:51Z

Can you access one of other repo secrets, like the DOCKER_USER, for example?

I have tried many different ways, and essentially, not yet. I've replaced the GEOFABRIK secret with ${{ secrets.DOCKER_USER }}, and made various modifications:

Requiring secret on workflow call:

on:
  workflow_call:
    secrets:
      DOCKER_USER:
        description: "Docker user known working secret test"
        required: true

Setting secret as an explicit environment variable:

env:
  GEOFABRIK_API: ${{ secrets.DOCKER_USER }}

and then printing that variable

    - name: Secret test
      run: echo "$GEOFABRIK_API"

which results in this:

Run echo $GEOFABRIK_API
  echo $GEOFABRIK_API
  shell: /usr/bin/bash -e {0}
  env:
    GEOFABRIK_API:

I'm thinking it has to do with me being the one pushing to the branch. Since I am not the repository owner, and I likely don't have permission to use repository secrets, it's blocking me from using them in my workflow. I am not sure, though.

shankari · 2023-10-24T20:13:19Z

Bingo!
https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions#using-secrets-in-a-workflow

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

Given that, and the fact that we use a fork and pull request model, we can never test this code on pull requests.
So let's remove the pull requests from the "trigger" for the workflow and let's add a CI badge for this regular run into the README

Adding the workflow status badge to the readme Running the test on my fork again before making further changes.

???? Not sure why it didn't run last push.

Previous run of this test succeeded. Going to see what happens when I remove port 8080 as the only change (again)

final changes (I hope!) to setup module + re-adding the port since the test did not work without it.

Reverting to running test on schedule since (I think) everything else is done.

shankari · 2023-10-31T19:59:15Z

@nataliejschultz looks like that worked? Can you confirm so that I can merge?

Done??

nataliejschultz · 2023-11-01T03:58:10Z

@nataliejschultz looks like that worked? Can you confirm so that I can merge?

I re-instated the workflow schedule, so hopefully (if everything else is okay with you) it's done?!

shankari

Ok this finally looks good.
I am going to merge and see how well it runs.
We should also do something similar for overpass.de and the transit matching code but let's get the email messages done first.

Creating TestNominatim.py

ceaddc7

Creating an integration test to make sure nominatim calls work properly. The tests call various classes from Nominatim.py and ensures that the format of the responses are as expected with the current calling format.

shankari requested changes Aug 8, 2023

View reviewed changes

emission/individual_tests/TestNominatim.py Outdated Show resolved Hide resolved

nataliejschultz marked this pull request as draft August 8, 2023 15:00

shankari requested changes Aug 10, 2023

View reviewed changes

shankari requested changes Aug 11, 2023

View reviewed changes

.github/workflows/nominatim-docker-test.yml Outdated Show resolved Hide resolved

emission/integrationTests/docker-compose.yml Outdated Show resolved Hide resolved

emission/integrationTests/Dockerfile Outdated Show resolved Hide resolved

nataliejschultz mentioned this pull request Aug 11, 2023

⚡ 🚸 Re-enable populating addresses in trips and make sure that they are used on the phone e-mission/e-mission-docs#937

Open

nataliejschultz added 3 commits August 13, 2023 21:42

Top of workflow edits

b7e77fb

Trying to get nominatim-docker-test.yml to run in GH actions with small syntax changes. Also changing the repo from which docker builds to current directory.

Update TestNominatim.py

01a3b4d

Added os import, removed print statement.

shankari requested changes Aug 15, 2023

View reviewed changes

emission/individual_tests/TestNominatim.py Outdated Show resolved Hide resolved

emission/integrationTests/Dockerfile Outdated Show resolved Hide resolved

emission/net/ext_service/geocoder/nominatim.py Outdated Show resolved Hide resolved

nataliejschultz added 11 commits August 15, 2023 11:18

Testing GH workflow file

fd453e6

Changed file to run on PR (temporarily) to see if it will run.

URL testing

6b9d98d

Directly testing two different potential solutions to the URL query issue in GH actions. Will change the environment variable for NOMINATIM_QUERY_URL when I find the right way to call the container in the GH actions network.

URL Testing pt 2

2bb9e66

TestNominatim.py: commented out irrelevant tests that won't help with troubleshooting Nominatim.py: Added port to nominatim call. Trying out calling by image name instead of name of container. runIntegrationTests.sh: Added command to list out processes on 8080

Removing lsof

3b76ac1

Did not recognize this command; trying again.

Network naming change

927a6f7

Specifying nominatim-network is separated from the service name, nominatim. It is possible that this was causing an issue with accessing the network.

"Universal" network removal

1ada684

Trying to remove universal network at the bottom of the file, and removing http.

shankari reviewed Aug 22, 2023

View reviewed changes

emission/integrationTests/Dockerfile Outdated Show resolved Hide resolved

nataliejschultz added 6 commits August 21, 2023 22:36

Remove restart on failure

b072a89

Added --wait to docker-compose command, which supposedly will cause the other containers to wait for my nominatim service to pass its health check. Removed restart on failure, which isn't necessary with wait.

More sleep

93420dc

Giving nominatim twice as long to initialize. I cannot get my local container to not build with old image data, despite deleting the old images.

Adding mock to bypass place_id.

9af4e2e

A small change while I'm working on implementing dockerize. Mock should bypass the issue with the place_id, which should get the tests to work!! :)

Re-Adding port 8080 (again)

70ff4ab

Despite being on one network, calling the container by name might have messed things up. Also adding a print for debugging.

Pulling repo secret

fa508ad

Slightly modifying the workflow to see if the API key can be pulled to the workflow successfully.

Replacing secret

a6be71a

Replacing the API key with a known working repo secret to see if the problem is GH accessing the repo secrets, or something with the way the secret was put in.

Setting secret as env var in workflow file

451a428

Setting the geofabrik api key as an environment variable prior to running compose. Hopefully, this will pull the secret properly and allow it to be used in the workflow.

nataliejschultz added 3 commits October 19, 2023 14:55

Switching to using docker_user until I figure this out.

b027485

Using DOCKER_USER in place of GEOFABRIK_API until the secret is successfully carried through to the test.

Secret issues

2000144

echoing the secret to see if it's being pulled at all

Adding quotes

e3b93eb

Maybe it's because the quotes are not there?

nataliejschultz added 6 commits October 26, 2023 14:29

Updating readme + Making sure the test still works (running on fork)

0b9e455

Adding the workflow status badge to the readme Running the test on my fork again before making further changes.

Getting test to run on my branch again

ce2db01

???? Not sure why it didn't run last push.

Removing port 8080 (yet again!!)

3bf52de

Previous run of this test succeeded. Going to see what happens when I remove port 8080 as the only change (again)

Re-adding port + setup changes

8c2efb0

final changes (I hope!) to setup module + re-adding the port since the test did not work without it.

Re-instating schedule (test will fail but that is expected)

b7e0165

Reverting to running test on schedule since (I think) everything else is done.

Testing removing port export but still specifying 8080 in container url

f5c35a0

Re-instating schedule (for the last time?!)

baf087a

Done??

shankari approved these changes Nov 1, 2023

View reviewed changes

shankari merged commit 81c4314 into e-mission:master Nov 1, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating TestNominatim.py #929

Creating TestNominatim.py #929

nataliejschultz commented Jul 21, 2023

shankari left a comment •

edited

Loading

shankari left a comment

shankari left a comment •

edited

Loading

shankari left a comment

shankari commented Oct 18, 2023

shankari commented Oct 18, 2023

nataliejschultz commented Oct 18, 2023

nataliejschultz commented Oct 18, 2023

nataliejschultz commented Oct 18, 2023

shankari commented Oct 19, 2023 •

edited

Loading

nataliejschultz commented Oct 19, 2023 •

edited

Loading

shankari commented Oct 19, 2023

nataliejschultz commented Oct 19, 2023 •

edited

Loading

shankari commented Oct 19, 2023

nataliejschultz commented Oct 24, 2023

shankari commented Oct 24, 2023

shankari commented Oct 31, 2023 •

edited

Loading

nataliejschultz commented Nov 1, 2023 •

edited

Loading

shankari left a comment

Creating TestNominatim.py #929

Creating TestNominatim.py #929

Conversation

nataliejschultz commented Jul 21, 2023

shankari left a comment • edited Loading

Choose a reason for hiding this comment

shankari left a comment

Choose a reason for hiding this comment

shankari left a comment • edited Loading

Choose a reason for hiding this comment

shankari left a comment

Choose a reason for hiding this comment

shankari commented Oct 18, 2023

shankari commented Oct 18, 2023

nataliejschultz commented Oct 18, 2023

nataliejschultz commented Oct 18, 2023

nataliejschultz commented Oct 18, 2023

shankari commented Oct 19, 2023 • edited Loading

nataliejschultz commented Oct 19, 2023 • edited Loading

shankari commented Oct 19, 2023

nataliejschultz commented Oct 19, 2023 • edited Loading

shankari commented Oct 19, 2023

nataliejschultz commented Oct 24, 2023

shankari commented Oct 24, 2023

shankari commented Oct 31, 2023 • edited Loading

nataliejschultz commented Nov 1, 2023 • edited Loading

shankari left a comment

Choose a reason for hiding this comment

shankari left a comment •

edited

Loading

shankari left a comment •

edited

Loading

shankari commented Oct 19, 2023 •

edited

Loading

nataliejschultz commented Oct 19, 2023 •

edited

Loading

nataliejschultz commented Oct 19, 2023 •

edited

Loading

shankari commented Oct 31, 2023 •

edited

Loading

nataliejschultz commented Nov 1, 2023 •

edited

Loading