fix(tests): consolidate NATS fixtures (3 runtime tests rewired) - #1201
Merged
Merged
Conversation
Adds three shared fixtures to pmoves/tests/conftest.py so the
runtime NATS tests stop hardcoding unauthenticated URLs and stop
invoking nats.connect() as a (non-existent) classmethod:
- nats_url: session-scoped, defaults to the canonical
nats://nats:pmoves@nats:4222 and honours NATS_URL overrides.
- nats_available: cheap socket probe on 127.0.0.1:4222 so
tests can skip cleanly when the broker is not running.
- nats_client: async fixture that bypasses the autouse
_FakeNATS stub (by popping nats modules from sys.modules
before importlib.import_module), connects with the real
nats-py client, and tears the connection down on exit.
Rewires the three tests that actually open a NATS connection:
- tests/functional/test_a2ui_bridge_integration.py::
test_nats_stream_exists and test_nats_a2ui_subjects now take
the nats_client fixture and drop their ImportError /
ConnectionRefusedError wrappers (skip handled by fixture).
- tests/functional/test_tokenism_simulator.py::
TestTokenismSimulatorIntegrations::test_nats_connection is
converted to async, takes nats_client, and removes the local
nats_url fixture whose default URL was unauthenticated. The
previous body called nats.connect(url) as if it were a
classmethod, which is not part of the nats-py API and
always raised.
Widens the _FakeNATS stub in stub_external_modules so its
connect() coroutine returns self with is_connected=True, which
prevents silent AttributeError crashes in unit tests that
accidentally import the stub.
No NATS service code, docker-compose, port_conflicts tests,
service_contracts tests, docker_hardening tests, or encoding
tests were touched.
Note: the task brief mentioned ~8-9 failing NATS tests, but
only 3 actually open a NATS socket at runtime. The others
(smoke/test_nats_configuration.py, smoke/test_nats_authentication.py,
test_clawz_field.py, a2ui/test_bridge.py, etc.) are static env
or doc grep checks that already skip or do not open connections.
Verification:
cd pmoves && python -m pytest \
tests/functional/test_a2ui_bridge_integration.py \
tests/functional/test_tokenism_simulator.py \
--collect-only -q
-> clean collection (19 tests)
cd pmoves && python -m pytest \
tests/functional/test_a2ui_bridge_integration.py::test_nats_stream_exists \
tests/functional/test_a2ui_bridge_integration.py::test_nats_a2ui_subjects \
"tests/functional/test_tokenism_simulator.py::TestTokenismSimulatorIntegrations::test_nats_connection"
-> 3 skipped (broker not running locally), 0 failed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe changes introduce a centralized, reusable NATS client fixture with environment configuration and availability detection, replacing manual connection logic across multiple integration tests. The stub client is updated to properly reflect a connected state. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Code
participant Fixture as nats_client Fixture
participant SysModules as sys.modules
participant NATS as NATS Broker
participant Client as NATS Client
Test->>Fixture: Request nats_client
Fixture->>Fixture: Check nats_available (TCP probe 127.0.0.1:4222)
alt Broker Unreachable
Fixture->>Test: Skip test
else Broker Reachable
Fixture->>SysModules: Remove stubbed nats modules
Fixture->>Client: Import real nats package
Fixture->>NATS: Connect with nats_url
NATS->>Client: Connection established
Client->>Fixture: Return connected client
Fixture->>Test: Provide nats_client
Test->>Client: Use connected client (e.g., jetstream())
Client->>NATS: Execute JetStream operations
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pmoves/tests/conftest.py (1)
185-186: Add a return type annotation to the async fixture signature.Consider annotating
nats_client(for example, as-> AsyncIterator[object]) to improve editor/static-analysis clarity.As per coding guidelines
pmoves/**/*.py: Use 4-space indentation and prefer type hints in Python 3.11+.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/conftest.py` around lines 185 - 186, Add an explicit return type annotation to the async fixture function nats_client (e.g., change its signature to include -> AsyncIterator[object]) and import the AsyncIterator type (from typing or collections.abc depending on project conventions) so static analysis and editors can infer the fixture's async iterator behavior; update the function signature in the nats_client definition and ensure necessary import (AsyncIterator) is present at the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pmoves/tests/conftest.py`:
- Around line 198-211: The fixture mutates sys.modules by popping ("nats",
"nats.aio", "nats.aio.client") and never restores them; record the original
entries before the loop (e.g., save a dict mapping mod_name -> original_module),
then after the yield/cleanup (in the same finally block that closes nc) restore
sys.modules to its prior state by reinserting saved modules or removing newly
inserted keys so the environment is unchanged for later tests; update the logic
around the loop that uses mod_name and the teardown that references nc/nats_real
to perform this restore.
- Around line 175-182: The nats_available fixture currently always probes
127.0.0.1:4222 which ignores any override via the NATS_URL environment/config;
update the nats_available fixture to read the NATS_URL (fallback to
"nats://127.0.0.1:4222"), parse it (e.g., with urlparse) to extract host and
port, and use those values in socket.create_connection instead of the hard-coded
address so the availability check respects configured NATS endpoints referenced
by NATS_URL.
---
Nitpick comments:
In `@pmoves/tests/conftest.py`:
- Around line 185-186: Add an explicit return type annotation to the async
fixture function nats_client (e.g., change its signature to include ->
AsyncIterator[object]) and import the AsyncIterator type (from typing or
collections.abc depending on project conventions) so static analysis and editors
can infer the fixture's async iterator behavior; update the function signature
in the nats_client definition and ensure necessary import (AsyncIterator) is
present at the top of the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94312b44-f7db-4abb-8dc1-50ce10ad7ecb
📒 Files selected for processing (3)
pmoves/tests/conftest.pypmoves/tests/functional/test_a2ui_bridge_integration.pypmoves/tests/functional/test_tokenism_simulator.py
Comment on lines
+175
to
+182
| @pytest.fixture(scope="session") | ||
| def nats_available() -> bool: | ||
| """Return True when a NATS broker is reachable on localhost:4222.""" | ||
| try: | ||
| with socket.create_connection(("127.0.0.1", 4222), timeout=1.0): | ||
| return True | ||
| except OSError: | ||
| return False |
Contributor
There was a problem hiding this comment.
Availability check ignores NATS_URL and can skip valid test environments.
nats_available always probes 127.0.0.1:4222, but Line 172 allows overriding NATS_URL. If CI/dev points to another host/port, tests will be skipped even when NATS is reachable.
💡 Proposed fix
+from urllib.parse import urlparse
...
-@pytest.fixture(scope="session")
-def nats_available() -> bool:
- """Return True when a NATS broker is reachable on localhost:4222."""
+@pytest.fixture(scope="session")
+def nats_available(nats_url: str) -> bool:
+ """Return True when the configured NATS broker is reachable."""
+ parsed = urlparse(nats_url)
+ host = parsed.hostname or "127.0.0.1"
+ port = parsed.port or 4222
try:
- with socket.create_connection(("127.0.0.1", 4222), timeout=1.0):
+ with socket.create_connection((host, port), timeout=1.0):
return True
except OSError:
return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/conftest.py` around lines 175 - 182, The nats_available fixture
currently always probes 127.0.0.1:4222 which ignores any override via the
NATS_URL environment/config; update the nats_available fixture to read the
NATS_URL (fallback to "nats://127.0.0.1:4222"), parse it (e.g., with urlparse)
to extract host and port, and use those values in socket.create_connection
instead of the hard-coded address so the availability check respects configured
NATS endpoints referenced by NATS_URL.
Comment on lines
+198
to
+211
| for mod_name in ("nats", "nats.aio", "nats.aio.client"): | ||
| sys.modules.pop(mod_name, None) | ||
| try: | ||
| nats_real = importlib.import_module("nats") | ||
| except ImportError: | ||
| pytest.skip("nats-py not installed") | ||
| nc = await nats_real.connect(nats_url) | ||
| try: | ||
| yield nc | ||
| finally: | ||
| try: | ||
| await nc.close() | ||
| except Exception: | ||
| pass |
Contributor
There was a problem hiding this comment.
sys.modules mutation is not reverted, which can leak state across tests.
This fixture removes stub modules globally and leaves the real nats modules loaded after completion. Later tests in the same session may run against an unintended module set.
💡 Proposed fix
`@pytest_asyncio.fixture`(scope="function")
async def nats_client(nats_url: str, nats_available: bool):
@@
- for mod_name in ("nats", "nats.aio", "nats.aio.client"):
+ mod_names = ("nats", "nats.aio", "nats.aio.client")
+ previous_modules = {name: sys.modules.get(name) for name in mod_names}
+ for mod_name in mod_names:
sys.modules.pop(mod_name, None)
@@
finally:
try:
await nc.close()
except Exception:
pass
+ finally:
+ for name, previous in previous_modules.items():
+ if previous is None:
+ sys.modules.pop(name, None)
+ else:
+ sys.modules[name] = previous📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for mod_name in ("nats", "nats.aio", "nats.aio.client"): | |
| sys.modules.pop(mod_name, None) | |
| try: | |
| nats_real = importlib.import_module("nats") | |
| except ImportError: | |
| pytest.skip("nats-py not installed") | |
| nc = await nats_real.connect(nats_url) | |
| try: | |
| yield nc | |
| finally: | |
| try: | |
| await nc.close() | |
| except Exception: | |
| pass | |
| mod_names = ("nats", "nats.aio", "nats.aio.client") | |
| previous_modules = {name: sys.modules.get(name) for name in mod_names} | |
| for mod_name in mod_names: | |
| sys.modules.pop(mod_name, None) | |
| try: | |
| nats_real = importlib.import_module("nats") | |
| except ImportError: | |
| pytest.skip("nats-py not installed") | |
| nc = await nats_real.connect(nats_url) | |
| try: | |
| yield nc | |
| finally: | |
| try: | |
| await nc.close() | |
| except Exception: | |
| pass | |
| finally: | |
| for name, previous in previous_modules.items(): | |
| if previous is None: | |
| sys.modules.pop(name, None) | |
| else: | |
| sys.modules[name] = previous |
🧰 Tools
🪛 Ruff (0.15.9)
[error] 210-211: try-except-pass detected, consider logging the exception
(S110)
[warning] 210-210: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/conftest.py` around lines 198 - 211, The fixture mutates
sys.modules by popping ("nats", "nats.aio", "nats.aio.client") and never
restores them; record the original entries before the loop (e.g., save a dict
mapping mod_name -> original_module), then after the yield/cleanup (in the same
finally block that closes nc) restore sys.modules to its prior state by
reinserting saved modules or removing newly inserted keys so the environment is
unchanged for later tests; update the logic around the loop that uses mod_name
and the teardown that references nc/nats_real to perform this restore.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0d3ff202c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| def nats_available() -> bool: | ||
| """Return True when a NATS broker is reachable on localhost:4222.""" | ||
| try: | ||
| with socket.create_connection(("127.0.0.1", 4222), timeout=1.0): |
There was a problem hiding this comment.
The new nats_available fixture probes only 127.0.0.1:4222, but the same test flow now allows NATS_URL overrides (and defaults to nats://nats:pmoves@nats:4222). In environments where the broker is reachable via a non-local host (for example Docker DNS nats or a CI/staging endpoint), this probe returns false and nats_client skips the tests even though NATS is actually reachable, which silently removes coverage of the JetStream assertions.
Useful? React with 👍 / 👎.
POWERFULMOVES
pushed a commit
that referenced
this pull request
Apr 10, 2026
Rebased from ea7f24d onto updated main after #1193/#1194/#1196/#1201 merges. 1. pr_monitor.py: _repo_name() prefers origin remote URL over gh repo view. On fork+upstream checkouts, gh auto-detect returns upstream (openclaw) instead of origin (POWERFULMOVES). Parses SSH + HTTPS URL formats. Changed --base default from PMOVES.AI-Edition-Hardened to main. 2. preflight.mk: pr-monitor and pr-monitor-strict targets now forward PR_MONITOR_REPO as --repo and default PR_MONITOR_BASE to main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merged
5 tasks
This was referenced Apr 23, 2026
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
2 participants
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.
Summary
Consolidates NATS test fixtures into a session-scoped
conftest.pyso runtime tests stop hardcoding unauthenticated URLs and stop invokingnats.connect()as a non-existent classmethod.Three new fixtures added to
pmoves/tests/conftest.py:nats_url— session-scoped, defaults to canonicalnats://nats:pmoves@nats:4222, honoursNATS_URLoverridenats_available— cheap socket probe on127.0.0.1:4222for clean skip when broker is downnats_client— async fixture that bypasses the autouse_FakeNATSstub (popsnats*fromsys.modulesbeforeimportlib.import_module), connects with realnats-py, and tears down on exitThree tests rewired to use the fixture:
tests/functional/test_a2ui_bridge_integration.py::test_nats_stream_existstests/functional/test_a2ui_bridge_integration.py::test_nats_a2ui_subjectstests/functional/test_tokenism_simulator.py::TestTokenismSimulatorIntegrations::test_nats_connection(converted sync->async; localnats_urlfixture with unauthenticated default removed)_FakeNATS.connect()widened to returnselfwithis_connected=Trueso unit tests that accidentally import the stub don't crash on silentAttributeError.Scope note
Task brief mentioned ~8-9 failing NATS tests. Investigation showed only 3 actually open a NATS connection at runtime. The rest (
smoke/test_nats_configuration.py,smoke/test_nats_authentication.py,test_clawz_field.py,a2ui/test_bridge.py, etc.) are static env/doc grep checks that already skip or never open sockets — left alone per task constraints.Files changed
pmoves/tests/conftest.py(+53 lines)pmoves/tests/functional/test_a2ui_bridge_integration.py(-25 lines net — fixture-managed skips replaced try/except wrappers)pmoves/tests/functional/test_tokenism_simulator.py(-11 lines net)Not touched (per task constraints)
docker-compose.yml, tier env filestests/smoke/test_port_conflicts.pytests/smoke/test_service_contracts.pytests/hardening/test_docker_hardening.pyTesting
Collection
Result: 19 tests collected cleanly,
test_nats_connectionnow a<Coroutine>.Target tests (broker not running locally -> expect skipped, not failed)
Result: 3 skipped, 0 failed (reason: "NATS not reachable on localhost:4222").
Regression check (smoke/a2ui)
tests/smoke -k nats— same 7 pre-existingenv.shared-dependent failures (unrelated, file is runtime-generated and gitignored)tests/a2ui— same pre-existingModuleNotFoundError: No module named 'bridge'collection error (unrelated)Both pre-existing failures confirmed out of scope and not introduced by this change.
Test plan
_FakeNATSstub still pass (widenedconnect()return)Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit