test(nodes): add comprehensive unit tests for pipeline nodes#501
Closed
nihalnihalani wants to merge 3 commits into
Closed
test(nodes): add comprehensive unit tests for pipeline nodes#501nihalnihalani wants to merge 3 commits into
nihalnihalani wants to merge 3 commits into
Conversation
- Add shared test infrastructure with conftest.py fixtures mocking rocketlib, ai.common, engLib, and depends modules - Add LLM node tests: OpenAI (24), Anthropic (18), Gemini (20) covering config validation, error handling, lifecycle, and IInstance operations - Add Vector DB tests: Chroma (8), Pinecone (21), Qdrant (32) covering config validation, collection names, mode compat, CRUD, rendering - Add Embedding tests: OpenAI embeddings (9) covering lifecycle, document/question encoding, batch operations - Add Tool tests: HTTP request node (43) covering method guardrails, URL whitelist, auth/body validation, shortcut normalization - Add Agent tests: RocketRide agent (22) covering lifecycle, memory requirements, tool delegation, planner helpers, wave planning - Update pyproject.toml to include test/ directory in testpaths - All 197 tests use mocks, no external service dependencies Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nihalnihalani
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds extensive node-focused pytest suites and a shared test fixture module; updates pytest discovery to include both Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/nodes/conftest.py`:
- Around line 394-397: Change any decorator usages that call pytest.fixture with
empty parentheses to the no-argument form: replace `@pytest.fixture`() with
`@pytest.fixture`. Specifically update the warned_messages fixture (function
warned_messages) and any other fixture functions in this file that use
`@pytest.fixture`() with no arguments to use `@pytest.fixture` instead to follow
pytest style conventions.
In `@test/nodes/test_agent_rocketride.py`:
- Around line 140-151: The test test_write_questions_no_memory_raises nests two
with blocks; simplify by combining the patch and pytest.raises into a single
with statement to reduce nesting—patch the
nodes.agent_rocketride.IInstance.AgentHostServices to return mock_host and
assert pytest.raises(ValueError, match='memory') in the same with, leaving the
setup of IInstance, mock_host.memory=None and the call
inst.writeQuestions(MagicMock()) unchanged.
- Around line 272-279: Update the test_json_default_datetime to use a
timezone-aware datetime when calling _json_default: construct dt with a tzinfo
(e.g., datetime.timezone.utc or another fixed timezone) and then adjust the
assertions to expect the ISO-formatted string to include the timezone indicator
(for UTC either 'Z' or '+00:00' depending on _json_default) while still
asserting the date and time substrings; this ensures test_json_default_datetime
and _json_default handle timezone-aware datetimes.
- Around line 334-341: The test test_wave_question_with_scratch currently only
asserts q is not None and doesn't verify scratch notes were included; update the
test to assert the scratch content is passed into the question context by
inspecting the question mock or its addContext calls (use _make_inputs to
provide scratch and _build_wave_question to create q) — for example, assert that
q.addContext was called with the scratch string or that q.context (or equivalent
attribute) contains 'Key: wave-0.r0 = 42' to ensure scratch handling is
exercised.
- Around line 202-217: Replace the bare try/except in
test_invoke_non_tool_op_falls_through with contextlib.suppress to explicitly
ignore expected exceptions: import contextlib if needed, then wrap the call to
inst.invoke(param) in contextlib.suppress(Exception) so the test still verifies
inst.IGlobal.agent.handle_invoke.assert_not_called() without using a bare
except; reference the test function name test_invoke_non_tool_op_falls_through
and the IInstance instance inst and its methods invoke and
inst.IGlobal.agent.handle_invoke when applying the change.
In `@test/nodes/test_llm_anthropic.py`:
- Around line 271-283: The pytest.mark.parametrize call in
test_format_error_parametrized uses a comma-separated string for the parameter
names; change that first argument to an explicit tuple of parameter names (e.g.,
('status', 'etype', 'emsg', 'expected_fragment')) so the decorator uses a tuple
rather than a comma-separated string; update the decorator line above
test_format_error_parametrized accordingly to reference the corrected tuple of
parameter names.
In `@test/nodes/test_llm_gemini.py`:
- Around line 331-339: The test test_extract_uses_prov_code_fallback unpacks
emsg from ig._extract_status_message_code but never uses it; update the
assertion to either explicitly check emsg for the expected message or mark it as
intentionally ignored by renaming it to _emsg in the unpack (or prefix with
underscore) so the unused-variable warning is resolved; locate the unpack in
test_extract_uses_prov_code_fallback and change "status, emsg, code = ..." to
either "status, _emsg, code = ..." or add an assertion like "assert emsg ==
'<expected message>'".
In `@test/nodes/test_llm_openai.py`:
- Around line 273-275: The assertion after ig.beginGlobal() is ambiguous because
it mixes multiple attribute names; replace it with an explicit check for the
single expected attribute state (either _chat or chat) by consulting the
production code to determine the correct attribute name, then assert that that
attribute is absent or None (e.g., use getattr(ig, '<correct_name>', None) is
None) or, if both should be cleared in CONFIG mode, assert both getattr(ig,
'_chat', None) is None and getattr(ig, 'chat', None) is None; update the test
around ig.beginGlobal() accordingly.
- Around line 202-214: The test function
test_newer_models_use_max_completion_tokens currently accepts an unused fixture
warned_messages; remove the warned_messages parameter from the test signature
or, if you want to assert no warnings were emitted, add a single assertion
referencing warned_messages (e.g., assert not warned_messages or similar) after
ig.validateConfig() to make its usage explicit; update only the test function
signature and/or add the assertion in
test_newer_models_use_max_completion_tokens to eliminate the unused fixture
warning.
- Around line 336-342: Move the repeated local import of IInvokeLLM into the
module-level imports: add "from conftest import IInvokeLLM" at the top of the
test file and remove the in-function import statements currently present inside
the test functions that call IInvokeLLM (e.g., the tests that create
IInvokeLLM(op='ask', question=...) and call inst.invoke). Ensure all tests (the
ones currently importing IInvokeLLM on lines near the MagicMock usages)
reference the module-level symbol and run without the inline imports.
In `@test/nodes/test_tool_http_request.py`:
- Around line 343-353: In test_build_guardrails_default_methods, the returned
`patterns` from IGlobal._build_guardrails is unused; update the unpacking to use
a throwaway variable (e.g., `_`) instead of `patterns` so the intent is clear
(locate the test function `test_build_guardrails_default_methods` and change the
assignment `methods, patterns = IGlobal._build_guardrails(cfg)` to use `methods,
_ = IGlobal._build_guardrails(cfg)`).
In `@test/nodes/test_vectordb_chroma.py`:
- Around line 111-118: The test exposes that writeQuestions calls
IGlobal.store.dispatchSearch but there is no dispatchSearch implementation; add
a dispatchSearch(self, instance, question) method to the Store class (or to the
Store's base class) so the call from writeQuestions succeeds, implement it to
route to the existing search handling logic (or raise a clear
NotImplementedError if intended abstract) and ensure the signature matches the
call from writeQuestions and any tests that assert dispatchSearch was invoked.
In `@test/nodes/test_vectordb_pinecone.py`:
- Around line 129-146: The pytest.mark.parametrize decorator in
test_invalid_collection_names uses a comma-separated string for parameter names;
change it to a tuple of strings to match style (e.g., replace
'name,violation_fragment' with ('name', 'violation_fragment')) so the parameter
declaration in the test_invalid_collection_names test matches the Qdrant tests
and avoids style inconsistency.
In `@test/nodes/test_vectordb_qdrant.py`:
- Around line 178-205: Change the pytest.mark.parametrize call in
test_url_construction to pass the parameter names as a tuple (e.g.,
('host','port','expected_scheme')) instead of a string, and simplify the final
assertion by using a single startswith call with a tuple of prefixes; update the
assertion that checks captured_url (from _capture_client / QdrantClient used by
ig.validateConfig) to assert captured_url is not None and then assert
captured_url.startswith((expected_scheme + '://', host)).
- Around line 320-339: Add a positive test that ensures renderObject calls the
store's render when hasVectorBatchId is True: in the test class create an
instance via _make_instance(), set inst.IGlobal.store to a MagicMock with a mock
render method, create an entry MagicMock with hasVectorBatchId = True and
vectorBatchId = 'batch-1', call inst.renderObject(entry), and assert that
inst.IGlobal.store.render was called with the expected arguments (including the
batch id or entry as per the implementation) to mirror the Chroma/Pinecone
positive tests; reference renderObject, IGlobal.store, hasVectorBatchId,
vectorBatchId, and store.render to locate the code to test.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1ab6686b-b7fe-4205-b065-7d7ee0aed4a6
📒 Files selected for processing (11)
pyproject.tomltest/nodes/conftest.pytest/nodes/test_agent_rocketride.pytest/nodes/test_embedding_openai.pytest/nodes/test_llm_anthropic.pytest/nodes/test_llm_gemini.pytest/nodes/test_llm_openai.pytest/nodes/test_tool_http_request.pytest/nodes/test_vectordb_chroma.pytest/nodes/test_vectordb_pinecone.pytest/nodes/test_vectordb_qdrant.py
Comment on lines
+140
to
+151
| def test_write_questions_no_memory_raises(self): | ||
| """WriteQuestions should raise ValueError if no memory node is connected.""" | ||
| inst = IInstance() | ||
| inst.IGlobal = MagicMock() | ||
| inst.IGlobal.agent = MagicMock() | ||
| inst.instance = MagicMock() | ||
|
|
||
| mock_host = MagicMock() | ||
| mock_host.memory = None # No memory connected | ||
| with patch('nodes.agent_rocketride.IInstance.AgentHostServices', return_value=mock_host): | ||
| with pytest.raises(ValueError, match='memory'): | ||
| inst.writeQuestions(MagicMock()) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider combining nested with statements.
The nested with statements can be combined into a single line for cleaner code.
♻️ Suggested refactor
- with patch('nodes.agent_rocketride.IInstance.AgentHostServices', return_value=mock_host):
- with pytest.raises(ValueError, match='memory'):
- inst.writeQuestions(MagicMock())
+ with (
+ patch('nodes.agent_rocketride.IInstance.AgentHostServices', return_value=mock_host),
+ pytest.raises(ValueError, match='memory'),
+ ):
+ inst.writeQuestions(MagicMock())📝 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
| def test_write_questions_no_memory_raises(self): | |
| """WriteQuestions should raise ValueError if no memory node is connected.""" | |
| inst = IInstance() | |
| inst.IGlobal = MagicMock() | |
| inst.IGlobal.agent = MagicMock() | |
| inst.instance = MagicMock() | |
| mock_host = MagicMock() | |
| mock_host.memory = None # No memory connected | |
| with patch('nodes.agent_rocketride.IInstance.AgentHostServices', return_value=mock_host): | |
| with pytest.raises(ValueError, match='memory'): | |
| inst.writeQuestions(MagicMock()) | |
| def test_write_questions_no_memory_raises(self): | |
| """WriteQuestions should raise ValueError if no memory node is connected.""" | |
| inst = IInstance() | |
| inst.IGlobal = MagicMock() | |
| inst.IGlobal.agent = MagicMock() | |
| inst.instance = MagicMock() | |
| mock_host = MagicMock() | |
| mock_host.memory = None # No memory connected | |
| with ( | |
| patch('nodes.agent_rocketride.IInstance.AgentHostServices', return_value=mock_host), | |
| pytest.raises(ValueError, match='memory'), | |
| ): | |
| inst.writeQuestions(MagicMock()) |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 149-150: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_agent_rocketride.py` around lines 140 - 151, The test
test_write_questions_no_memory_raises nests two with blocks; simplify by
combining the patch and pytest.raises into a single with statement to reduce
nesting—patch the nodes.agent_rocketride.IInstance.AgentHostServices to return
mock_host and assert pytest.raises(ValueError, match='memory') in the same with,
leaving the setup of IInstance, mock_host.memory=None and the call
inst.writeQuestions(MagicMock()) unchanged.
Comment on lines
+202
to
+217
| def test_invoke_non_tool_op_falls_through(self): | ||
| """Invoke with non-tool.* op should fall through to base class.""" | ||
| inst = IInstance() | ||
| inst.IGlobal = MagicMock() | ||
| inst.instance = MagicMock() | ||
|
|
||
| # Non-tool op should go to super().invoke() | ||
| param = {'op': 'lifecycle.close'} | ||
| # This may raise or return depending on base impl; we just verify | ||
| # agent.handle_invoke is NOT called | ||
| try: | ||
| inst.invoke(param) | ||
| except Exception: | ||
| pass # Base class may not handle this | ||
| inst.IGlobal.agent.handle_invoke.assert_not_called() | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use contextlib.suppress for cleaner exception handling.
The test intentionally allows exceptions from the base class. Using contextlib.suppress is more explicit and addresses the Ruff S110/BLE001 warnings.
♻️ Suggested refactor
+import contextlib
+
def test_invoke_non_tool_op_falls_through(self):
"""Invoke with non-tool.* op should fall through to base class."""
inst = IInstance()
inst.IGlobal = MagicMock()
inst.instance = MagicMock()
# Non-tool op should go to super().invoke()
param = {'op': 'lifecycle.close'}
# This may raise or return depending on base impl; we just verify
# agent.handle_invoke is NOT called
- try:
- inst.invoke(param)
- except Exception:
- pass # Base class may not handle this
+ with contextlib.suppress(Exception):
+ inst.invoke(param)
inst.IGlobal.agent.handle_invoke.assert_not_called()🧰 Tools
🪛 Ruff (0.15.7)
[warning] 212-215: Use contextlib.suppress(Exception) instead of try-except-pass
(SIM105)
[error] 214-215: try-except-pass detected, consider logging the exception
(S110)
[warning] 214-214: 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 `@test/nodes/test_agent_rocketride.py` around lines 202 - 217, Replace the bare
try/except in test_invoke_non_tool_op_falls_through with contextlib.suppress to
explicitly ignore expected exceptions: import contextlib if needed, then wrap
the call to inst.invoke(param) in contextlib.suppress(Exception) so the test
still verifies inst.IGlobal.agent.handle_invoke.assert_not_called() without
using a bare except; reference the test function name
test_invoke_non_tool_op_falls_through and the IInstance instance inst and its
methods invoke and inst.IGlobal.agent.handle_invoke when applying the change.
Comment on lines
+334
to
+341
| def test_wave_question_with_scratch(self): | ||
| """Scratch notes should be included in the question context.""" | ||
| inputs = self._make_inputs(scratch='Key: wave-0.r0 = 42') | ||
| q = _build_wave_question(**inputs) | ||
| # The question object should have had addContext called | ||
| # We verify via the mock infrastructure | ||
| assert q is not None | ||
|
|
Contributor
There was a problem hiding this comment.
Test assertion is too weak to verify scratch handling.
The test only asserts q is not None, which doesn't verify that scratch notes were actually included in the question context. Consider asserting on the actual behavior.
💡 Suggested improvement
def test_wave_question_with_scratch(self):
"""Scratch notes should be included in the question context."""
inputs = self._make_inputs(scratch='Key: wave-0.r0 = 42')
q = _build_wave_question(**inputs)
- # The question object should have had addContext called
- # We verify via the mock infrastructure
- assert q is not None
+ # Verify scratch notes appear in the question's context or text
+ # Adjust based on how _build_wave_question incorporates scratch
+ assert q is not None
+ # Example: assert 'wave-0.r0' in str(q.context) or similar🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_agent_rocketride.py` around lines 334 - 341, The test
test_wave_question_with_scratch currently only asserts q is not None and doesn't
verify scratch notes were included; update the test to assert the scratch
content is passed into the question context by inspecting the question mock or
its addContext calls (use _make_inputs to provide scratch and
_build_wave_question to create q) — for example, assert that q.addContext was
called with the scratch string or that q.context (or equivalent attribute)
contains 'Key: wave-0.r0 = 42' to ensure scratch handling is exercised.
Comment on lines
+336
to
+342
| from conftest import IInvokeLLM | ||
|
|
||
| question = MagicMock() | ||
| param = IInvokeLLM(op='ask', question=question) | ||
|
|
||
| result = inst.invoke(param) | ||
| assert result is mock_answer |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider moving IInvokeLLM import to module level.
The from conftest import IInvokeLLM is repeated inside multiple test methods (lines 336, 350, 363, 377, 389). Moving it to the module-level imports would be cleaner and more conventional.
♻️ Move import to module level
from nodes.llm_openai.IGlobal import IGlobal # noqa: E402
from nodes.llm_openai.IInstance import IInstance # noqa: E402
from nodes.llm_base.IInstance import IInstanceGenericLLM # noqa: E402
+from conftest import IInvokeLLM # noqa: E402Then remove the from conftest import IInvokeLLM lines from within each test method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_llm_openai.py` around lines 336 - 342, Move the repeated
local import of IInvokeLLM into the module-level imports: add "from conftest
import IInvokeLLM" at the top of the test file and remove the in-function import
statements currently present inside the test functions that call IInvokeLLM
(e.g., the tests that create IInvokeLLM(op='ask', question=...) and call
inst.invoke). Ensure all tests (the ones currently importing IInvokeLLM on lines
near the MagicMock usages) reference the module-level symbol and run without the
inline imports.
Comment on lines
+343
to
+353
| def test_build_guardrails_default_methods(self): | ||
| """_build_guardrails with no explicit flags should enable default methods.""" | ||
| cfg = {} | ||
| methods, patterns = IGlobal._build_guardrails(cfg) | ||
| # Defaults: GET, POST, PUT, PATCH, DELETE | ||
| assert 'GET' in methods | ||
| assert 'POST' in methods | ||
| assert 'PUT' in methods | ||
| assert 'PATCH' in methods | ||
| assert 'DELETE' in methods | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use underscore prefix for unused variable.
The patterns variable is not used in this test. Use _ to indicate it's intentionally ignored.
♻️ Suggested fix
def test_build_guardrails_default_methods(self):
"""_build_guardrails with no explicit flags should enable default methods."""
cfg = {}
- methods, patterns = IGlobal._build_guardrails(cfg)
+ methods, _ = IGlobal._build_guardrails(cfg)
# Defaults: GET, POST, PUT, PATCH, DELETE
assert 'GET' in methods📝 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
| def test_build_guardrails_default_methods(self): | |
| """_build_guardrails with no explicit flags should enable default methods.""" | |
| cfg = {} | |
| methods, patterns = IGlobal._build_guardrails(cfg) | |
| # Defaults: GET, POST, PUT, PATCH, DELETE | |
| assert 'GET' in methods | |
| assert 'POST' in methods | |
| assert 'PUT' in methods | |
| assert 'PATCH' in methods | |
| assert 'DELETE' in methods | |
| def test_build_guardrails_default_methods(self): | |
| """_build_guardrails with no explicit flags should enable default methods.""" | |
| cfg = {} | |
| methods, _ = IGlobal._build_guardrails(cfg) | |
| # Defaults: GET, POST, PUT, PATCH, DELETE | |
| assert 'GET' in methods | |
| assert 'POST' in methods | |
| assert 'PUT' in methods | |
| assert 'PATCH' in methods | |
| assert 'DELETE' in methods |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 346-346: Unpacked variable patterns is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_tool_http_request.py` around lines 343 - 353, In
test_build_guardrails_default_methods, the returned `patterns` from
IGlobal._build_guardrails is unused; update the unpacking to use a throwaway
variable (e.g., `_`) instead of `patterns` so the intent is clear (locate the
test function `test_build_guardrails_default_methods` and change the assignment
`methods, patterns = IGlobal._build_guardrails(cfg)` to use `methods, _ =
IGlobal._build_guardrails(cfg)`).
Comment on lines
+111
to
+118
| def test_write_questions_dispatches_search(self): | ||
| """WriteQuestions should delegate to store.dispatchSearch.""" | ||
| inst = self._make_instance() | ||
| question = MagicMock() | ||
|
|
||
| inst.writeQuestions(question) | ||
|
|
||
| inst.IGlobal.store.dispatchSearch.assert_called_once_with(inst, question) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for dispatchSearch method in the chroma module
rg -n 'dispatchSearch' nodes/src/nodes/chroma/
# Also check if it's inherited from a base class
rg -n 'def dispatchSearch' nodes/src/Repository: rocketride-org/rocketride-server
Length of output: 172
dispatchSearch is called in production code but method definition is not found in nodes/src/.
The test correctly asserts that writeQuestions delegates to store.dispatchSearch. However, verification reveals:
- Production code at
nodes/src/nodes/chroma/IInstance.py:42callsself.IGlobal.store.dispatchSearch(self, question) - The
dispatchSearchmethod definition is not present innodes/src/(search fordef dispatchSearchfound no results)
This indicates the method is being invoked but its definition is missing from the codebase, which would cause a runtime error. Verify that dispatchSearch is either properly defined in the Store class or inherited from a parent class.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_vectordb_chroma.py` around lines 111 - 118, The test exposes
that writeQuestions calls IGlobal.store.dispatchSearch but there is no
dispatchSearch implementation; add a dispatchSearch(self, instance, question)
method to the Store class (or to the Store's base class) so the call from
writeQuestions succeeds, implement it to route to the existing search handling
logic (or raise a clear NotImplementedError if intended abstract) and ensure the
signature matches the call from writeQuestions and any tests that assert
dispatchSearch was invoked.
Comment on lines
+129
to
+146
| @pytest.mark.parametrize( | ||
| 'name,violation_fragment', | ||
| [ | ||
| ('My-Index', 'lowercase'), | ||
| ('my_index!', 'lowercase'), | ||
| ('-my-index', 'start or end'), | ||
| ('my-index-', 'start or end'), | ||
| ('my--index', 'consecutive'), | ||
| ('a' * 46, 'too long'), | ||
| ], | ||
| ) | ||
| def test_invalid_collection_names(self, name, violation_fragment, mock_config, warned_messages): | ||
| """Various invalid collection names should produce appropriate warnings.""" | ||
| config = {'apikey': 'test-key', 'collection': name, 'mode': 'serverless-dense'} | ||
| ig = self._make_iglobal(config, mock_config) | ||
| ig.validateConfig() | ||
| assert len(warned_messages) >= 1 | ||
| assert any(violation_fragment in m.lower() for m in warned_messages) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use tuple for parametrize first argument.
Same style suggestion as the Qdrant tests - use a tuple instead of a comma-separated string for the parameter names.
♻️ Proposed style fix
`@pytest.mark.parametrize`(
- 'name,violation_fragment',
+ ('name', 'violation_fragment'),
[
('My-Index', 'lowercase'),🧰 Tools
🪛 Ruff (0.15.7)
[warning] 130-130: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_vectordb_pinecone.py` around lines 129 - 146, The
pytest.mark.parametrize decorator in test_invalid_collection_names uses a
comma-separated string for parameter names; change it to a tuple of strings to
match style (e.g., replace 'name,violation_fragment' with ('name',
'violation_fragment')) so the parameter declaration in the
test_invalid_collection_names test matches the Qdrant tests and avoids style
inconsistency.
Comment on lines
+320
to
+339
| def test_render_object_no_store_raises(self): | ||
| """RenderObject should raise when store is None.""" | ||
| inst = self._make_instance() | ||
| inst.IGlobal.store = None | ||
|
|
||
| entry = MagicMock() | ||
| entry.hasVectorBatchId = True | ||
| entry.vectorBatchId = 'batch-1' | ||
|
|
||
| with pytest.raises(Exception, match='No document store'): | ||
| inst.renderObject(entry) | ||
|
|
||
| def test_render_object_no_batch_id_skips(self): | ||
| """RenderObject should skip when no vectorBatchId.""" | ||
| inst = self._make_instance() | ||
| entry = MagicMock() | ||
| entry.hasVectorBatchId = False | ||
| entry.vectorBatchId = None | ||
|
|
||
| inst.renderObject(entry) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
renderObject tests are incomplete - missing positive case.
The test class covers renderObject raising when store is None and skipping when hasVectorBatchId is False, but there's no test verifying the positive path where hasVectorBatchId is True and store.render is actually called. This matches the pattern in the Chroma and Pinecone tests.
➕ Add missing positive test case
def test_render_object_no_batch_id_skips(self):
"""RenderObject should skip when no vectorBatchId."""
inst = self._make_instance()
entry = MagicMock()
entry.hasVectorBatchId = False
entry.vectorBatchId = None
inst.renderObject(entry)
inst.IGlobal.store.render.assert_not_called()
+
+ def test_render_object_with_batch_id_renders(self):
+ """RenderObject should call store.render when vectorBatchId is set."""
+ inst = self._make_instance()
+ entry = MagicMock()
+ entry.hasVectorBatchId = True
+ entry.vectorBatchId = 'batch-1'
+ entry.objectId = 'obj-123'
+
+ inst.renderObject(entry)
+ inst.IGlobal.store.render.assert_called_once()
+ call_kwargs = inst.IGlobal.store.render.call_args
+ assert call_kwargs.kwargs['objectId'] == 'obj-123'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_vectordb_qdrant.py` around lines 320 - 339, Add a positive
test that ensures renderObject calls the store's render when hasVectorBatchId is
True: in the test class create an instance via _make_instance(), set
inst.IGlobal.store to a MagicMock with a mock render method, create an entry
MagicMock with hasVectorBatchId = True and vectorBatchId = 'batch-1', call
inst.renderObject(entry), and assert that inst.IGlobal.store.render was called
with the expected arguments (including the batch id or entry as per the
implementation) to mirror the Chroma/Pinecone positive tests; reference
renderObject, IGlobal.store, hasVectorBatchId, vectorBatchId, and store.render
to locate the code to test.
- Add session-scoped fixture for sys.modules cleanup to prevent test bleed - Save original sys.modules entries before mutation, restore on teardown - Add module-scoped cleanup fixtures in per-file SDK mock installations - Replace weak three-way or assertions with specific single-condition checks - Remove redundant path setup already handled by conftest.py Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/nodes/conftest.py`:
- Around line 407-409: The test setup inserts _nodes_src into sys.path but only
restores sys.modules; change the fixture (or teardown) that currently restores
sys.modules to also restore sys.path by removing the injected _nodes_src if
present (e.g., capture original_sys_path = list(sys.path) or record the index
when inserting _nodes_src and then restore/pop that entry), and apply the same
cleanup for the other similar insertion block around lines 417-432 so the
temporary nodes/src import root is removed after the fixture completes.
- Around line 174-188: The MockQuestion currently no-ops addInstruction and
addContext so mutations from _build_wave_question are lost; update __init__ to
initialize the inner mock's instruction/context containers (e.g., q =
self.questions[0]; q.instructions = []; q.contexts = []) and implement
addInstruction(name, body) to append a structured entry (e.g., {"name": name,
"body": body}) to q.instructions and addContext(ctx) to append ctx to q.contexts
so tests can inspect retained instructions and context applied by
_build_wave_question.
In `@test/nodes/test_llm_gemini.py`:
- Around line 393-398: The test is asserting the wrong attribute—production uses
self._chat while the test checks self.chat; update the
test_end_global_clears_chat to set ig._chat = MagicMock() (or call beginGlobal
to create it) before calling ig.endGlobal(), then assert that ig._chat is None
to verify the Chat instance is actually cleared; reference the IGlobal class and
its endGlobal method/_chat attribute when locating the change.
In `@test/nodes/test_tool_http_request.py`:
- Around line 43-59: The teardown currently restores the original http_client
module but leaves other node modules that imported execute_request cached with
the mocked reference; update the fixture _restore_http_sdk_modules to also pop
any cached modules that imported the mocked client (e.g., remove entries like
'nodes.tool_http_request.http_driver' and any submodules that may have been
loaded which contain HttpDriver, IGlobal, or IInstance) from sys.modules so
subsequent tests re-import fresh real modules; implement this by iterating over
a list or prefix (e.g., any key.startswith('nodes.tool_http_request')) and
deleting those keys from sys.modules in the teardown after restoring the
original http_client.
In `@test/nodes/test_vectordb_qdrant.py`:
- Around line 248-268: The tests assert parts (_format_error including
response.status_code and plain/text message) that _format_error does not
produce; update the two tests (test_format_http_status_error_with_json and
test_format_http_status_error_with_plain_text) to match _format_error's
contract: for the JSON case mock the response.json() to return {"status":
{"error": "Collection not found"}} and assert the returned string contains
"Collection not found" (but do not expect "404" or status_code), and for the
plain-text case have the mock response raise a JSON parse error (or return no
{"status":...}) and assert the result falls back to str(err) (e.g., contains the
_FakeHTTPStatusError message) instead of asserting the response.status_code or
plain text body; reference helpers _format_error and _FakeHTTPStatusError and
the IGlobal logic that extracts status.error.
- Around line 189-216: The test test_url_construction allows bare "host:port"
because it accepts captured_url.startswith(host); change the assertion to
require an explicit scheme for cases where host does not already include one by
(1) capturing the URL via _capture_client as is, (2) detecting if the input host
string includes "http://" or "https://", and (3) asserting that if the input
host lacks a scheme then captured_url starts with expected_scheme + "://",
otherwise assert captured_url starts with the original host (which already
contains a scheme); update the assertion logic in test_url_construction
accordingly so captured_url cannot be a bare host:port when a scheme is
expected.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: bbf202fd-4d56-4a15-a4cd-38661e7bbe2a
📒 Files selected for processing (10)
test/nodes/conftest.pytest/nodes/test_agent_rocketride.pytest/nodes/test_embedding_openai.pytest/nodes/test_llm_anthropic.pytest/nodes/test_llm_gemini.pytest/nodes/test_llm_openai.pytest/nodes/test_tool_http_request.pytest/nodes/test_vectordb_chroma.pytest/nodes/test_vectordb_pinecone.pytest/nodes/test_vectordb_qdrant.py
Comment on lines
+174
to
+188
| def __init__(self, text='test question'): | ||
| """Initialize.""" | ||
| self.questions = [MagicMock(text=text, embedding=None, embedding_model=None)] | ||
| self.role = '' | ||
| self.expectJson = False | ||
| self.goals = [] | ||
|
|
||
| def addGoal(self, text): | ||
| self.goals.append(text) | ||
|
|
||
| def addInstruction(self, name, body): | ||
| pass | ||
|
|
||
| def addContext(self, ctx): | ||
| pass |
Contributor
There was a problem hiding this comment.
Make MockQuestion retain appended instructions and context.
_build_wave_question() mutates the question through addInstruction() and addContext(), but both methods are no-ops here. That makes the planner tests blind to scratch/context regressions and is why the downstream scratch-path assertion can only check that the object exists.
♻️ Suggested fix
class MockQuestion:
"""Simulates ai.common.schema.Question."""
def __init__(self, text='test question'):
"""Initialize."""
self.questions = [MagicMock(text=text, embedding=None, embedding_model=None)]
self.role = ''
self.expectJson = False
self.goals = []
+ self.instructions = []
+ self.contexts = []
@@
def addInstruction(self, name, body):
- pass
+ self.instructions.append({'name': name, 'body': body})
def addContext(self, ctx):
- pass
+ self.contexts.append(ctx)📝 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
| def __init__(self, text='test question'): | |
| """Initialize.""" | |
| self.questions = [MagicMock(text=text, embedding=None, embedding_model=None)] | |
| self.role = '' | |
| self.expectJson = False | |
| self.goals = [] | |
| def addGoal(self, text): | |
| self.goals.append(text) | |
| def addInstruction(self, name, body): | |
| pass | |
| def addContext(self, ctx): | |
| pass | |
| def __init__(self, text='test question'): | |
| """Initialize.""" | |
| self.questions = [MagicMock(text=text, embedding=None, embedding_model=None)] | |
| self.role = '' | |
| self.expectJson = False | |
| self.goals = [] | |
| self.instructions = [] | |
| self.contexts = [] | |
| def addGoal(self, text): | |
| self.goals.append(text) | |
| def addInstruction(self, name, body): | |
| self.instructions.append({'name': name, 'body': body}) | |
| def addContext(self, ctx): | |
| self.contexts.append(ctx) |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 174-174: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/conftest.py` around lines 174 - 188, The MockQuestion currently
no-ops addInstruction and addContext so mutations from _build_wave_question are
lost; update __init__ to initialize the inner mock's instruction/context
containers (e.g., q = self.questions[0]; q.instructions = []; q.contexts = [])
and implement addInstruction(name, body) to append a structured entry (e.g.,
{"name": name, "body": body}) to q.instructions and addContext(ctx) to append
ctx to q.contexts so tests can inspect retained instructions and context applied
by _build_wave_question.
Comment on lines
+407
to
+409
| _nodes_src = _os.path.abspath(_os.path.join(_os.path.dirname(__file__), '..', '..', 'nodes', 'src')) | ||
| if _nodes_src not in sys.path: | ||
| sys.path.insert(0, _nodes_src) |
Contributor
There was a problem hiding this comment.
Clean up the injected nodes/src path too.
Line 409 prepends _nodes_src to sys.path, but the session fixture only restores sys.modules. That leaves this extra import root at the front of the interpreter path for the rest of the pytest run, so later non-node tests can resolve against this test-only path.
♻️ Suggested fix
_nodes_src = _os.path.abspath(_os.path.join(_os.path.dirname(__file__), '..', '..', 'nodes', 'src'))
+_inserted_nodes_src = False
if _nodes_src not in sys.path:
sys.path.insert(0, _nodes_src)
+ _inserted_nodes_src = True
...
`@pytest.fixture`(autouse=True, scope='session')
def _restore_mock_modules():
"""Restore original sys.modules entries after the test session ends.
@@
"""
yield
+
+ if _inserted_nodes_src and _nodes_src in sys.path:
+ sys.path.remove(_nodes_src)
for mod_name in _MOCKED_MODULE_NAMES:
if mod_name in _original_modules:
sys.modules[mod_name] = _original_modules[mod_name]Also applies to: 417-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/conftest.py` around lines 407 - 409, The test setup inserts
_nodes_src into sys.path but only restores sys.modules; change the fixture (or
teardown) that currently restores sys.modules to also restore sys.path by
removing the injected _nodes_src if present (e.g., capture original_sys_path =
list(sys.path) or record the index when inserting _nodes_src and then
restore/pop that entry), and apply the same cleanup for the other similar
insertion block around lines 417-432 so the temporary nodes/src import root is
removed after the fixture completes.
Comment on lines
+393
to
+398
| def test_end_global_clears_chat(self): | ||
| """EndGlobal should set chat to None.""" | ||
| ig = IGlobal() | ||
| ig.chat = MagicMock() | ||
| ig.endGlobal() | ||
| assert ig.chat is None |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which attribute name endGlobal clears in Gemini production code
rg -n '_chat|\.chat' --type=py nodes/src/nodes/llm_gemini/IGlobal.py | head -30Repository: rocketride-org/rocketride-server
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Find the endGlobal method in IGlobal.py
rg -n "def endGlobal" nodes/src/nodes/llm_gemini/IGlobal.py -A 10Repository: rocketride-org/rocketride-server
Length of output: 648
🏁 Script executed:
#!/bin/bash
# Check the full IGlobal.py to understand both attributes and if chat is a property
cat -n nodes/src/nodes/llm_gemini/IGlobal.py | sed -n '170,200p'Repository: rocketride-org/rocketride-server
Length of output: 1550
🏁 Script executed:
#!/bin/bash
# Search for `@property` chat to see if it wraps _chat
rg -n "@property" nodes/src/nodes/llm_gemini/IGlobal.py -A 3Repository: rocketride-org/rocketride-server
Length of output: 58
🏁 Script executed:
#!/bin/bash
# Look for class-level initialization of both attributes
rg -n "self\.chat\s*=" nodes/src/nodes/llm_gemini/IGlobal.pyRepository: rocketride-org/rocketride-server
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Check the broader context of IGlobal class to understand attribute initialization
cat -n nodes/src/nodes/llm_gemini/IGlobal.py | head -180 | tail -80Repository: rocketride-org/rocketride-server
Length of output: 4042
Attribute name inconsistency: test should verify _chat cleanup, not chat.
The production code in beginGlobal() creates self._chat (the actual Chat instance), but endGlobal() clears self.chat instead—a different, uninitialized attribute. The test currently validates the wrong behavior. To actually verify that the Chat instance is cleaned up, the test must use _chat:
🔧 Proposed fix
def test_end_global_clears_chat(self):
"""EndGlobal should set chat to None."""
ig = IGlobal()
- ig.chat = MagicMock()
+ ig._chat = MagicMock()
ig.endGlobal()
- assert ig.chat is None
+ assert ig._chat is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_llm_gemini.py` around lines 393 - 398, The test is asserting
the wrong attribute—production uses self._chat while the test checks self.chat;
update the test_end_global_clears_chat to set ig._chat = MagicMock() (or call
beginGlobal to create it) before calling ig.endGlobal(), then assert that
ig._chat is None to verify the Chat instance is actually cleared; reference the
IGlobal class and its endGlobal method/_chat attribute when locating the change.
Comment on lines
+43
to
+59
| _SDK_MODULES = ['nodes.tool_http_request.http_client'] | ||
| _saved_sdk_modules = {name: sys.modules[name] for name in _SDK_MODULES if name in sys.modules} | ||
|
|
||
| _mock_http_client = types.ModuleType('nodes.tool_http_request.http_client') | ||
| _mock_http_client.execute_request = MagicMock(return_value={'status': 200, 'body': 'ok'}) | ||
| sys.modules['nodes.tool_http_request.http_client'] = _mock_http_client | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True, scope='module') | ||
| def _restore_http_sdk_modules(): | ||
| """Restore original SDK modules after all tests in this module run.""" | ||
| yield | ||
| for name in _SDK_MODULES: | ||
| if name in _saved_sdk_modules: | ||
| sys.modules[name] = _saved_sdk_modules[name] | ||
| elif name in sys.modules: | ||
| del sys.modules[name] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test setup installs fake module before importing the node =="
sed -n '43,68p' test/nodes/test_tool_http_request.py
echo
echo "== production driver uses a module-level execute_request symbol =="
sed -n '287,305p' nodes/src/nodes/tool_http_request/http_driver.py
echo
echo "== teardown does not unload the cached node modules =="
rg -n "del sys.modules\\['nodes\\.tool_http_request|pop\\('nodes\\.tool_http_request" test/nodes/test_tool_http_request.py || trueRepository: rocketride-org/rocketride-server
Length of output: 2213
🏁 Script executed:
head -30 nodes/src/nodes/tool_http_request/http_driver.pyRepository: rocketride-org/rocketride-server
Length of output: 1624
🏁 Script executed:
rg -n "from.*execute_request|import.*execute_request" nodes/src/nodes/tool_http_request/http_driver.pyRepository: rocketride-org/rocketride-server
Length of output: 119
🏁 Script executed:
sed -n '1,50p' nodes/src/nodes/tool_http_request/http_driver.py | grep -E "^import|^from"Repository: rocketride-org/rocketride-server
Length of output: 251
🏁 Script executed:
grep -n "sys.modules" test/nodes/test_tool_http_request.py test/conftest.py 2>/dev/null | head -20Repository: rocketride-org/rocketride-server
Length of output: 566
🏁 Script executed:
fd -t f "IGlobal|IInstance" nodes/src/nodes/tool_http_request/Repository: rocketride-org/rocketride-server
Length of output: 167
🏁 Script executed:
rg -n "from.*http_client|import.*execute_request" nodes/src/nodes/tool_http_request/IGlobal.py nodes/src/nodes/tool_http_request/IInstance.pyRepository: rocketride-org/rocketride-server
Length of output: 58
🏁 Script executed:
sed -n '43,70p' test/nodes/test_tool_http_request.pyRepository: rocketride-org/rocketride-server
Length of output: 1291
Unload the imported node modules that cached the mocked HTTP client.
This test injects a fake nodes.tool_http_request.http_client module before importing HttpDriver, IGlobal, and IInstance. When http_driver.py imports execute_request from .http_client, it captures a reference to the mocked version. The teardown restores only the original http_client module, leaving the three imported node modules cached in sys.modules with stale mocked references. Subsequent tests in the same worker will reuse these cached modules with the mocked behavior.
♻️ Suggested fix
_SDK_MODULES = ['nodes.tool_http_request.http_client']
+_NODE_MODULES = [
+ 'nodes.tool_http_request.http_driver',
+ 'nodes.tool_http_request.IGlobal',
+ 'nodes.tool_http_request.IInstance',
+]
_saved_sdk_modules = {name: sys.modules[name] for name in _SDK_MODULES if name in sys.modules}
@@
def _restore_http_sdk_modules():
"""Restore original SDK modules after all tests in this module run."""
yield
+ for name in _NODE_MODULES:
+ sys.modules.pop(name, None)
for name in _SDK_MODULES:
if name in _saved_sdk_modules:
sys.modules[name] = _saved_sdk_modules[name]
elif name in sys.modules:
del sys.modules[name]🧰 Tools
🪛 Ruff (0.15.7)
[warning] 52-52: Missing return type annotation for private function _restore_http_sdk_modules
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_tool_http_request.py` around lines 43 - 59, The teardown
currently restores the original http_client module but leaves other node modules
that imported execute_request cached with the mocked reference; update the
fixture _restore_http_sdk_modules to also pop any cached modules that imported
the mocked client (e.g., remove entries like
'nodes.tool_http_request.http_driver' and any submodules that may have been
loaded which contain HttpDriver, IGlobal, or IInstance) from sys.modules so
subsequent tests re-import fresh real modules; implement this by iterating over
a list or prefix (e.g., any key.startswith('nodes.tool_http_request')) and
deleting those keys from sys.modules in the teardown after restoring the
original http_client.
Comment on lines
+189
to
+216
| @pytest.mark.parametrize( | ||
| 'host,port,expected_scheme', | ||
| [ | ||
| ('localhost', 6333, 'http'), | ||
| ('127.0.0.1', 6333, 'http'), | ||
| ('my-cloud.qdrant.io', 443, 'https'), | ||
| ('my-cloud.qdrant.io', 6333, 'https'), | ||
| ('http://localhost', 6333, 'http'), | ||
| ('https://cloud.qdrant.io', 443, 'https'), | ||
| ], | ||
| ) | ||
| def test_url_construction(self, host, port, expected_scheme, mock_config, warned_messages): | ||
| """URL construction should choose the right scheme based on host/port.""" | ||
| config = {'host': host, 'port': port, 'apikey': '', 'collection': 'test-col'} | ||
| ig = self._make_iglobal(config, mock_config) | ||
|
|
||
| captured_url = None | ||
|
|
||
| def _capture_client(*args, **kwargs): | ||
| nonlocal captured_url | ||
| captured_url = kwargs.get('url', args[0] if args else None) | ||
| return _FakeQdrantClient() | ||
|
|
||
| with patch('qdrant_client.QdrantClient', side_effect=_capture_client): | ||
| ig.validateConfig() | ||
|
|
||
| assert captured_url is not None | ||
| assert captured_url.startswith(expected_scheme + '://') or captured_url.startswith(host) |
Contributor
There was a problem hiding this comment.
Require a scheme for hosts that do not already include one.
Line 216 still passes when captured_url is just localhost:6333, because startswith(host) is allowed for the bare-host cases. That weakens the exact branch this test is supposed to cover.
♻️ Suggested fix
with patch('qdrant_client.QdrantClient', side_effect=_capture_client):
ig.validateConfig()
assert captured_url is not None
- assert captured_url.startswith(expected_scheme + '://') or captured_url.startswith(host)
+ if '://' in host:
+ assert captured_url.startswith(host)
+ else:
+ assert captured_url.startswith(f'{expected_scheme}://')🧰 Tools
🪛 Ruff (0.15.7)
[warning] 190-190: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
[warning] 200-200: Unused method argument: warned_messages
(ARG002)
[warning] 207-207: Missing return type annotation for private function _capture_client
(ANN202)
[warning] 207-207: Missing type annotation for *args
(ANN002)
[warning] 207-207: Missing type annotation for **kwargs
(ANN003)
[warning] 216-216: Call startswith once with a tuple
Merge into a single startswith call
(PIE810)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_vectordb_qdrant.py` around lines 189 - 216, The test
test_url_construction allows bare "host:port" because it accepts
captured_url.startswith(host); change the assertion to require an explicit
scheme for cases where host does not already include one by (1) capturing the
URL via _capture_client as is, (2) detecting if the input host string includes
"http://" or "https://", and (3) asserting that if the input host lacks a scheme
then captured_url starts with expected_scheme + "://", otherwise assert
captured_url starts with the original host (which already contains a scheme);
update the assertion logic in test_url_construction accordingly so captured_url
cannot be a bare host:port when a scheme is expected.
Comment on lines
+248
to
+268
| def test_format_http_status_error_with_json(self): | ||
| """Should parse JSON body from HTTPStatusError.""" | ||
| mock_resp = MagicMock() | ||
| mock_resp.status_code = 404 | ||
| mock_resp.text = '{"message": "Collection not found"}' | ||
| err = _FakeHTTPStatusError('Not found', response=mock_resp) | ||
|
|
||
| result = _format_error(err) | ||
| assert '404' in result | ||
| assert 'Collection not found' in result | ||
|
|
||
| def test_format_http_status_error_with_plain_text(self): | ||
| """Should use plain text when JSON parsing fails.""" | ||
| mock_resp = MagicMock() | ||
| mock_resp.status_code = 500 | ||
| mock_resp.text = 'Internal server error' | ||
| err = _FakeHTTPStatusError('Error', response=mock_resp) | ||
|
|
||
| result = _format_error(err) | ||
| assert '500' in result | ||
| assert 'Internal server error' in result |
Contributor
There was a problem hiding this comment.
Align these HTTPStatusError assertions with _format_error()'s actual contract.
nodes/src/nodes/qdrant/IGlobal.py:145-175 only extracts status.error from JSON bodies and otherwise falls back to str(e). It never includes response.status_code, and it ignores {"message": ...} / plain-text bodies, so these two tests currently assert behavior the helper does not implement.
♻️ Suggested fix
def test_format_http_status_error_with_json(self):
"""Should parse JSON body from HTTPStatusError."""
mock_resp = MagicMock()
- mock_resp.status_code = 404
- mock_resp.text = '{"message": "Collection not found"}'
+ mock_resp.text = '{"status": {"error": "Collection not found"}}'
err = _FakeHTTPStatusError('Not found', response=mock_resp)
result = _format_error(err)
- assert '404' in result
- assert 'Collection not found' in result
+ assert result == 'Collection not found'
@@
def test_format_http_status_error_with_plain_text(self):
"""Should use plain text when JSON parsing fails."""
mock_resp = MagicMock()
- mock_resp.status_code = 500
mock_resp.text = 'Internal server error'
- err = _FakeHTTPStatusError('Error', response=mock_resp)
+ err = _FakeHTTPStatusError('Internal server error', response=mock_resp)
result = _format_error(err)
- assert '500' in result
- assert 'Internal server error' in result
+ assert result == 'Internal server error'📝 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
| def test_format_http_status_error_with_json(self): | |
| """Should parse JSON body from HTTPStatusError.""" | |
| mock_resp = MagicMock() | |
| mock_resp.status_code = 404 | |
| mock_resp.text = '{"message": "Collection not found"}' | |
| err = _FakeHTTPStatusError('Not found', response=mock_resp) | |
| result = _format_error(err) | |
| assert '404' in result | |
| assert 'Collection not found' in result | |
| def test_format_http_status_error_with_plain_text(self): | |
| """Should use plain text when JSON parsing fails.""" | |
| mock_resp = MagicMock() | |
| mock_resp.status_code = 500 | |
| mock_resp.text = 'Internal server error' | |
| err = _FakeHTTPStatusError('Error', response=mock_resp) | |
| result = _format_error(err) | |
| assert '500' in result | |
| assert 'Internal server error' in result | |
| def test_format_http_status_error_with_json(self): | |
| """Should parse JSON body from HTTPStatusError.""" | |
| mock_resp = MagicMock() | |
| mock_resp.text = '{"status": {"error": "Collection not found"}}' | |
| err = _FakeHTTPStatusError('Not found', response=mock_resp) | |
| result = _format_error(err) | |
| assert result == 'Collection not found' | |
| def test_format_http_status_error_with_plain_text(self): | |
| """Should use plain text when JSON parsing fails.""" | |
| mock_resp = MagicMock() | |
| mock_resp.text = 'Internal server error' | |
| err = _FakeHTTPStatusError('Internal server error', response=mock_resp) | |
| result = _format_error(err) | |
| assert result == 'Internal server error' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nodes/test_vectordb_qdrant.py` around lines 248 - 268, The tests assert
parts (_format_error including response.status_code and plain/text message) that
_format_error does not produce; update the two tests
(test_format_http_status_error_with_json and
test_format_http_status_error_with_plain_text) to match _format_error's
contract: for the JSON case mock the response.json() to return {"status":
{"error": "Collection not found"}} and assert the returned string contains
"Collection not found" (but do not expect "404" or status_code), and for the
plain-text case have the mock response raise a JSON parse error (or return no
{"status":...}) and assert the result falls back to str(err) (e.g., contains the
_FakeHTTPStatusError message) instead of asserting the response.status_code or
plain text body; reference helpers _format_error and _FakeHTTPStatusError and
the IGlobal logic that extracts status.error.
- Remove empty parentheses from @pytest.fixture() decorators in conftest.py for idiomatic pytest style - Use timezone-aware datetime (UTC) in test_json_default_datetime to avoid naive datetime deprecation warnings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
|
Hey @nihalnihalani — thanks for the effort here, and the mock infrastructure in conftest.py is genuinely well-constructed. Unfortunately we can't merge this as-is. The project already has a test framework that handles exactly this use case: nodes/test/ contains a dynamic test runner (test_dynamic.py) that discovers and executes test cases defined in each node's services.json under a "test" key. You can see examples of this pattern in nodes/src/nodes/llm_anthropic/services.json, nodes/src/nodes/chroma/services.json, etc. The framework runs these against a live server and supports profiles, input types, and output assertions natively. Adding test coverage for a node means adding a "test" block to its services.json — not a separate mocked pytest file. The mocked approach unfortunately bypasses the engine and provider integrations entirely, which is where most of the real failure modes live. A few specific issues:
If you'd like to contribute test coverage, adding "test" blocks to the nodes that are currently missing them (there are ~30) would be a great and mergeable contribution. Happy to point you to existing examples if helpful. Closing for now — appreciate you taking the time. |
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.
#Hack-with-bay-2
Contribution Type
Feature — Comprehensive unit test suite for pipeline nodes (197 tests)
Problem / Use Case
RocketRide Server has 56+ pipeline nodes but only ~7 test files existed, giving roughly 12% test coverage on the node layer. This means regressions in LLM provider integrations, vector database operations, embedding generation, and agent orchestration go undetected until production.
The existing mock infrastructure (
test/nodes/test_init_mocks.py) providesengLibmocking but lacks the comprehensive fixture system needed to test IGlobal/IInstance lifecycle, config validation, and error handling across all node categories.Proposed Solution
test/nodes/conftest.py) — Session-scoped fixtures mockingrocketlib,ai.common.config,ai.common.chat,ai.common.schema,engLib, anddepends. Includes propersys.modulescleanup via autouse fixtures to prevent test bleed.Value Added
Why This Feature Fits This Codebase
Every RocketRide node follows the IGlobal/IInstance pattern defined in
rocketlib. The test infrastructure mirrors this by providing mockIGlobalBaseandIInstanceBasethat simulate the engine's lifecycle calls:validateConfig()→beginGlobal()→writeQuestions()/writeDocuments()→endGlobal().The tests validate real code paths. For example,
test_llm_openai.pyimports the actualnodes/src/nodes/llm_openai/IGlobal.pyand verifies thatvalidateConfig()correctly handlesopenai.AuthenticationErrorvsopenai.RateLimitError— the same provider-driven exception pattern used across all LLM nodes (established in the OpenAI node at lines 73-107 ofIGlobal.py).The conftest mock for
Config.getNodeConfig()returns profile-merged configurations matching thepreconfig.profilesstructure in each node'sservices.json, ensuring tests exercise the same config resolution path as production.Validation
How This Could Be Extended
🤖 Generated with Claude Code