Opened 9 years ago

Closed 9 years ago

#25407 closed Cleanup/optimization (fixed)

Remove network dependency in GeoIP tests

Reported by: Markus Holtermann Owned by: Anton Baklanov
Component: GIS Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Occasionally some GIS tests fail due to connectivity or availability issues when trying to get live data. Those requests should be mocked away. One of the tests failing is gis_tests.test_geoip2.GeoIPTest.test05_unicode_response:

ERROR [15.017s]: test05_unicode_response (gis_tests.test_geoip2.GeoIPTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jenkins/workspace/pull-requests-trusty/database/postgis/label/trusty-pr/python/python2.7/tests/gis_tests/test_geoip2.py", line 127, in test05_unicode_response
    d = g.city("nigde.edu.tr")
  File "/home/jenkins/workspace/pull-requests-trusty/database/postgis/label/trusty-pr/python/python2.7/django/contrib/gis/geoip2/base.py", line 172, in city
    enc_query = self._check_query(query, city=True)
  File "/home/jenkins/workspace/pull-requests-trusty/database/postgis/label/trusty-pr/python/python2.7/django/contrib/gis/geoip2/base.py", line 162, in _check_query
    query = socket.gethostbyname(query)
gaierror: [Errno -2] Name or service not known

Attachments (1)

25407.diff (978 bytes ) - added by Tim Graham 9 years ago.

Download all attachments as: .zip

Change History (10)

by Tim Graham, 9 years ago

Attachment: 25407.diff added

comment:1 by Tim Graham, 9 years ago

Easy pickings: set
Summary: Mock GIS call to live domainsRemove network dependency in GeoIP tests

Attached an example of how to fix the test in question, but needs to be applied to the other tests in that file as well as test_geoip.py. Should be fairly straightforward.

comment:2 by Anton Baklanov, 9 years ago

Owner: changed from nobody to Anton Baklanov
Status: newassigned

Let me try this.

comment:3 by Berker Peksag, 9 years ago

Instead of mocking socket.gethostbyname()(the IP can be changed), you can skip the test if there is a transient failure. CPython uses the following helper: https://hg.python.org/cpython/file/default/Lib/test/support/__init__.py#l1294 The API could be better, of course.

comment:4 by Tim Graham, 9 years ago

Thanks for the suggestion Berker, but in this case the IP of the website isn't important for the purposes of the test (other than it must be in the tested country/city), so actually I think the mocking approach is better because we don't have to worry if the website does relocate their server outside of the given test country/city (of course the IP could be reassigned geographically too, but at that point we just have to update the test).

comment:5 by Anton Baklanov, 9 years ago

Hi all!

Had to wait until a weekend to move forward on this. Here is what I've got for now:

(initial branch is there)

  • Other tests seem to work fine without network (had 146 skips though).

Unfortunately Berker's hint on catching network errors during the test (using context manager for example) is not going to work in this case because the actual network error is being consumed inside the C library and all we have is just None as a result of a geoip query.

I can see the following options here:

  • Introduce some sort of a skip decorator or just a routine inside a testsuite for checking if there is a proper network connection (and possibly skipping tests that require internet). This seems a bit tricky to me. How do we check connectivity? Which resources should we query? Besides, for this particular case we don't need a full internet connection at all, just DNS (local caching server will do fine).
  • Add hacks to geoip tests (3 tests require such 'fixing') so they will try to do socket.gethostbyname for domains under test in the beginning or setUp. In case gethostbyname fails - tests will proceed with IP checks only, skipping fqdn checks (probably issuing a warning, so it's easy to catch if the issue persists).
  • Leave it as is. geoip is deprecated after all.

@timgraham, @berkerpeksag - could you advise a proper way to go here?

Meanwhile, I'm going to get some sleep. Perhaps there are other solutions hidden out there which I can't see now.

Version 0, edited 9 years ago by Anton Baklanov (next)

comment:6 by Tim Graham, 9 years ago

I wouldn't worry about the deprecated geoip tests if it's a big pain. In fact, I only recently enabled them on Jenkins. If they are problematic in the future we can just disable them.

comment:8 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In d0ed01c:

Fixed #25407 -- Removed network dependency in GeoIP tests.

Note: See TracTickets for help on using tickets.
Back to Top