-
Notifications
You must be signed in to change notification settings - Fork 583
test: Assert setup_once() only raises DidNotEnable with shadowed dependencies
#5478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
527dd60
dcecb3d
e995101
de31a87
bd4c4ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ def pytest_generate_tests(metafunc): | |
| submodule_names = { | ||
| submodule_name | ||
| for _, submodule_name, _ in pkgutil.walk_packages(integrations.__path__) | ||
| } | ||
| } - {"beam", "spark", "unraisablehook"} | ||
|
|
||
| metafunc.parametrize( | ||
| "integration_submodule_name", | ||
|
|
@@ -79,18 +79,23 @@ def test_shadowed_modules_when_importing_integrations( | |
| An integration is determined to be for a third-party module if it cannot | ||
| be imported in the environment in which the tests run. | ||
| """ | ||
| module_path = f"sentry_sdk.integrations.{integration_submodule_name}" | ||
| parts = integration_submodule_name.split(".") | ||
| module_path = pathlib.Path("sentry_sdk") / "integrations" / pathlib.Path(*parts) | ||
| import_path = ".".join(("sentry_sdk", "integrations", *parts)) | ||
|
|
||
| integration_dependencies = set() | ||
| for py_file in pathlib.Path(module_path).rglob("*.py"): | ||
| source = py_file.read_text(encoding="utf-8") | ||
| tree = ast.parse(source, filename=str(py_file)) | ||
| integration_dependencies.update(find_unrecognized_dependencies(tree)) | ||
|
|
||
| mod = None | ||
| try: | ||
| # If importing the integration succeeds in the current environment, assume | ||
| # that the integration has no non-standard imports. | ||
| importlib.import_module(module_path) | ||
| return | ||
| except integrations.DidNotEnable: | ||
| spec = importlib.util.find_spec(module_path) | ||
| source = pathlib.Path(spec.origin).read_text(encoding="utf-8") | ||
| tree = ast.parse(source, filename=spec.origin) | ||
| integration_dependencies = find_unrecognized_dependencies(tree) | ||
| mod = importlib.import_module(import_path) | ||
|
|
||
| except integrations.DidNotEnable: | ||
| # For each non-standard import, create an empty shadow module to | ||
| # emulate an empty "agents.py" or analogous local module that | ||
| # shadows the package. | ||
|
|
@@ -101,7 +106,31 @@ def test_shadowed_modules_when_importing_integrations( | |
| # SDK catches the exception type when attempting to activate | ||
| # auto-enabling integrations. | ||
| with pytest.raises(integrations.DidNotEnable): | ||
| importlib.import_module(module_path) | ||
| importlib.import_module(import_path) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shadow modules leak if reimport assertion failsMedium Severity If the |
||
|
|
||
| for dependency in integration_dependencies: | ||
| del sys.modules[dependency] | ||
|
|
||
| # `setup_once()` can also raise when initializing the SDK with a shadowed module. | ||
| if mod is not None: | ||
| # For each non-standard import, create an empty shadow module to | ||
| # emulate an empty "agents.py" or analogous local module that | ||
| # shadows the package. | ||
| for dependency in integration_dependencies: | ||
| sys.modules[dependency] = types.ModuleType(dependency) | ||
|
|
||
| integration_types = [ | ||
| v | ||
| for v in mod.__dict__.values() | ||
| if isinstance(v, type) and issubclass(v, Integration) | ||
| ] | ||
alexander-alderman-webb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| for integration in integration_types: | ||
| # The `setup_once()` method should only raise DidNotEnable. | ||
| try: | ||
| integration.setup_once() | ||
| except integrations.DidNotEnable: | ||
| pass | ||
alexander-alderman-webb marked this conversation as resolved.
Show resolved
Hide resolved
alexander-alderman-webb marked this conversation as resolved.
Show resolved
Hide resolved
alexander-alderman-webb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| for dependency in integration_dependencies: | ||
| del sys.modules[dependency] | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path construction fails for single-file integrations
High Severity
The
module_pathis constructed without the.pyextension for single-file integrations likeaiohttp. Whenrglobis called onsentry_sdk/integrations/aiohttp(a non-existent directory since the actual file isaiohttp.py), it returns an empty iterator. This causesintegration_dependenciesto remain empty, making the test ineffective for single-file integrations—it doesn't actually test them with shadowed dependencies.