From 069b6e98429a78110fb269865e5bb0f6676916c8 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Mon, 13 Nov 2023 15:09:45 +0100 Subject: [PATCH 1/9] Add test to ensure redis integration disabled unless installed --- tests/test_api.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/test_api.py b/tests/test_api.py index 1adb9095f0..311a7105a6 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1,3 +1,5 @@ +import pytest + from sentry_sdk import ( configure_scope, continue_trace, @@ -7,12 +9,20 @@ start_transaction, ) from sentry_sdk.hub import Hub +from sentry_sdk.integrations.redis import RedisIntegration try: from unittest import mock # python 3.3 and above except ImportError: import mock # python < 3.3 +try: + import redis # noqa: F401 +except ImportError: + REDIS_INSTALLED = False +else: + REDIS_INSTALLED = True + def test_get_current_span(): fake_hub = mock.MagicMock() @@ -113,3 +123,10 @@ def test_continue_trace(sentry_init): assert propagation_context["dynamic_sampling_context"] == { "trace_id": "566e3688a61d4bc888951642d6f14a19" } + + +@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 From b039d84e62a388e6cb1484eb1cba92054ee7745a Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Mon, 13 Nov 2023 15:50:23 +0100 Subject: [PATCH 2/9] Integrations added to enabled list if actually installed --- sentry_sdk/integrations/__init__.py | 10 ++++++++-- tests/test_basics.py | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/sentry_sdk/integrations/__init__.py b/sentry_sdk/integrations/__init__.py index 0fe958d217..38af2e7bdb 100644 --- a/sentry_sdk/integrations/__init__.py +++ b/sentry_sdk/integrations/__init__.py @@ -144,8 +144,14 @@ def setup_integrations( logger.debug( "Did not enable default integration %s: %s", identifier, e ) - - _installed_integrations.add(identifier) + else: + _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) diff --git a/tests/test_basics.py b/tests/test_basics.py index b2b8846eb9..5a7be44a8e 100644 --- a/tests/test_basics.py +++ b/tests/test_basics.py @@ -59,8 +59,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 From 0f307bbdc5e9953be2d03bd9546964f00b621381 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Mon, 13 Nov 2023 18:50:55 +0100 Subject: [PATCH 3/9] Move test to test_basics.py --- tests/test_api.py | 17 ----------------- tests/test_basics.py | 20 ++++++++++++++++++++ 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 311a7105a6..1adb9095f0 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1,5 +1,3 @@ -import pytest - from sentry_sdk import ( configure_scope, continue_trace, @@ -9,20 +7,12 @@ start_transaction, ) from sentry_sdk.hub import Hub -from sentry_sdk.integrations.redis import RedisIntegration try: from unittest import mock # python 3.3 and above except ImportError: import mock # python < 3.3 -try: - import redis # noqa: F401 -except ImportError: - REDIS_INSTALLED = False -else: - REDIS_INSTALLED = True - def test_get_current_span(): fake_hub = mock.MagicMock() @@ -123,10 +113,3 @@ def test_continue_trace(sentry_init): assert propagation_context["dynamic_sampling_context"] == { "trace_id": "566e3688a61d4bc888951642d6f14a19" } - - -@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 diff --git a/tests/test_basics.py b/tests/test_basics.py index 5a7be44a8e..71e87b69a9 100644 --- a/tests/test_basics.py +++ b/tests/test_basics.py @@ -20,6 +20,7 @@ from sentry_sdk._compat import reraise from sentry_sdk.integrations import _AUTO_ENABLING_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 +29,18 @@ 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 + + def test_processors(sentry_init, capture_events): sentry_init() events = capture_events() @@ -686,3 +699,10 @@ 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 From b3a81a1499cabfd2478b095a62bdff2fe28b894c Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Wed, 15 Nov 2023 16:00:16 +0100 Subject: [PATCH 4/9] Code review suggestions --- sentry_sdk/integrations/__init__.py | 11 +++++++---- tests/conftest.py | 6 +++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/sentry_sdk/integrations/__init__.py b/sentry_sdk/integrations/__init__.py index 38af2e7bdb..99d8089b88 100644 --- a/sentry_sdk/integrations/__init__.py +++ b/sentry_sdk/integrations/__init__.py @@ -16,7 +16,7 @@ _installer_lock = Lock() -_installed_integrations = set() # type: Set[str] +_install_attempted_integrations = set() # type: Set[str] def _generate_default_integrations_iterator( @@ -119,9 +119,10 @@ def setup_integrations( integrations[instance.identifier] = instance used_as_default_integration.add(instance.identifier) + installed_integrations = set() # type: set[str] for identifier, integration in iteritems(integrations): with _installer_lock: - if identifier not in _installed_integrations: + if identifier not in _install_attempted_integrations: logger.debug( "Setting up previously not enabled integration %s", identifier ) @@ -145,12 +146,14 @@ def setup_integrations( "Did not enable default integration %s: %s", identifier, e ) else: - _installed_integrations.add(identifier) + installed_integrations.add(identifier) + + _install_attempted_integrations.add(identifier) integrations = { identifier: integration for identifier, integration in iteritems(integrations) - if identifier in _installed_integrations + if identifier in installed_integrations } for identifier in integrations: diff --git a/tests/conftest.py b/tests/conftest.py index d9d88067dc..d87a1f128e 100644 --- a/tests/conftest.py +++ b/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 _install_attempted_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 _install_attempted_integrations + _install_attempted_integrations.clear() @pytest.fixture From 76841a1293fdb0f50873079473d4d18bdc09cf99 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 17 Nov 2023 09:59:58 -0500 Subject: [PATCH 5/9] Fixed test failures --- sentry_sdk/integrations/__init__.py | 16 ++++++++++------ tests/conftest.py | 6 +++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/sentry_sdk/integrations/__init__.py b/sentry_sdk/integrations/__init__.py index 99d8089b88..21f7188ff1 100644 --- a/sentry_sdk/integrations/__init__.py +++ b/sentry_sdk/integrations/__init__.py @@ -16,7 +16,12 @@ _installer_lock = Lock() -_install_attempted_integrations = set() # type: Set[str] + +# 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] def _generate_default_integrations_iterator( @@ -119,10 +124,9 @@ def setup_integrations( integrations[instance.identifier] = instance used_as_default_integration.add(instance.identifier) - installed_integrations = set() # type: set[str] for identifier, integration in iteritems(integrations): with _installer_lock: - if identifier not in _install_attempted_integrations: + if identifier not in _processed_integrations: logger.debug( "Setting up previously not enabled integration %s", identifier ) @@ -146,14 +150,14 @@ def setup_integrations( "Did not enable default integration %s: %s", identifier, e ) else: - installed_integrations.add(identifier) + _installed_integrations.add(identifier) - _install_attempted_integrations.add(identifier) + _processed_integrations.add(identifier) integrations = { identifier: integration for identifier, integration in iteritems(integrations) - if identifier in installed_integrations + if identifier in _installed_integrations } for identifier in integrations: diff --git a/tests/conftest.py b/tests/conftest.py index d87a1f128e..5b0f1a8493 100644 --- a/tests/conftest.py +++ b/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 _install_attempted_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 _install_attempted_integrations - _install_attempted_integrations.clear() + global _processed_integrations + _processed_integrations.clear() @pytest.fixture From d9bd1fde093cbd065bd48f591881d9390da21f39 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 17 Nov 2023 10:22:04 -0500 Subject: [PATCH 6/9] Add unit test to check multiple `setup_integrations` calls --- tests/integrations/test_init.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 tests/integrations/test_init.py diff --git a/tests/integrations/test_init.py b/tests/integrations/test_init.py new file mode 100644 index 0000000000..7d2017aec8 --- /dev/null +++ b/tests/integrations/test_init.py @@ -0,0 +1,26 @@ +from sentry_sdk.integrations import Integration, setup_integrations + + +class NoOpIntegration(Integration): + """ + A simple no-op integration for testing purposes. + """ + + identifier = "noop" + + def setup_once(): # type: () -> None + pass + + def __eq__(self, __value: object) -> bool: + """ + All instances of NoOpIntegration should be considered equal to each other. + """ + return type(__value) == type(self) + + +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()} From b3dc172a917bcc702613deffea6cb3d7c10ecd91 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 17 Nov 2023 19:19:46 -0500 Subject: [PATCH 7/9] fix type hint for 2.7 --- tests/integrations/test_init.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integrations/test_init.py b/tests/integrations/test_init.py index 7d2017aec8..069bdc0b98 100644 --- a/tests/integrations/test_init.py +++ b/tests/integrations/test_init.py @@ -11,7 +11,7 @@ class NoOpIntegration(Integration): def setup_once(): # type: () -> None pass - def __eq__(self, __value: object) -> bool: + def __eq__(self, __value): # type: (object) -> bool """ All instances of NoOpIntegration should be considered equal to each other. """ From 1a63d0dc21bbb2d8533c59114c3e455406a59625 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 17 Nov 2023 19:33:35 -0500 Subject: [PATCH 8/9] Added staticmethod --- tests/integrations/test_init.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integrations/test_init.py b/tests/integrations/test_init.py index 069bdc0b98..330f52dec9 100644 --- a/tests/integrations/test_init.py +++ b/tests/integrations/test_init.py @@ -8,6 +8,7 @@ class NoOpIntegration(Integration): identifier = "noop" + @staticmethod def setup_once(): # type: () -> None pass From dc86efe688e7aac902dc5f88bad60b4e71192d51 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 17 Nov 2023 20:16:36 -0500 Subject: [PATCH 9/9] Move test to `test_basics` --- tests/integrations/test_init.py | 27 --------------------------- tests/test_basics.py | 32 +++++++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 28 deletions(-) delete mode 100644 tests/integrations/test_init.py diff --git a/tests/integrations/test_init.py b/tests/integrations/test_init.py deleted file mode 100644 index 330f52dec9..0000000000 --- a/tests/integrations/test_init.py +++ /dev/null @@ -1,27 +0,0 @@ -from sentry_sdk.integrations import Integration, setup_integrations - - -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_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()} diff --git a/tests/test_basics.py b/tests/test_basics.py index 71e87b69a9..2c2dcede3f 100644 --- a/tests/test_basics.py +++ b/tests/test_basics.py @@ -18,7 +18,11 @@ 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 @@ -41,6 +45,24 @@ def _redis_installed(): # type: () -> bool 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() @@ -706,3 +728,11 @@ 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()}