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 custom pytest marker: skip extract location tests in case of RequestTimeoutError from geolocator Nominatim #2002

Closed
wants to merge 13 commits into from

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Apr 12, 2023

Description

I have actually tested this by force-failing geolocator from within itself (nifty way they coded up their exceptions raisers), and there are 8 tests that get skippity-skipped, if a GeocoderUnavailable exception is raised (no matter what message - who cares).

A few notes about this implementation:

  • the place where the custom marker is located is in conftest.py - so we can extend it to other related HTTP issues as @bouweandela suggested (am not even sure if that'd work if the marker sat in a given test module and not in the conftest top-level script)
  • geopy's geolocator is terrible - it's either very busy, or very fragile, or both - and the connection drops VERY frequently when CI's run (not so much , at all actually, when I run locally); when too many retries with no cigar happen, it spits out a collection of error instances, sometimes different, for the same underlying reason
  • I tried gather all the possible cases of error instances (I literally played a game of exceptions whack-a-mole), but am not 100% the collection is exhaustive
  • skipping these tests when these errors happen offers us a more or less stable testing session, but it's very inefficient since the skip happens only after the darn thing tried a few times to connect

My take would be to get rid of this piece of unstable c**p

Closes #1982


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@valeriupredoi
Copy link
Contributor Author

this Geocoder thing is either very busy or very fragile, or both, the test now reveals a possible GeocoderRateLimited error as well, which I am not catching in the custom marker 🤦‍♂️

@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #2002 (05a714b) into main (435ade0) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2002      +/-   ##
==========================================
- Coverage   92.79%   92.71%   -0.09%     
==========================================
  Files         236      236              
  Lines       12438    12438              
==========================================
- Hits        11542    11532      -10     
- Misses        896      906      +10     

see 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@valeriupredoi
Copy link
Contributor Author

@ESMValGroup/technical-lead-development-team I reckon we should get this in sooner than later since these HTTP request fails are multiplying now at every step of our testing sessions - it's getting a bit ridiculous if you asked me, I'll go straight and open an issue at Geopy

@bouweandela
Copy link
Member

The people running the geolocation service seem to not want people to run unit tests against it. Therefore, the solution that honours their usage policy would be to use a mock, instead of keep using the service in CI but skipping the test if it fails.

@bouweandela
Copy link
Member

Ah, I just saw you reached the same conclusion in the issue and @schlunma implemented a mock in #2005, which has now been merged. Can this be closed then @valeriupredoi?

@valeriupredoi
Copy link
Contributor Author

cheers @bouweandela - indeed this is ready to be closed, being superseded by #2005 that is the right way to go, but this may be quirky enough if we ever decide to add custom pytest markers (that I didn't know how to do, until I got to work on this), so I am going to change its name so at least I can find it in the future 👍

@valeriupredoi valeriupredoi changed the title Skip extract location tests in case of RequestTimeoutError from geolocator Nominatim Add custom pytest marker: skip extract location tests in case of RequestTimeoutError from geolocator Nominatim Apr 19, 2023
@valeriupredoi valeriupredoi deleted the skip_extract_location branch April 19, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants