Make the unit test suite deterministic under ambient configuration - #441
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… operator notice Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughThe PR isolates unit tests from ambient configuration, adds clean/adverse CI coverage, documents configuration-testing rules, protects the shared config singleton across reloads, and makes live address integration assertions robust to reordered notes. ChangesAmbient Configuration Isolation
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- tests/integration/address/test_get_address_info_real.py: result.notes is None on the fully-healthy path (notes=notes or None in get_address_info, preserved by build_tool_response), so iterating it directly raised TypeError in a clean environment (no ambient operator notice). Iterate a `result.notes or []` local instead. Verified live: the old code fails and the fixed code passes with BLOCKSCOUT_PRO_API_KEY_REQUIRED_NOTICE empty, and still passes with the notice set. - tests/conftest.py: pydantic-settings matches environment variables case-insensitively, so a lowercase `blockscout_...` (or `port`) variable leaked into the pristine instance and got pinned onto the singleton. Delete ambient variables by case-insensitive comparison. - tests/test_pristine_config_fixture.py: make the env-channel contract test case-insensitive to enforce the above. - .github/workflows/ci.yml: export a lowercase `blockscout_bs_timeout` in the adverse leg so a case-sensitivity regression fails CI (verified load-bearing: the pre-fix fixture fails the contract module under it). - tests/test_server.py: fix the docstring pointer to the identity contract test (it lives in tests/test_pristine_config_fixture.py, not conftest). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…broader adverse leg
- tests/conftest.py: detect the integration exemption via
request.node.get_closest_marker("integration") instead of request.keywords,
which also matches node *names*, so a test literally named "integration"
would silently escape pinning.
- Docstrings: drop references to "Phase 1/Phase 2 of the issue #436
implementation plan" (not committed to the repo) in favor of naming the
actual artifacts (pristine_config in tests/conftest.py,
_isolate_dotenv_and_singleton in tests/test_server.py).
- tests/test_pristine_config_fixture.py: compare spot-checked fields against
ServerConfig.model_fields[...].default instead of hardcoded literals, so a
legitimate default change doesn't read as a fixture regression; document the
collection-order caveat of the singleton-identity canary.
- CI adverse leg: the hostile .env now sets every ServerConfig field to a
valid non-default value (was: 2 fields), and the exported environment also
exercises the unprefixed PORT alias channel.
Verified: 977 passed / 92 deselected, identical verdict clean vs. the exact
hostile .env + exported variables from the adverse CI step; lint and format
checks clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ementation-cc53e9 # Conflicts: # AGENTS.md # tests/conftest.py
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Closes #436.
The unit suite's verdict depended on the machine it ran on. A developer with a populated
.env(or an exportedBLOCKSCOUT_*variable) saw failures that CI never reproduced, becauseServerConfigpicked up ambient operator settings — most visiblyBLOCKSCOUT_PRO_API_KEY_REQUIRED_NOTICE, which prepends a note to tool responses and broke assertions in 34 test files.This PR pins the configuration unit tests see, and adds a CI leg that would catch a regression.
What changed
Autouse
pristine_configfixture (tests/conftest.py) — every unit test now runs against aServerConfigconstructed with_env_file=Noneand the ambientBLOCKSCOUT_*environment closed out, pinned overmodel_fields. Integration tests are deliberately exempt: they need the real environment. A new contract module,tests/test_pristine_config_fixture.py, covers the fixture's six prescribed scenarios..envisolation for the reload tests (tests/test_server.py) — these tests reconstruct the config singleton, so the fixture alone can't protect them. A function-scoped autouse fixture chdirs to atmp_path(no.envto find) and a_restore_canonical_confighelper repairs singleton identity on teardown.CI ambient-state matrix (
.github/workflows/ci.yml) — the unit job gains anambient: ["clean", "adverse"]dimension. The adverse leg writes a hostile.envthat sets everyServerConfigfield to a valid non-default value and exports adverse variables (including a lowercase spelling and the unprefixedPORTalias) before running the full suite, so ambient-config leakage fails CI instead of only failing on someone's laptop.Documentation —
.cursor/rules/200-development-testing-workflow.mdcgains an "Ambient Configuration Is Pinned" section;.cursor/AGENTS.mdnarrows the rule-210 trigger totests/tools/;AGENTS.mdrecords the new test files.Notes for the reviewer
One pre-existing integration failure was fixed here (
4e16df3).test_get_address_info_integrationfailed on unmodifiedmaintoo, from the same root cause in a place the fixture deliberately does not reach: the operator notice landed atresult.notes[0]and defeated a positional assertion. The fix is a strengthening, not a loosening — the oldif result.notes:gate meant the full assertion block only ran whennoteswas empty, whereas it now runs unless one of the two specific failure notes is present. Skip semantics for the genuine failure modes are unchanged.CI job rename. The unit job is now named
Run Unit Tests (clean)/Run Unit Tests (adverse)instead ofRun Unit Tests. Branch protection formaincurrently defines no required status checks, so the rename does not affect merge requirements. If required checks are ever configured, use the new job names.No version bump — no functional or server change ships in this PR.
Verification
pytest→ 977 passed, 92 deselected, and the identical verdict under an exportedBLOCKSCOUT_PRO_API_KEY_REQUIRED_NOTICEand under a hostile.envat the repo root. That determinism is the point of the change.pristine_configfixture reintroduces 36–37 failures, so it is load-bearing rather than vacuously passing. No tests were skipped, xfailed, deleted, or loosened.ruff check .andruff format --check .clean across 215 files.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
.envfiles.Tests