[Django] #25407: Mock GIS call to live domains
Django
------------------------------------------------+------------------------
Reporter: MarkusH | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: GIS | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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`:
{{{#!python
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25407>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: MarkusH | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: GIS | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* Attachment "25407.diff" added.
Django
--------------------------------------+------------------------------------
Reporter: MarkusH | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: GIS | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* easy: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:1>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
* owner: nobody => bak1an
* status: new => assigned
Comment:
Let me try this.
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: MarkusH | Owner: bak1an
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by berkerpeksag):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: MarkusH | Owner: bak1an
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by timgraham):
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: MarkusH | Owner: bak1an
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by bak1an):
Hi all!
Had to wait until a weekend to move forward on this. Here is what I've got
for now:
- Mocking works fine for {{{geoip2}}} since domains are resolved within
python code -
https://github.com/django/django/blob/master/django/contrib/gis/geoip2/base.py#L162
([https://github.com/django/django/compare/master...bak1an:ticket_25407_gis_tests
initial branch is there])
- With {{{geoip}}} it is not that simple. Domain resolution is done within
maxmind C library - https://github.com/maxmind/geoip-
api-c/blob/733b36e88138a6d603e411976c0a18972f8e2045/libGeoIP/GeoIP.c#L1681
and django just uses it as is via ctypes -
https://github.com/django/django/blob/master/django/contrib/gis/geoip/prototypes.py#L122
- 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: MarkusH | Owner: bak1an
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: MarkusH | Owner: bak1an
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by bak1an):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/5343
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:7>
Django
Reporter: MarkusH | Owner: bak1an
Type: | Status: assigned
Cleanup/optimization |
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: MarkusH | Owner: bak1an
Cleanup/optimization |
Component: GIS | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d0ed01cef0c5ebb7ea9a1fb36de823aa01428600" d0ed01c]:
{{{
#!CommitTicketReference repository=""
revision="d0ed01cef0c5ebb7ea9a1fb36de823aa01428600"
Fixed #25407 -- Removed network dependency in GeoIP tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25407#comment:9>