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

#160 Add Route Testing #218

Merged
merged 2 commits into from Nov 19, 2017

Conversation

Projects
None yet
2 participants
@WGierke
Copy link
Contributor

WGierke commented Nov 8, 2017

I added a simple Python script that can test whether routes error. It is highly customizable (e.g. one could add to check for the occurrence of the word "error" or "exception" etc.).

Reference to official issue

This addresses #160

Motivation and Context

Currently, the Travis test passes even if some routes are non-functional (like http://0.0.0.0:8001/).

How Has This Been Tested?

It's currently not passing due to the unfixed issue #203 .

➜  concept-to-clinic git:(160_route_errors) ✗ bash tests/test_docker.sh
...
+ python tests/test_routes.py
http://localhost:8001/ returns 500
+ echo 'ERROR: test_routes did not pass. Check above for details.'
ERROR: test_routes did not pass. Check above for details.
+ exit 1

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well

def check_page(url):
response = requests.get(url)
if response.status_code != 200:

This comment has been minimized.

@lamby

lamby Nov 9, 2017

Contributor

The canonical way of doing this with requests is using/catching raise_for_status - I think it gives you a nicer error message, but I'm not sure. Can you have a look?

@WGierke WGierke force-pushed the WGierke:160_route_errors branch 3 times, most recently from 1284dec to eb39601 Nov 9, 2017

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Nov 9, 2017

@lamby Thanks for the hint, that indeed is a way better solution to test for unsuccessful responses. However, apparently one has to do some more bash magic than expected. The docker containers must be started so the ports are served at localhost. However, the compile_docs service is producing such a big output that it hits the Travis log limit of 4MB and the test fails (apart from that, fixing compile_docs might be a new separate issue?). Thus, I'm piping the output to /dev/null. While the tests now work on my machine, they're still failing on Travis:

➜  concept-to-clinic git:(160_route_errors) ✗ git log | head -n 1
commit 514ceecd7e99ff5877751c3d51ce7b1da38f0a98
➜  concept-to-clinic git:(160_route_errors) ✗ bash tests/test_docker.sh
...
+ docker-compose -f local.yml run interface python manage.py makemigrations --dry-run --check
Starting concepttoclinic_postgres_1 ... 
Starting base ... 
Starting base ... done
Starting concepttoclinic_prediction_1 ... done
Postgres is up - continuing...
/usr/local/lib/python3.6/dist-packages/environ/environ.py:618: UserWarning: Error reading /app/.env - if you're not configuring your environment separately, check this.
  "environment separately, check this." % env_file)
No changes detected
+ sleep 60
+ docker-compose -f local.yml up
+ python tests/test_routes.py
Fetching http://localhost:8000/ ...
Fetching http://localhost:8000/api ...
Fetching http://localhost:8080/ ...
Fetching http://localhost:8001/ ...
Fetching http://localhost:8001/classify/predict/ ...
Fetching http://localhost:8001/identify/predict/ ...
Fetching http://localhost:8001/segment/predict/ ...
Fetching http://localhost:8002/ ...
➜  concept-to-clinic git:(160_route_errors) ✗ echo "$?"        
0

I'd be happy over any solution suggestions :)

Edit: Alright, it works now! Setting the sleep duration to 60 seconds seems to have solved the problem :)

@WGierke WGierke force-pushed the WGierke:160_route_errors branch from 0e85259 to 514ceec Nov 9, 2017

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 12, 2017

I really don't like the sleep - can we not test instead? :p

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Nov 12, 2017

:D Okay so how about, instead of sleep 60, we try to connect to the 4 ports and after 60 seconds we time out and exit with 1, otherwise we perform the tests? Because we somehow have to make sure that the docker containers are ready to be tested - we can't assume that they serve as soon as the docker-compose up command has been fired, otherwise we'll get random Connection refused errors when invoking requests.get(url).

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 13, 2017

are ready to be tested

Right, can't we just wait for that to happen by testing them? Not sure why we need a timeout for this.

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Nov 13, 2017

Okay but what if a port won't be opened because the service died for some reason? In that case we test endlessly which might be no problem on Travis since there is a timeout but not on local computers.

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 15, 2017

@WGierke Sorry for the delay in getting back to you. Perhaps I'm misunderstanding something - if the port is open and subsequently closes, we are safe to assumine something is FUBAR and fail the test. However, AIUI this is about waiting for things to spin-up, no? Sorry if I'm misunderstanding something here. :)

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Nov 16, 2017

this is about waiting for things to spin-up, no
Exactly, that's what I meant. Sorry for the confusion :)

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 17, 2017

Getcha. So to summarise, I understand the need to timeout in the case that the port never materialises, but it would be still be great to start the tests before this 60 seconds if the ports are open. ie. would you be okay pushing ahead with #218 (comment) ? :)

@WGierke WGierke force-pushed the WGierke:160_route_errors branch from 9c8bdc4 to e33dbd3 Nov 19, 2017

@WGierke WGierke force-pushed the WGierke:160_route_errors branch from e33dbd3 to 0e35971 Nov 19, 2017

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Nov 19, 2017

@lamby Yes ;) It should be done now.

@lamby lamby merged commit 7f73134 into drivendataorg:master Nov 19, 2017

2 checks passed

concept-to-clinic/cla @WGierke has signed the CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 19, 2017

Awesome, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment