fix(tests): collect tests/, and correct the two assertions that never ran - #314
Merged
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 |
antosubash
added a commit
that referenced
this pull request
Sep 5, 2026
… its own contract (#315) Two failures on main, neither of which any single PR's CI could have caught. **`provider.py` at 305 lines.** #309 and #314 were each green against the main they branched from; squash-merging both put the file over the 300-line cap. Split on the seam already there: `_resolve_bearer` moves to `token_strategy`, which is where `ExpiringDatabaseStrategy` lives. Those two are the only readers of `users_access_token` and they have to apply the same deadline and `session_version` rules — keeping them in one file is what stops them drifting. `provider.py` keeps a one-line delegate so the method stays on the provider's surface. 305 → 265, and `token_strategy` → 164. **`setup_pending_app` boots *with* an administrator.** `UsersModule.on_startup` seeds one from `SM_USERS_BOOTSTRAP_*`, read from the environment *and* from a `.env` on disk. A developer who followed `.env.example` has those set, so the fixture whose entire contract is "an app with no administrator" hands back an app that has two — the setup gate releases, the wizard routes 404, and eleven tests in `framework/hosting/tests` fail. CI has no `.env`, so it never saw this: the failure was local-only, which is the worst shape for a fixture to be wrong in. It also looked like test-ordering noise, because whether it reproduced depended on what else had booted an app first. The fixture now scrubs the bootstrap vars and stubs the dotenv reader for the app it builds, the same way `modules/users/tests/conftest.py` does for its own. Adding that pushed `fixtures.py` over the cap too, so the schema machinery (model imports, alembic heads, table creation) moves to `_schema.py` — that module declares fixtures, this one is what they stand on.
antosubash
added a commit
that referenced
this pull request
Sep 5, 2026
The second half of #295. Its first half — the two assertions that never ran — closed in #314; these are the units the branch shipped with no test at all, plus one assertion that could not fail. `scripts/gen_i18n.py` was the only script in `scripts/` with no test beside it. What it pins is the property #302 added: the command's exit code has to tell "wrote the key files" from "wrote nothing", and both halves of the `strict` flag — the boot path still preferring stale types to a failed start. `user_state` and `deriveState` are the same rule in two languages, because the edit page recomputes the status pill after a local change without a reload. Nothing held them together, so the first edit to either would have drifted them silently. They now share one six-row table, written out in both suites, and the Python side asserts the table covers every declared state. `PasswordInput`'s reveal toggle, `useLeaveGuard`, `Error.tsx`'s 403 branch and `file_storage`'s `SelectionFooter` and `showEmpty` had no tests. The two worth naming: - `useLeaveGuard` was stubbed out by both page suites (`router: { on: () => () => {} }`), so neither the prompt nor its escape hatch ran. The escape hatch is the load-bearing half: the page's own save is an Inertia visit too, and prompting on it would ask "discard your changes?" while saving them. - `showEmpty` keys on `pagination.total`, not `files.length`. A page past the last one renders an empty `files` array while the bucket is full, so the simpler condition would announce "No files yet" over a full bucket. Its copy also splits on whether a filter is active, because "No files yet" is wrong and discouraging when the filter is merely too narrow. `_clamp`'s out-of-range branch — zero, negative, `None`, a numeric string, and both bools, since `bool` is an `int` and `True` must not read as one second. And `test_remember_me.py`'s `if reissued is not None:` becomes an assertion. It let the test pass when the page wrote no session cookie at all, which is exactly the case where it has stopped exercising the regression. Closes #295
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.
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.