fix(tests): collect tests/, and correct the two assertions that never ran - #314
Merged
Conversation
… ran The issue calls these order-dependent — passing under `pytest`, failing under `pytest <file>`. They are not. `testpaths` listed `tests/integration`, `tests/e2e`, `tests/benchmarks` and `tests/perf` but never `tests/` itself, so the four modules directly under it were not collected by a bare `pytest` at all. The tests "passed" by not running. 26 tests were in that gap, including a lifespan hydration test and fifteen audit-log tests. Replacing the four entries with `"tests"` collects them; the marker filter in `addopts` still holds e2e and perf out, so nothing new runs in CI that should not. That leaves exactly the two failures the issue describes, and both are the test being wrong rather than the code. `test_host_settings_ignores_env` asserted "HostSettings must NOT read env". That is not this codebase's contract: precedence is env → DB → default and env has to keep winning, or an upgrade silently changes a deployment's behaviour. `HostSettings` declares `env_prefix="SM_"` for that reason. Rewritten as three tests covering what the prefix actually guarantees — prefixed env wins, an unprefixed name is ignored, the default applies when neither is set. `test_session_wins_over_bad_bearer` asserted a valid session rescues a bad `Authorization: Bearer`. `resolve_user` does the opposite. Keeping the code is the deliberate call: falling through would make an invalid token indistinguishable from no token, so a client whose credential expired keeps working on whatever other identity it carries and its 401s depend on what else is in the request — while gaining nothing, since the fall-through can only resolve the session's own identity, which the caller already had. The precedence is now stated where it is implemented, with a sibling test proving the session alone still succeeds so the 401 is the header's doing. Closes #295
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Deploying simple-module-python with
|
| Latest commit: |
dad8de2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://12541d38.simple-module-python.pages.dev |
| Branch Preview URL: | https://fix-order-dependent-tests.simple-module-python.pages.dev |
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
1 participant
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.
Partially addresses #295 — the order-dependence half. See "Still open" below.
The diagnosis in the issue is wrong, and the truth is worse
The issue describes two tests as order-dependent: green under
pytest, red underpytest <file>. They are not order-dependent.testpathslistedbut never
tests/itself, so the four modules sitting directly under it were never collected by a barepytest. They "passed" by not running. 26 tests were in that gap, includingtest_hydration_lifespan.pyand all fifteen oftests/test_audit_log.py.Replacing the four entries with
"tests"collects them. The marker filter already inaddopts(-m 'not e2e and not perf') still holds e2e and perf out, so nothing new runs in CI that shouldn't. Collection goes 2787 → 2813, and that leaves exactly the two failures the issue names.Both are the test being wrong, not the code
test_host_settings_ignores_envasserted "HostSettings must NOT read env — env-sprawl is what we're removing." That is not this codebase's contract. CLAUDE.md § Conventions: "Precedence is always env → DB → default; env must keep winning or existing deployments change behaviour silently on upgrade." AndHostSettingsdeclaresenv_prefix="SM_"with a comment explaining that a bareHostSettings()would otherwise read unprefixed names. Rewritten as three tests covering what the prefix actually guarantees: prefixed env wins, an unprefixed name is ignored, the default applies when neither is set.test_session_wins_over_bad_bearerasserted a valid session cookie rescues a badAuthorization: Bearer.UsersAuthProvider.resolve_userdoes the opposite — the header is checked first and a bad token returnsNone.The precedence is now stated where it's implemented, with a sibling test proving the session alone still returns 200 so the 401 is demonstrably the header's doing and not a broken fixture.
Verification
uv run pytest -q— 2814 passed, 2 skipped, 60 deselected (was 2787 collected; +27)ruff format --check ./ruff check ./ty check framework modules host/check_file_size.py— all passStill open on #295
The issue's second half — the missing tests list (
gen_i18n.py,PasswordInput,Error.tsx's 403 branch,useLeaveGuard,deriveState/user_stateparity,file_storagepages,_clamp's out-of-range branch, and the softif reissued is not None:guard intest_remember_me.py) — is not in this PR. Several of those files are touched by #312 and #313, which are in flight; doing them here would have collided. They should follow once those land, and I'd keep #295 open until then.