Fix Storage.path clobber at import time (#6061) - #157
Draft
evnchn wants to merge 2 commits into
Draft
Conversation
PR zauberzeug#5960 set `Storage.path = None` at module top of `general_fixtures.py` as the "not configured yet" sentinel. That assignment runs on plain `import nicegui.testing` (not just under pytest), so any production app whose import graph reaches the testing module — e.g. the docs site via `from nicegui.testing import Screen` in `screen_documentation.py` — has its `Storage.path` clobbered to `None`. The next `app.storage.user` access then crashes in `Storage._create_persistent_dict`: TypeError: unsupported operand type(s) for /: 'NoneType' and 'str' Fix: replace the overloaded `Storage.path is None` sentinel with a separate module-level `_storage_configured` boolean. `Storage.path` keeps its class-level default on plain import and is only assigned inside `pytest_configure`. Re-entry guard semantics are preserved for the case where `plugin.py` and `user_plugin.py` both load and both call `_general_pytest_configure`. Regression test added in `tests/test_lazy_imports.py`: subprocess imports `nicegui.testing` and asserts `Storage.path` is unchanged. Fails on the pre-fix code, passes after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the module-level boolean sentinel introduced in the previous commit with an `atexit.register(shutil.rmtree, ..., ignore_errors=True)` call inside `pytest_configure`. This matches the existing pattern for `DOWNLOAD_DIR` in `screen_plugin.py` and lets us drop `pytest_unconfigure` entirely (along with its three re-exports in plugin.py, user_plugin.py, screen_plugin.py). Trade-off vs the boolean approach: cleanup is deferred to process exit instead of pytest session end. For one-shot pytest invocations (CI, normal dev runs) the behavior is identical. For in-process repeat usage (IDE test re-runs, `pytest --looponfail`) leaked tempdirs accumulate in the system temp dir until process exit — small files, OS-GC'd eventually. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Fixes zauberzeug#6061. PR zauberzeug#5960 introduced a module-level
Storage.path = Noneassignment innicegui/testing/general_fixtures.pyintended as a "not configured yet" sentinel for pytest. But that line executes on plainimport nicegui.testing— not just under pytest — so any production app whose import graph reaches the testing module has itsStorage.pathclobbered toNone.Concretely, NiceGUI's own docs site does this:
website/documentation/content/screen_documentation.py:2doesfrom nicegui.testing import Screen, which transitively importsgeneral_fixtures, which setsStorage.path = None. The nextapp.storage.useraccess then crashes inStorage._create_persistent_dictatnicegui/storage.py:98:Empirical repro (pre-fix):
Implementation
Stop mutating
Storage.pathat module-level. The tempdir creation now happens only insidepytest_configure, and its cleanup is registered viaatexit.register(shutil.rmtree, ..., ignore_errors=True)— the same pattern already used inscreen_plugin.pyforDOWNLOAD_DIR. This lets us droppytest_unconfigureentirely (and its three re-exports inplugin.py,user_plugin.py,screen_plugin.py).Net change in
general_fixtures.py: +1 import, -1 module-level assignment, -1 hook function, -1 sentinel variable (after design iteration; see below).A regression test is added in
tests/test_lazy_imports.py(subprocess pattern matches existingtest_module_access_does_not_import_others):Verified the test fails on pre-fix code (stash/unstash) and passes after — not vacuous.
Design discussion: why atexit instead of a `_storage_configured` boolean + `pytest_unconfigure`?
The first commit on this branch used a separate module-level
_storage_configured: boolas the sentinel and kept a symmetricpytest_unconfigurehook for cleanup. The second commit replaces that withatexit.registerand drops the hook.Alternatives considered for replacing the broken
Storage.path is Nonesentinel:_storage_configuredboolean — clean, but adds a new mutable module-level variable._INITIAL_STORAGE_PATH = Storage.pathat import, compare inpytest_configure— still a new variable, and fragile if a user has setStorage.paththemselves before pytest runs.Storage— leaks pytest concerns into production class. Rejected.pytest.StashKey— proper pytest idiom but still introduces a module-levelStashKeyinstance, same effective scope as a bool.pytest_configure._called) — poor discoverability. Rejected.atexit.registerfor cleanup — zero new module-level variables, matches existingscreen_plugin.pypattern. Chosen.Trade-off vs the boolean +
pytest_unconfigureapproach:pytestinvocations (CI, normal dev runs): identical behavior.pytest --looponfail, somepytest-xdistsetups): cleanup is deferred to process exit instead of pytest session end. Leaked tempdirs accumulate in the system temp dir until the Python process dies. The directories are small (empty or a few JSON files) and OS-GC'd eventually.Why the dual-plugin re-entry guard wasn't necessary:
The previous "is already configured" check guarded against
pytest_plugins = ['nicegui.testing.plugin', 'nicegui.testing.user_plugin']both being loaded, which would call_general_pytest_configuretwice. NiceGUI's owntests/conftest.pyloads onlynicegui.testing.plugin(which already re-exports user-plugin fixtures), and the dual-load is redundant by design — it would cause fixture redefinition warnings. Without the guard, the worst case is one extratempfile.mkdtempcall whose result is immediately shadowed; the orphaned dir is still rmtree'd at exit via its ownatexitregistration.I'm happy to revert to the boolean approach if you prefer the deterministic per-session cleanup.
Validation
pytest tests/test_storage.py tests/test_lazy_imports.py tests/test_main_file_marker.py tests/test_user_simulation.py→ 102 passed / 2 xfailed (pre-existing).ruff check,pylint,mypyon changed files → clean.Progress
tests/test_lazy_imports.py).Draft against
evnchn/nicegui:mainfor review/finetune before upstreaming tozauberzeug/nicegui. Two commits are currently on the branch; squash on merge if preferred.