Repository: geopy/geopy
Pull request #480 (2021-03-28 14:51:18)

Add geocoder signature tests
Add new geocoders tests for checking that:
- All geocoders are exported and have expected `__all__` values.
- All required params in `__init__`, `geocode`/`reverse`/`reverse_timezone` methods are present in the signatures and have expected defaults.
- All params are documented with `:param ...:` directives.
- No unexpected public methods are introduced.

This should highlight interface errors early before a PR review when a new geocoder class is introduced.


Patch:
diff --git a/geopy/geocoders/banfrance.py b/geopy/geocoders/banfrance.py
index bd5227c..e51ca05 100644
--- a/geopy/geocoders/banfrance.py
+++ b/geopy/geocoders/banfrance.py
@@ -91,14 +91,14 @@ class BANFrance(Geocoder):
             results the BAN API will return 5 results by default.
             This will be reset to one if ``exactly_one`` is True.
 
+        :param bool exactly_one: Return one result or a list of results, if
+            available.
+
         :param int timeout: Time, in seconds, to wait for the geocoding service
             to respond before raising a :class:`geopy.exc.GeocoderTimedOut`
             exception. Set this only if you wish to override, on this call
             only, the value set during the geocoder's initialization.
 
-        :param bool exactly_one: Return one result or a list of results, if
-            available.
-
         :rtype: ``None``, :class:`geopy.location.Location` or a list of them, if
             ``exactly_one=False``.
 
diff --git a/geopy/geocoders/geonames.py b/geopy/geocoders/geonames.py
index 15a8a0b..d2e1ba2 100644
--- a/geopy/geocoders/geonames.py
+++ b/geopy/geocoders/geonames.py
@@ -274,7 +274,7 @@ class GeoNames(Geocoder):
             exception. Set this only if you wish to override, on this call
             only, the value set during the geocoder's initialization.
 
-        :rtype: :class:`geopy.timezone.Timezone`
+        :rtype: :class:`geopy.timezone.Timezone`.
         """
         ensure_pytz_is_installed()
 
diff --git a/geopy/geocoders/googlev3.py b/geopy/geocoders/googlev3.py
index d88b6f1..2185684 100644
--- a/geopy/geocoders/googlev3.py
+++ b/geopy/geocoders/googlev3.py
@@ -343,7 +343,7 @@ class GoogleV3(Geocoder):
             exception. Set this only if you wish to override, on this call
             only, the value set during the geocoder's initialization.
 
-        :rtype: ``None`` or :class:`geopy.timezone.Timezone`
+        :rtype: ``None`` or :class:`geopy.timezone.Timezone`.
         """
         ensure_pytz_is_installed()
 
diff --git a/setup.py b/setup.py
index cc7efd7..b82b9e1 100755
--- a/setup.py
+++ b/setup.py
@@ -35,6 +35,7 @@ EXTRAS_DEV_TEST = [
     "coverage",
     "pytest-aiohttp",  # for `async def` tests
     "pytest>=3.10",
+    "sphinx",  # `docutils` from sphinx is used in tests
 ]
 
 EXTRAS_DEV_DOCS = [
@@ -58,10 +59,12 @@ setup(
     packages=find_packages(exclude=["*test*"]),
     install_requires=INSTALL_REQUIRES,
     extras_require={
-        "dev": (EXTRAS_DEV_TESTFILES_COMMON +
-                EXTRAS_DEV_LINT +
-                EXTRAS_DEV_TEST +
-                EXTRAS_DEV_DOCS),
+        "dev": sorted(set(
+            EXTRAS_DEV_TESTFILES_COMMON +
+            EXTRAS_DEV_LINT +
+            EXTRAS_DEV_TEST +
+            EXTRAS_DEV_DOCS
+        )),
         "dev-lint": (EXTRAS_DEV_TESTFILES_COMMON +
                      EXTRAS_DEV_LINT),
         "dev-test": (EXTRAS_DEV_TESTFILES_COMMON +


Test patch:
diff --git a/test/geocoders/__init__.py b/test/geocoders/__init__.py
index e69de29..de6f91c 100644
--- a/test/geocoders/__init__.py
+++ b/test/geocoders/__init__.py
@@ -0,0 +1,299 @@
+import importlib
+import inspect
+import pkgutil
+
+import docutils.core
+import docutils.utils
+import pytest
+
+import geopy.geocoders
+from geopy.geocoders.base import DEFAULT_SENTINEL, Geocoder
+
+skip_modules = [
+    "geopy.geocoders.base",  # doesn't contain actual geocoders
+    "geopy.geocoders.osm",  # deprecated
+]
+
+geocoder_modules = sorted(
+    [
+        importlib.import_module(name)
+        for _, name, _ in pkgutil.iter_modules(
+            geopy.geocoders.__path__, "geopy.geocoders."
+        )
+        if name not in skip_modules
+    ],
+    key=lambda m: m.__name__,
+)
+
+geocoder_classes = sorted(
+    {
+        v
+        for v in (
+            getattr(module, name) for module in geocoder_modules for name in dir(module)
+        )
+        if inspect.isclass(v) and issubclass(v, Geocoder) and v is not Geocoder
+    },
+    key=lambda cls: cls.__name__,
+)
+
+
+def assert_no_varargs(sig):
+    assert not [
+        str(p)
+        for p in sig.parameters.values()
+        if p.kind in (inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.VAR_KEYWORD)
+    ], (
+        "Geocoders must not have any (*args) or (**kwargs). "
+        "See CONTRIBUTING.md for explanation."
+    )
+
+
+def assert_rst(sig, doc, allowed_rtypes=(None,)):
+    # Parse RST from the docstring and generate an XML tree:
+    doctree = docutils.core.publish_doctree(
+        doc,
+        settings_overrides={
+            "report_level": docutils.utils.Reporter.SEVERE_LEVEL + 1,
+        },
+    ).asdom()
+
+    def get_all_text(node):
+        if node.nodeType == node.TEXT_NODE:
+            return node.data
+        else:
+            text_string = ""
+            for child_node in node.childNodes:
+                if child_node.nodeName == "system_message":
+                    # skip warnings/errors
+                    continue
+                if child_node.nodeName == "literal":
+                    tmpl = "``%s``"
+                else:
+                    tmpl = "%s"
+                text_string += tmpl % (get_all_text(child_node),)
+            return text_string
+
+    documented_rtype = None
+    documented_params = []
+    for field in doctree.getElementsByTagName("field"):
+        field_name = get_all_text(field.getElementsByTagName("field_name")[0])
+        if field_name == "rtype":
+            assert documented_rtype is None, "There must be single :rtype: directive"
+            field_body = get_all_text(field.getElementsByTagName("field_body")[0])
+            assert field_body, ":rtype: directive must have a value"
+            documented_rtype = field_body.replace("\n", " ")
+        if field_name.startswith("param"):
+            param_name = field_name.split(" ")[-1]
+            documented_params.append(param_name)
+
+    method_params = list(sig.parameters.keys())[1:]  # skip `self`
+
+    assert method_params == documented_params, (
+        "Actual method params set or order doesn't match the documented "
+        ":param ...: directives in the docstring."
+    )
+    assert documented_rtype in allowed_rtypes
+
+
+def test_all_geocoders_are_exported_from_package():
+    expected = {cls.__name__ for cls in geocoder_classes}
+    actual = set(dir(geopy.geocoders))
+    not_exported = expected - actual
+    assert not not_exported, (
+        "These geocoders must be exported (via imports) "
+        "in geopy/geocoders/__init__.py"
+    )
+
+
+def test_all_geocoders_are_listed_in_all():
+    expected = {cls.__name__ for cls in geocoder_classes}
+    actual = set(geopy.geocoders.__all__)
+    not_exported = expected - actual
+    assert not not_exported, (
+        "These geocoders must be listed in the `__all__` tuple "
+        "in geopy/geocoders/__init__.py"
+    )
+
+
+def test_all_geocoders_are_listed_in_service_to_geocoder():
+    assert set(geocoder_classes) == set(geopy.geocoders.SERVICE_TO_GEOCODER.values()), (
+        "All geocoders must be listed in the `SERVICE_TO_GEOCODER` dict "
+        "in geopy/geocoders/__init__.py"
+    )
+
+
+@pytest.mark.parametrize("geocoder_module", geocoder_modules, ids=lambda m: m.__name__)
+def test_geocoder_module_all(geocoder_module):
+    current_all = geocoder_module.__all__
+    expected_all = tuple(
+        cls.__name__
+        for cls in geocoder_classes
+        if cls.__module__ == geocoder_module.__name__
+    )
+    assert expected_all == current_all
+
+
+@pytest.mark.parametrize("geocoder_cls", geocoder_classes)
+def test_init_method_signature(geocoder_cls):
+    method = geocoder_cls.__init__
+    sig = inspect.signature(method)
+
+    assert_no_varargs(sig)
+
+    sig_timeout = sig.parameters["timeout"]
+    assert sig_timeout.kind == inspect.Parameter.KEYWORD_ONLY
+    assert sig_timeout.default is DEFAULT_SENTINEL
+
+    sig_proxies = sig.parameters["proxies"]
+    assert sig_proxies.kind == inspect.Parameter.KEYWORD_ONLY
+    assert sig_proxies.default is DEFAULT_SENTINEL
+
+    sig_user_agent = sig.parameters["user_agent"]
+    assert sig_user_agent.kind == inspect.Parameter.KEYWORD_ONLY
+    assert sig_user_agent.default is None
+
+    sig_ssl_context = sig.parameters["ssl_context"]
+    assert sig_ssl_context.kind == inspect.Parameter.KEYWORD_ONLY
+    assert sig_ssl_context.default is DEFAULT_SENTINEL
+
+    sig_adapter_factory = sig.parameters["adapter_factory"]
+    assert sig_adapter_factory.kind == inspect.Parameter.KEYWORD_ONLY
+    assert sig_adapter_factory.default is None
+
+    assert_rst(sig, method.__doc__)
+
+
+@pytest.mark.parametrize("geocoder_cls", geocoder_classes)
+def test_geocode_method_signature(geocoder_cls):
+    # Every geocoder should have at least a `geocode` method.
+    method = geocoder_cls.geocode
+    sig = inspect.signature(method)
+
+    assert_no_varargs(sig)
+
+    # The first arg (except self) must be called `query`:
+    sig_query = list(sig.parameters.values())[1]
+    assert sig_query.name == "query"
+    assert sig_query.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD
+
+    # The rest must be kwargs-only:
+    sig_kwargs = list(sig.parameters.values())[2:]
+    assert all(p.kind == inspect.Parameter.KEYWORD_ONLY for p in sig_kwargs), (
+        "All method args except `query` must be keyword-only "
+        "(i.e. separated with an `*`)."
+    )
+
+    # kwargs must contain `exactly_one`:
+    sig_exactly_one = sig.parameters["exactly_one"]
+    assert sig_exactly_one.default is True, "`exactly_one` must be True"
+
+    # kwargs must contain `timeout`:
+    sig_timeout = sig.parameters["timeout"]
+    assert sig_timeout.default is DEFAULT_SENTINEL, "`timeout` must be DEFAULT_SENTINEL"
+
+    assert_rst(
+        sig,
+        method.__doc__,
+        allowed_rtypes=[
+            ":class:`geopy.location.Location` or a list of them, "
+            "if ``exactly_one=False``.",  # what3words
+            "``None``, :class:`geopy.location.Location` or a list of them, "
+            "if ``exactly_one=False``.",
+        ],
+    )
+
+
+@pytest.mark.parametrize(
+    "geocoder_cls",
+    [cls for cls in geocoder_classes if getattr(cls, "reverse", None)],
+)
+def test_reverse_method_signature(geocoder_cls):
+    # `reverse` method is optional.
+    method = geocoder_cls.reverse
+    sig = inspect.signature(method)
+
+    assert_no_varargs(sig)
+
+    # First arg (except self) must be called `query`:
+    sig_query = list(sig.parameters.values())[1]
+    assert sig_query.name == "query"
+    assert sig_query.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD
+
+    # The rest must be kwargs-only:
+    sig_kwargs = list(sig.parameters.values())[2:]
+    assert all(p.kind == inspect.Parameter.KEYWORD_ONLY for p in sig_kwargs), (
+        "All method args except `query` must be keyword-only "
+        "(i.e. separated with an `*`)."
+    )
+
+    # kwargs must contain `exactly_one`:
+    sig_exactly_one = sig.parameters["exactly_one"]
+    assert sig_exactly_one.default is True, "`exactly_one` must be True"
+
+    # kwargs must contain `timeout`:
+    sig_timeout = sig.parameters["timeout"]
+    assert sig_timeout.default is DEFAULT_SENTINEL, "`timeout` must be DEFAULT_SENTINEL"
+
+    assert_rst(
+        sig,
+        method.__doc__,
+        allowed_rtypes=[
+            ":class:`geopy.location.Location` or a list of them, "  # what3words
+            "if ``exactly_one=False``.",
+            "``None``, :class:`geopy.location.Location` or a list of them, "
+            "if ``exactly_one=False``.",
+        ],
+    )
+
+
+@pytest.mark.parametrize(
+    "geocoder_cls",
+    [cls for cls in geocoder_classes if getattr(cls, "reverse_timezone", None)],
+)
+def test_reverse_timezone_method_signature(geocoder_cls):
+    method = geocoder_cls.reverse_timezone
+    sig = inspect.signature(method)
+
+    assert_no_varargs(sig)
+
+    # First arg (except self) must be called `query`:
+    sig_query = list(sig.parameters.values())[1]
+    assert sig_query.name == "query"
+    assert sig_query.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD
+
+    # The rest must be kwargs-only:
+    sig_kwargs = list(sig.parameters.values())[2:]
+    assert all(p.kind == inspect.Parameter.KEYWORD_ONLY for p in sig_kwargs), (
+        "All method args except `query` must be keyword-only "
+        "(i.e. separated with an `*`)."
+    )
+
+    # kwargs must contain `timeout`:
+    sig_timeout = sig.parameters["timeout"]
+    assert sig_timeout.default is DEFAULT_SENTINEL, "`timeout` must be DEFAULT_SENTINEL"
+
+    assert_rst(
+        sig,
+        method.__doc__,
+        allowed_rtypes=[
+            ":class:`geopy.timezone.Timezone`.",
+            "``None`` or :class:`geopy.timezone.Timezone`.",
+        ],
+    )
+
+
+@pytest.mark.parametrize("geocoder_cls", geocoder_classes)
+def test_no_extra_public_methods(geocoder_cls):
+    methods = {
+        n
+        for n in dir(geocoder_cls)
+        if not n.startswith("_") and inspect.isfunction(getattr(geocoder_cls, n))
+    }
+    allowed = {
+        "geocode",
+        "reverse",
+        "reverse_timezone",
+    }
+    assert methods <= allowed, (
+        "Geopy geocoders are currently allowed to only have these methods: %s" % allowed
+    )
diff --git a/test/geocoders/algolia.py b/test/geocoders/algolia.py
index 3a8f55b..dba3805 100644
--- a/test/geocoders/algolia.py
+++ b/test/geocoders/algolia.py
@@ -33,14 +33,6 @@ class TestAlgoliaPlaces(BaseTestGeocoder):
         )
         assert 'A272' in location.address
 
-    async def test_reverse_no_result(self):
-        await self.reverse_run(
-            # North Atlantic Ocean
-            {'query': (35.173809, -37.485351)},
-            {},
-            expect_failure=True
-        )
-
     async def test_explicit_type(self):
         location = await self.geocode_run(
             {'query': 'Madrid', 'type': 'city', 'language': 'en'},
@@ -68,7 +60,7 @@ class TestAlgoliaPlaces(BaseTestGeocoder):
         assert "Madrid" in location.address
 
     async def test_countries_no_result(self):
-        countries = ["NO", "IT"]
+        countries = ["UA", "RU"]
         await self.geocode_run(
             {'query': 'Madrid', 'language': 'en',
              'countries': countries},
diff --git a/test/geocoders/mapquest.py b/test/geocoders/mapquest.py
index 7830e2b..ff06bfd 100644
--- a/test/geocoders/mapquest.py
+++ b/test/geocoders/mapquest.py
@@ -35,13 +35,6 @@ class TestMapQuest(BaseTestGeocoder):
             expect_failure=True,
         )
 
-    async def test_geocode_empty(self):
-        await self.geocode_run(
-            {'query': 'sldkfhdskjfhsdkhgflaskjgf'},
-            {},
-            expect_failure=True,
-        )
-
     async def test_geocode_bbox(self):
         await self.geocode_run(
             {
diff --git a/test/geocoders/util.py b/test/geocoders/util.py
index 677236c..d470606 100644
--- a/test/geocoders/util.py
+++ b/test/geocoders/util.py
@@ -109,10 +109,11 @@ class BaseTestGeocoder(ABC):
             skiptest_on_errors=skiptest_on_errors,
             **payload,
         )
+        if expect_failure:
+            assert result is None
+            return
         if result is None:
-            if expect_failure:
-                return
-            elif skiptest_on_failure:
+            if skiptest_on_failure:
                 pytest.skip('%s: Skipping test due to empty result' % cls.__name__)
             else:
                 pytest.fail('%s: No result found' % cls.__name__)
@@ -138,10 +139,11 @@ class BaseTestGeocoder(ABC):
             skiptest_on_errors=skiptest_on_errors,
             **payload,
         )
+        if expect_failure:
+            assert result is None
+            return
         if result is None:
-            if expect_failure:
-                return
-            elif skiptest_on_failure:
+            if skiptest_on_failure:
                 pytest.skip('%s: Skipping test due to empty result' % cls.__name__)
             else:
                 pytest.fail('%s: No result found' % cls.__name__)