test: add unit tests for get_current_user (superset/tasks/utils.py)#40878
test: add unit tests for get_current_user (superset/tasks/utils.py)#40878AndreMeyerr wants to merge 6 commits into
Conversation
Code Review Agent Run #8cd5d7Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| _stub("celery") | ||
| _stub("celery.utils") | ||
| _stub("celery.utils.log", get_task_logger=lambda n: MagicMock()) | ||
| _stub("superset_core") | ||
| _stub("superset_core.tasks") | ||
| _stub("superset_core.tasks.types", TaskProperties=dict, TaskScope=MagicMock()) | ||
| _stub( | ||
| "superset.tasks.exceptions", | ||
| ExecutorNotFoundError=Exception, | ||
| InvalidExecutorError=Exception, | ||
| ) | ||
| _stub( | ||
| "superset.tasks.types", | ||
| ChosenExecutor=MagicMock(), | ||
| Executor=MagicMock(), | ||
| ExecutorType=MagicMock(), | ||
| FixedExecutor=MagicMock(), | ||
| ) | ||
| _stub("superset.utils") | ||
| _stub( | ||
| "superset.utils.json", | ||
| loads=MagicMock(), | ||
| dumps=MagicMock(), | ||
| JSONDecodeError=ValueError, | ||
| ) | ||
| _stub("superset.utils.hashing", hash_from_str=MagicMock(return_value="abc" * 30)) | ||
| _stub("superset.utils.urls", get_url_path=MagicMock()) | ||
|
|
||
| _path = pathlib.Path(__file__).parents[3] / "superset" / "tasks" / "utils.py" | ||
| _spec = importlib.util.spec_from_file_location("superset.tasks.utils", _path) | ||
| assert _spec is not None | ||
| assert _spec.loader is not None | ||
| _mod = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(_mod) | ||
|
|
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The test module mutates global sys.modules at import time with broad stubs (e.g. superset.utils, superset.utils.hashing, superset.tasks.types, superset_core) via _stub(...) and never restores them, so these fake modules replace the real packages for all later tests in the same process. Given pytest's deterministic collection order (tests/unit_tests/tasks/test_get_current_user.py is imported before tests/unit_tests/utils/test_hashing.py and other tests that import these modules), this injects cross-test contamination and causes order-dependent failures when other tests expect the real implementations.
Suggestion: Avoid permanent sys.modules mutation by either importing superset.tasks.utils directly as in tests/unit_tests/tasks/test_utils.py:1-14 (so normal imports resolve real dependencies), or, if module-level stubbing is required, follow the existing pattern used in tests/unit_tests/mcp_service/sql_lab/tool/test_open_sql_lab_with_context.py:15-55 and tests/unit_tests/mcp_service/sql_lab/tool/test_save_sql_query.py:8-50 to save original sys.modules entries and restore them after each import.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** tests/unit_tests/tasks/test_get_current_user.py
**Line:** 42:76
**Comment:**
*HIGH: The test module mutates global `sys.modules` at import time with broad stubs (e.g. `superset.utils`, `superset.utils.hashing`, `superset.tasks.types`, `superset_core`) via `_stub(...)` and never restores them, so these fake modules replace the real packages for all later tests in the same process. Given pytest's deterministic collection order (`tests/unit_tests/tasks/test_get_current_user.py` is imported before `tests/unit_tests/utils/test_hashing.py` and other tests that import these modules), this injects cross-test contamination and causes order-dependent failures when other tests expect the real implementations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The architectural review finding is correct. The current implementation of Below is a minimal fix using a context manager to ensure import sys
from contextlib import contextmanager
@contextmanager
def temporary_stubs(stubs):
original = {name: sys.modules.get(name) for name in stubs}
try:
for name, mod in stubs.items():
sys.modules[name] = mod
yield
finally:
for name, original_mod in original.items():
if original_mod is None:
sys.modules.pop(name, None)
else:
sys.modules[name] = original_mod
# Usage:
# stubs = {"celery": _stub("celery"), ...}
# with temporary_stubs(stubs):
# _spec.loader.exec_module(_mod)Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? tests/unit_tests/tasks/test_get_current_user.py |
| _stub("superset_core") | ||
| _stub("superset_core.tasks") | ||
| _stub("superset_core.tasks.types", TaskProperties=dict, TaskScope=MagicMock()) | ||
| _stub( | ||
| "superset.tasks.exceptions", | ||
| ExecutorNotFoundError=Exception, | ||
| InvalidExecutorError=Exception, | ||
| ) | ||
| _stub( | ||
| "superset.tasks.types", | ||
| ChosenExecutor=MagicMock(), | ||
| Executor=MagicMock(), | ||
| ExecutorType=MagicMock(), | ||
| FixedExecutor=MagicMock(), | ||
| ) | ||
| _stub("superset.utils") | ||
| _stub( | ||
| "superset.utils.json", | ||
| loads=MagicMock(), | ||
| dumps=MagicMock(), | ||
| JSONDecodeError=ValueError, | ||
| ) | ||
| _stub("superset.utils.hashing", hash_from_str=MagicMock(return_value="abc" * 30)) | ||
| _stub("superset.utils.urls", get_url_path=MagicMock()) | ||
|
|
||
| _path = pathlib.Path(__file__).parents[3] / "superset" / "tasks" / "utils.py" | ||
| _spec = importlib.util.spec_from_file_location("superset.tasks.utils", _path) | ||
| assert _spec is not None | ||
| assert _spec.loader is not None | ||
| _mod = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(_mod) |
There was a problem hiding this comment.
Suggestion: This test bootstraps superset.tasks.utils via exec_module after stubbing only deep submodules, but it does not stub the parent superset package chain. During from superset.tasks.exceptions import ..., Python can still import the real superset package (triggering heavy app/bootstrap imports from superset/__init__.py), which makes this unit test fail in minimal environments and introduces import-order fragility. Stub the parent packages (superset, superset.tasks) or load the target module under an isolated module name with a fully patched module graph. [import error]
Severity Level: Major ⚠️
- CRIT: Unit tests import full Superset package unnecessarily.
- WARN: Test requires Celery and SQLAlchemy just to import.
- WARN: Harder to run test in minimal environments.Steps of Reproduction ✅
1. Import `tests/unit_tests/tasks/test_get_current_user.py`, which executes the `_stub`
and import bootstrapping logic at lines 42–75 (notably `_stub("superset.tasks.exceptions",
...)` at lines 48–52 and `_spec.loader.exec_module(_mod)` at line 75).
2. During `_spec.loader.exec_module(_mod)`, the real module `superset/tasks/utils.py` is
executed under the name `"superset.tasks.utils"` (see `superset/tasks/utils.py` lines
27–40 for its top-level imports).
3. While executing `superset/tasks/utils.py`, the `from superset.tasks.exceptions import
ExecutorNotFoundError, InvalidExecutorError` statement at line 31 triggers Python's import
machinery to load the real `superset` package hierarchy, executing `superset/__init__.py`
lines 17–31, which in turn imports `superset.app` and `superset.extensions`.
4. Importing `superset.extensions` at `superset/extensions/__init__.py` lines 17–61 and
139–160 runs significant side-effectful initialization (Celery app, SQLAlchemy `db`, cache
manager, feature flags, etc.), meaning every run of this unit test implicitly bootstraps
the full Superset package instead of remaining isolated to `superset.tasks.utils`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/tasks/test_get_current_user.py
**Line:** 45:75
**Comment:**
*Import Error: This test bootstraps `superset.tasks.utils` via `exec_module` after stubbing only deep submodules, but it does not stub the parent `superset` package chain. During `from superset.tasks.exceptions import ...`, Python can still import the real `superset` package (triggering heavy app/bootstrap imports from `superset/__init__.py`), which makes this unit test fail in minimal environments and introduces import-order fragility. Stub the parent packages (`superset`, `superset.tasks`) or load the target module under an isolated module name with a fully patched module graph.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Pull request overview
Adds a dedicated unit-test module covering superset.tasks.utils.get_current_user, which is used to resolve the executor identity for async task execution.
Changes:
- Introduces
tests/unit_tests/tasks/test_get_current_user.pywith 7 test cases covering missing user, anonymous user, authenticated user, and username boundary values. - Uses mocking/patching of
gto exercise the function’s branches.
| def _stub(name, **attrs): | ||
| mod = types.ModuleType(name) | ||
| mod.__dict__.update(attrs) | ||
| sys.modules.setdefault(name, mod) | ||
| return mod |
| """Testes unitários para get_current_user (superset/tasks/utils.py). | ||
|
|
||
| Técnicas aplicadas: | ||
| - Caixa-preta : particionamento de equivalência (CV1, CI1-CI3) | ||
| - Caixa-branca: cobertura de branch + MC/DC (D1, D2) | ||
| - Isolamento : substituição de g via Flask app_context + patch direto | ||
| """ |
| class TestGetCurrentUser: | ||
| def test_retorna_none_quando_g_nao_possui_user(self, app): | ||
| """CT01 | CI1 | D1=F (hasattr=False, curto-circuito).""" | ||
| mock_g = MagicMock(spec=[]) # spec=[] → hasattr(g, "user") == False | ||
| with app.app_context(): |
| """CT05 | CI2 ampliada | D1=F (g.user = 0, falsy mas não None). | ||
|
|
||
| Verifica que qualquer valor falsy em g.user (não apenas None) encerra | ||
| o fluxo antes de avaliar is_anonymous, retornando None. | ||
| """ |
| """CT06 | CV1 borda | D1=V; D2=V; username = "". | ||
|
|
||
| Comportamento não especificado formalmente — a função retorna string | ||
| vazia em vez de None quando user.username == "". | ||
| """ |
| """CT07 | CV1 borda | D1=V; D2=V; username = None. | ||
|
|
||
| username=None retorna None via user.username, não pelo fluxo principal | ||
| (ou seja, a guarda `not user.is_anonymous` é satisfeita, mas o valor | ||
| retornado é None porque user.username é None). |
| class TestGetCurrentUser: | ||
| def test_retorna_none_quando_g_nao_possui_user(self, app): | ||
| """CT01 | CI1 | D1=F (hasattr=False, curto-circuito).""" | ||
| mock_g = MagicMock(spec=[]) # spec=[] → hasattr(g, "user") == False |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40878 +/- ##
=======================================
Coverage 68.30% 68.30%
=======================================
Files 1865 1865
Lines 77997 77997
Branches 25032 25032
=======================================
Hits 53274 53274
Misses 24723 24723
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Adds dedicated unit tests for
get_current_userinsuperset/tasks/utils.py.This function had no prior unit test coverage in the repository.
Motivation
get_current_useris used to resolve the executor of scheduled async tasks(e.g. thumbnail generation via
get_executor). Despite its role in taskexecution, it had no dedicated unit tests — only integration-level references
where it was mocked as a dependency of other tests.
Changes
tests/unit_tests/tasks/get_current_user.pywith 7 test casesimportlib+patch.objectongto avoidrequiring a full Superset application context
Test cases
g.userattribute absentNoneg.user = None(falsy)Noneis_anonymous=True)Noneusernameg.user = 0(non-None falsy)Noneusername = ""(boundary)""username = None(boundary)NoneExecution