Ensure RedisIntegration is disabled, unless redis is installed (#…

…2504)

* Add test to ensure redis integration disabled unless installed

* Integrations added to enabled list if actually installed

* Move test to test_basics.py

* Code review suggestions

* Fixed test failures

* Add unit test to check multiple `setup_integrations` calls

* fix type hint for 2.7

* Added staticmethod

* Move test to `test_basics`

sentry_sdk/integrations/__init__.py

@@ -16,6 +16,11 @@
 
 
 _installer_lock = Lock()
+
+# Set of all integration identifiers we have attempted to install
+_processed_integrations = set()  # type: Set[str]
+
+# Set of all integration identifiers we have actually installed
 _installed_integrations = set()  # type: Set[str]
 
 
@@ -121,7 +126,7 @@ def setup_integrations(
 
     for identifier, integration in iteritems(integrations):
         with _installer_lock:
-            if identifier not in _installed_integrations:
+            if identifier not in _processed_integrations:
                 logger.debug(
                     "Setting up previously not enabled integration %s", identifier
                 )
@@ -144,8 +149,16 @@ def setup_integrations(
                     logger.debug(
                         "Did not enable default integration %s: %s", identifier, e
                     )
+                else:
+                    _installed_integrations.add(identifier)
+
+                _processed_integrations.add(identifier)
 
-                _installed_integrations.add(identifier)
+    integrations = {
+        identifier: integration
+        for identifier, integration in iteritems(integrations)
+        if identifier in _installed_integrations
+    }
 
     for identifier in integrations:
         logger.debug("Enabling integration %s", identifier)

tests/conftest.py

@@ -30,7 +30,7 @@
 import sentry_sdk
 from sentry_sdk._compat import iteritems, reraise, string_types
 from sentry_sdk.envelope import Envelope
-from sentry_sdk.integrations import _installed_integrations  # noqa: F401
+from sentry_sdk.integrations import _processed_integrations  # noqa: F401
 from sentry_sdk.profiler import teardown_profiler
 from sentry_sdk.transport import Transport
 from sentry_sdk.utils import capture_internal_exceptions
@@ -187,8 +187,8 @@ def reset_integrations():
     with a clean slate to ensure monkeypatching works well,
     but this also means some other stuff will be monkeypatched twice.
     """
-    global _installed_integrations
-    _installed_integrations.clear()
+    global _processed_integrations
+    _processed_integrations.clear()
 
 
 @pytest.fixture

tests/test_basics.py

@@ -18,8 +18,13 @@
     Hub,
 )
 from sentry_sdk._compat import reraise
-from sentry_sdk.integrations import _AUTO_ENABLING_INTEGRATIONS
+from sentry_sdk.integrations import (
+    _AUTO_ENABLING_INTEGRATIONS,
+    Integration,
+    setup_integrations,
+)
 from sentry_sdk.integrations.logging import LoggingIntegration
+from sentry_sdk.integrations.redis import RedisIntegration
 from sentry_sdk.scope import (  # noqa: F401
     add_global_event_processor,
     global_event_processors,
@@ -28,6 +33,36 @@
 from sentry_sdk.tracing_utils import has_tracing_enabled
 
 
+def _redis_installed():  # type: () -> bool
+    """
+    Determines whether Redis is installed.
+    """
+    try:
+        import redis  # noqa: F401
+    except ImportError:
+        return False
+
+    return True
+
+
+class NoOpIntegration(Integration):
+    """
+    A simple no-op integration for testing purposes.
+    """
+
+    identifier = "noop"
+
+    @staticmethod
+    def setup_once():  # type: () -> None
+        pass
+
+    def __eq__(self, __value):  # type: (object) -> bool
+        """
+        All instances of NoOpIntegration should be considered equal to each other.
+        """
+        return type(__value) == type(self)
+
+
 def test_processors(sentry_init, capture_events):
     sentry_init()
     events = capture_events()
@@ -59,8 +94,8 @@ def test_auto_enabling_integrations_catches_import_error(sentry_init, caplog):
     sentry_init(auto_enabling_integrations=True, debug=True)
 
     for import_string in _AUTO_ENABLING_INTEGRATIONS:
-        # Ignore redis in the test case, because it is installed as a
-        # dependency for running tests, and therefore always enabled.
+        # Ignore redis in the test case, because it does not raise a DidNotEnable
+        # exception on import; rather, it raises the exception upon enabling.
         if _AUTO_ENABLING_INTEGRATIONS[redis_index] == import_string:
             continue
 
@@ -686,3 +721,18 @@ def test_functions_to_trace_with_class(sentry_init, capture_events):
     assert len(event["spans"]) == 2
     assert event["spans"][0]["description"] == "tests.test_basics.WorldGreeter.greet"
     assert event["spans"][1]["description"] == "tests.test_basics.WorldGreeter.greet"
+
+
+@pytest.mark.skipif(_redis_installed(), reason="skipping because redis is installed")
+def test_redis_disabled_when_not_installed(sentry_init):
+    sentry_init()
+
+    assert Hub.current.get_integration(RedisIntegration) is None
+
+
+def test_multiple_setup_integrations_calls():
+    first_call_return = setup_integrations([NoOpIntegration()], with_defaults=False)
+    assert first_call_return == {NoOpIntegration.identifier: NoOpIntegration()}
+
+    second_call_return = setup_integrations([NoOpIntegration()], with_defaults=False)
+    assert second_call_return == {NoOpIntegration.identifier: NoOpIntegration()}