Feature/UI node endpoints tests #4
Conversation
WalkthroughAdds centralized HTTP request/response logging, a token-aware Nodes API client with stateful node lifecycle methods, Node model/state types, OpenAPI Nodes endpoints and schemas, expired-token generators, new node-focused fixtures, and comprehensive node tests (create/get/list/delete/concurrency/security/state). Changes
Sequence Diagram(s)sequenceDiagram
participant TestClient as Test Client
participant NodesClient as NodesAPIClient
participant APIServer as API Server
participant NodeBackend as Node Backend
TestClient->>NodesClient: create_node(preset_id)
NodesClient->>APIServer: POST /v1/ui/nodes (Bearer)
APIServer->>NodeBackend: init deployment
NodeBackend-->>APIServer: 201 Created {deployment_id, state:PENDING}
APIServer-->>NodesClient: response
NodesClient->>NodesClient: _log_response(resp, "create")
NodesClient->>NodesClient: nodes_list.add(deployment_id)
NodesClient-->>TestClient: CreateNodeResponse
TestClient->>NodesClient: get_node(node_id)
NodesClient->>APIServer: GET /v1/ui/nodes/{id} (Bearer)
APIServer->>NodeBackend: fetch node
NodeBackend-->>APIServer: 200 {node_data, state:RUNNING}
APIServer-->>NodesClient: response
NodesClient->>NodesClient: _log_response(resp, "get")
NodesClient-->>TestClient: Node details
TestClient->>NodesClient: schedule_delete_node(node_id)
NodesClient->>APIServer: POST /v1/ui/nodes/{id}/schedule-delete (Bearer)
APIServer->>NodeBackend: schedule deletion -> DELETING
NodeBackend-->>APIServer: 200 Scheduled
APIServer-->>NodesClient: response
NodesClient->>NodesClient: _log_response(resp, "delete")
NodesClient->>NodesClient: nodes_list.remove(node_id)
NodesClient-->>TestClient: ScheduleDeleteNodeResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/api/test_auth_username.py (1)
196-202: Bug: Header modification not applied before request.The test modifies
headers(a copy) on line 198 but callschange_usernameon line 199 which uses the originalauthenticated_auth_client.headers. The modified headers are only assigned back on line 202, after the request. The test is not actually testing wrong auth type.Proposed fix
def test_change_username_with_wrong_auth_type(self, authenticated_auth_client, valid_username): headers = authenticated_auth_client.headers.copy() - headers["Authorization"] = "Basic " + base64.b64encode(authenticated_auth_client.token.encode()).decode() - response = authenticated_auth_client.change_username(valid_username) + authenticated_auth_client.headers["Authorization"] = "Basic " + base64.b64encode(authenticated_auth_client.token.encode()).decode() + try: + response = authenticated_auth_client.change_username(valid_username) + assert response.status_code == 401, f"Expected 401, got {response.status_code}" + ErrorResponse(**response.json()) + finally: + authenticated_auth_client.headers = headers - assert response.status_code == 401, f"Expected 401, got {response.status_code}" - ErrorResponse(**response.json()) - authenticated_auth_client.headers = headersThe same bug exists in:
test_change_username_with_wrong_auth_format(lines 206-212)test_change_username_with_too_long_access_token(lines 216-222)test_change_username_without_content_type(lines 240-246)test_change_username_with_wrong_content_type(lines 252-257)tests/api/test_auth_profile.py (1)
208-214: Bug: Content-Type modification not applied before request.Line 210 modifies
headers(a copy) but the request on line 211 uses the originalauthenticated_auth_client.headers. The test is not actually testing wrong Content-Type.Proposed fix
def test_get_profile_with_wrong_content_type(self, authenticated_auth_client, content_type): headers = authenticated_auth_client.headers.copy() - headers["Content-Type"] = content_type - response = authenticated_auth_client.get_profile() + authenticated_auth_client.headers["Content-Type"] = content_type + try: + response = authenticated_auth_client.get_profile() + assert response.status_code == 400, f"Expected 400, got {response.status_code}" + ErrorResponse(**response.json()) + finally: + authenticated_auth_client.headers = headers - assert response.status_code == 400, f"Expected 400, got {response.status_code}" - ErrorResponse(**response.json()) - authenticated_auth_client.headers = headerstests/api/test_auth_refresh.py (1)
70-76: Expired‑token tests may not exercise the expiry path.
generate_expired_token()andgenerate_expired_refresh_token()produce JWTs with a fake signature, so servers that validate signatures first will treat them as invalid-signature tokens rather than expired. Consider generating a signed expired token (if a test key is available) or renaming/asserting the intent to avoid a false sense of coverage.Also applies to: 173-181
🤖 Fix all issues with AI agents
In `@clients/api_client.py`:
- Around line 153-173: create_node appends the API-returned deployment_id as-is
but schedule_delete_node lowercases node_id before checking self.nodes_list,
causing mismatches for mixed-case IDs; make case handling consistent by
normalizing IDs the same way in both places (e.g., change create_node to append
response.json()["deployment_id"].lower() or change schedule_delete_node to
compare/remove using the exact casing returned by the API), and ensure both
create_node and schedule_delete_node use the same normalization logic when
adding/removing from self.nodes_list.
- Around line 175-177: _teardown currently iterates self.nodes_list while
schedule_delete_node mutates that same list, causing skipped items or
RuntimeError; fix by iterating over a shallow copy (e.g., for node_id in
list(self.nodes_list)) or by popping/consuming nodes until empty, then call
schedule_delete_node(node_id) for each and finally ensure self.nodes_list is
cleared; update the _teardown implementation referencing _teardown,
schedule_delete_node, and nodes_list to use the copy/clear approach so mutation
during iteration is avoided.
In `@control_panel/node.py`:
- Around line 9-11: The __init__ signature for class Node incorrectly types the
state parameter as NodeState while callers pass string constants like
NodeState.RUNNING; update this by either changing the Node.__init__(...) type
hint to state: str and keep NodeState as a plain class of string constants, or
convert NodeState into an enum (from enum import Enum) and update Node.__init__
to state: NodeState and ensure all callers pass the Enum members (e.g.,
NodeState.RUNNING) consistently; locate and edit the Node class __init__ and the
NodeState definition/usage to apply one of these two consistent fixes.
In `@fixtures/api_fixtures.py`:
- Around line 126-139: The fixture authenticated_nodes_client currently raises a
bare Exception on failed authentication and never closes the AuthAPIClient
(auth_client) leading to a resource leak; fix by wrapping the login/use logic in
try/finally so auth_client.close() is always called, instantiate and use
NodesAPIClient(token=token) only when response.status_code == 200 (yielding it
and calling client._teardown() after yield), and raise a more specific
RuntimeError with the response.status_code when authentication fails.
In `@openapi.yaml`:
- Around line 661-668: The parameter description for the path parameter named
"id" in the parameters block is incorrect; update the description from
"Deployment ID" to "Node ID" so it accurately reflects the resource (the
parameter named id under parameters with schema type: string, format: uuid).
Locate the parameters section (parameter name "id") in openapi.yaml and replace
the description text accordingly; ensure similar path parameter definitions
referencing "id" for node routes are reviewed and corrected if needed.
- Around line 614-620: The path parameter "id" currently has a description
"Deployment ID" but this endpoint refers to a Node resource; update the
parameter description for the path parameter named "id" in openapi.yaml to read
"Node ID" (leave name: id, in: path, required: true, schema/type/format
unchanged) so the documentation matches the endpoint semantics.
- Around line 1335-1346: The CreateNodeRequest schema still uses the old "CTI"
term in the preset_instance_id description; update that description to use "GTS"
instead (e.g., change "CTI preset instance ID" to "GTS preset instance ID") and
ensure any example text in preset_instance_id or related comments references GTS
consistently; edit the CreateNodeRequest block (properties preset_instance_id
and preset_override_values) to replace remaining "CTI" occurrences with "GTS".
In `@tests/api/test_nodes_get.py`:
- Around line 168-171: The test test_get_node_invalid_method is calling
authenticated_nodes_client.send_custom_request with the wrong endpoint; update
the endpoint string to include the missing /v1/ui prefix so it matches the rest
of tests (use "/v1/ui/nodes/{existing_node_id}" instead of
"/nodes/{existing_node_id}"); modify the call in test_get_node_invalid_method
(and ensure it still uses the existing_node_id and method variables) so the
request targets the correct API path.
In `@tests/api/test_nodes_list.py`:
- Around line 75-83: The tests (test_list_nodes_without_auth_token,
test_list_nodes_with_invalid_token, test_list_nodes_with_wrong_auth_type,
test_list_nodes_with_too_long_token) temporarily mutate
authenticated_nodes_client.token and currently restore it only after assertions,
which can leave the token mutated if an assertion fails; wrap the token
modification and request/assertion in a try/finally (or implement a small
context manager) so that authenticated_nodes_client.token is always restored in
the finally block, ensuring token restoration regardless of test failures.
- Around line 98-108: The test test_list_nodes_with_wrong_auth_type incorrectly
sets authenticated_nodes_client.token which causes the client to send
"Authorization: Bearer Basic ..."; instead, modify the client's headers
directly: save authenticated_nodes_client.headers, set
authenticated_nodes_client.headers["Authorization"] to a proper "Basic
<base64(token)>" value, call authenticated_nodes_client.list_nodes(), assert
status_code == 401 and validate ErrorResponse(**response.json()), then restore
the original headers; this uses the same pattern as test_auth_refresh.py and
targets authenticated_nodes_client and its headers rather than the token
property.
In `@tests/api/test_nodes_security.py`:
- Around line 52-60: The test test_unauthorized_response_security_headers can
leave authenticated_nodes_client.token unset if an assertion fails; wrap the
token removal and assertions in a try/finally so the original token is always
restored: save token from authenticated_nodes_client.token, set
authenticated_nodes_client.token = None, perform response =
authenticated_nodes_client.list_nodes() and the assertions (status code and call
to _verify_security_headers) inside the try, and in the finally block restore
authenticated_nodes_client.token = token to guarantee restoration even on
failures.
- Around line 36-41: Add an explicit status code assertion before verifying
headers in test_delete_node_security_headers: after calling
authenticated_nodes_client.schedule_delete_node(existing_node_id) assert the
expected HTTP status (e.g., assert response.status_code == 204 — match the
status used by other delete tests) and then call
self._verify_security_headers(response); update the asserted value to whatever
other tests in this file use for deletes if different.
🧹 Nitpick comments (16)
control_panel/node.py (1)
1-7: Consider usingEnumforNodeStateinstead of a plain class with constants.Using
Enumwould provide better type safety, IDE autocompletion, and prevent invalid state values:♻️ Suggested refactor using Enum
+from enum import Enum + -class NodeState: - PENDING = "pending" - RUNNING = "running" - STOPPED = "stopped" - ERROR = "error" - DELETING = "deleting" - DELETED = "deleted" +class NodeState(str, Enum): + PENDING = "pending" + RUNNING = "running" + STOPPED = "stopped" + ERROR = "error" + DELETING = "deleting" + DELETED = "deleted"tests/api/cases/test_cases.py (1)
6-6: Consider adding truly malformed UUID formats toINVALID_UUID_CASES.The current list includes structurally valid UUIDs (nil UUID and a random valid UUID). If the intent is to test UUID validation, consider adding malformed formats:
♻️ Suggested additions for better edge case coverage
-INVALID_UUID_CASES = ["invalid-uuid", "00000000-0000-0000-0000-000000000000", "123e4567-e89b-12d3-a456-426614174000"] +INVALID_UUID_CASES = [ + "invalid-uuid", + "00000000-0000-0000-0000-000000000000", # nil UUID - may be rejected by some APIs + "123e4567-e89b-12d3-a456-426614174000", # valid format but non-existent + "not-a-uuid-at-all", + "12345", + "123e4567-e89b-12d3-a456", # truncated + "123e4567-e89b-12d3-a456-426614174000-extra", # too long +]utils/token_generator.py (1)
228-245: Consider whether refresh tokens need different claims.The function generates a token with access token claims (email, username, tenant_id, tenant_role) by using default values from
generate_expired_token. If refresh tokens in your system have a different payload structure (e.g., onlysubandtype: "refresh"), this could be refined. However, since the signature is invalid anyway and this is for testing the "expired/unauthorized" path, the current implementation should work.clients/api_client.py (1)
179-186: Consider usingTimeoutErrorinstead of genericException.A more specific exception type improves debuggability and allows callers to handle timeout scenarios differently from other errors.
♻️ Suggested improvement
def _wait_node_until_status(self, node_id: str, expected_status: NodeState, timeout: int = 60): start_time = time.time() while time.time() - start_time < timeout: response = self.get_node(node_id) if response.json()["status"] == expected_status: return time.sleep(1) - raise Exception(f"Node {node_id} is not {expected_status} after {timeout} seconds") + raise TimeoutError(f"Node {node_id} did not reach status '{expected_status}' within {timeout} seconds")fixtures/api_fixtures.py (1)
160-180: Missing yield in the failure path and potential assertion improvement.The fixture uses
pytest.skip()but doesn't yield anything in the failure case. Whilepytest.skip()raises an exception so the yield is never reached, the pattern could be clearer. Also, consider adding a status code assertion for the successful path.Additionally, the polling loop (lines 169-176) doesn't handle the case where
response.status_code != 200during polling—it just continues waiting.♻️ Suggested improvement
`@pytest.fixture`(scope="function") def created_node_for_deletion(authenticated_nodes_client, valid_eth_preset_instance_id): """Create a node specifically for deletion tests.""" deployment_response = authenticated_nodes_client.create_node( preset_instance_id=valid_eth_preset_instance_id ) - if deployment_response.status_code == 201: - max_wait = 30 - waited = 0 - while waited < max_wait: - response = authenticated_nodes_client.get_node(deployment_response.json()["deployment_id"]) - if response.status_code == 200: - current_status = response.json().get("status") - if current_status != NodeState.PENDING: - break - time.sleep(1) - waited += 1 - # Use deployment_response for deployment_id (get_node returns 'id', not 'deployment_id') - yield {"deployment_id": deployment_response.json()["deployment_id"]} - else: + if deployment_response.status_code != 201: pytest.skip(f"Could not create node for deletion test: {deployment_response.status_code}") + + max_wait = 30 + waited = 0 + node_id = deployment_response.json()["deployment_id"] + while waited < max_wait: + response = authenticated_nodes_client.get_node(node_id) + if response.status_code == 200: + current_status = response.json().get("status") + if current_status != NodeState.PENDING: + break + time.sleep(1) + waited += 1 + yield {"deployment_id": node_id}tests/api/test_nodes_concurrency.py (2)
5-5: Unused import:CreateNodeResponse.
CreateNodeResponseis imported but never used in this file. Consider removing it.-from tests.api.schemas.node_schemas import CreateNodeResponse
82-85: Missing exception handling inget_nodelambda.Unlike
create_nodeanddelete_nodefunctions in this file,get_nodedoesn't have a try/except block. Ifget_node()raises an exception (network error, timeout, etc.), the test will fail with an unhandled exception rather than capturing it for analysis.♻️ Proposed fix for consistency
def get_node(): - response = authenticated_nodes_client.get_node(existing_node_id) - with lock: - responses.append(response.json() if response.status_code == 200 else None) + try: + response = authenticated_nodes_client.get_node(existing_node_id) + with lock: + responses.append(response.json() if response.status_code == 200 else None) + except Exception as e: + with lock: + responses.append(None)tests/api/test_nodes_state.py (1)
4-5: Unused imports:ValidationError,Node,ErrorResponse.These imports are not used anywhere in the file. Consider removing them to keep the imports clean.
-from pydantic import ValidationError -from tests.api.schemas.node_schemas import Node, ErrorResponsetests/api/test_nodes_create.py (2)
54-54: Remove unused variable assignments.The
create_responsevariables are assigned but never used. If schema validation is the intent, theCreateNodeResponse(**response.json())call alone suffices (it will raiseValidationErroron failure).Proposed fix
- create_response = CreateNodeResponse(**response.json()) + CreateNodeResponse(**response.json()) # Validates schemaApply similar change at line 71.
Also applies to: 71-71
230-240: Use try/finally for token restoration to ensure cleanup on test failure.If any assertion fails before
authenticated_nodes_client.token = tokenis reached, the client remains in a modified state, potentially affecting subsequent tests.Proposed fix
def test_create_node_without_auth_token(self, authenticated_nodes_client, valid_eth_preset_instance_id): token = authenticated_nodes_client.token authenticated_nodes_client.token = None - - response = authenticated_nodes_client.create_node( - preset_instance_id=valid_eth_preset_instance_id - ) - - assert response.status_code == 401, f"Expected 401, got {response.status_code}" - ErrorResponse(**response.json()) - authenticated_nodes_client.token = token + try: + response = authenticated_nodes_client.create_node( + preset_instance_id=valid_eth_preset_instance_id + ) + + assert response.status_code == 401, f"Expected 401, got {response.status_code}" + ErrorResponse(**response.json()) + finally: + authenticated_nodes_client.token = tokenApply the same pattern to
test_create_node_with_invalid_token(lines 245-255) andtest_create_node_with_wrong_auth_type(lines 259-269).tests/api/test_nodes_delete.py (3)
11-12: Consider removing trivial wrapper function.
generate_random_uuid()simply wrapsstr(uuid4()). Sinceuuid4()is already imported and used directly elsewhere in the file (line 99), this wrapper adds no value.Proposed simplification
-def generate_random_uuid(): - return str(uuid4())Then use
str(uuid4())directly where needed, which is already done on line 99.
62-69: Tests rely on private API method_wait_node_until_status.The tests call
_wait_node_until_statuswhich is marked as private (underscore prefix). If this method is intended for test use, consider removing the underscore prefix to make it part of the public API. Otherwise, consider adding a public wrapper or fixture for waiting on node state.
136-144: Use try/finally for token restoration.Same issue as in
test_nodes_create.py- the token restoration should be wrapped in try/finally to ensure cleanup even if assertions fail.tests/api/test_nodes_get.py (3)
26-26: Remove extraneous f-prefix from string.The f-string
f"Node ID mismatch"has no placeholders. Remove thefprefix.Proposed fix
- assert node.id == existing_node_id, f"Node ID mismatch" + assert node.id == existing_node_id, "Node ID mismatch"
80-81: Inconsistent use of NodeState constants vs hardcoded strings.Line 80 uses
"pending"as a hardcoded string, while line 101 usesNodeState.DELETED. For consistency and maintainability, importNodeStateat the module level and use constants throughout.Proposed fix
Add to imports at top of file:
from control_panel.node import NodeStateThen update line 80:
- assert response.json()["status"] == "pending", "Node should be pending" + assert response.json()["status"] == NodeState.PENDING, "Node should be pending"And remove the local import at line 86.
Also applies to: 86-86
181-189: Use try/finally for token restoration.Consistent with other test files, wrap the token manipulation in try/finally to ensure proper cleanup.
| def create_node(self, preset_instance_id: str, preset_override_values: Optional[Dict[str, Any]] = None) -> httpx.Response: | ||
| payload = {"preset_instance_id": preset_instance_id} | ||
| if preset_override_values: | ||
| payload["preset_override_values"] = preset_override_values | ||
| response = self.post("/v1/ui/nodes", json=payload) | ||
| if response.status_code == 201: | ||
| self.nodes_list.append(response.json()["deployment_id"]) | ||
| return response | ||
|
|
||
| def create_node(self, node_data: Dict[str, Any]) -> httpx.Response: | ||
| return self.post("/ui/nodes", json=node_data) | ||
|
|
||
| def get_nodes(self, filters: Optional[Dict[str, Any]] = None) -> httpx.Response: | ||
| return self.get("/ui/nodes", params=filters) | ||
| def list_nodes(self) -> httpx.Response: | ||
| return self.get("/v1/ui/nodes") | ||
|
|
||
| def get_node(self, node_id: str) -> httpx.Response: | ||
| return self.get(f"/ui/nodes/{node_id}") | ||
| return self.get(f"/v1/ui/nodes/{node_id}") | ||
|
|
||
| def update_node(self, node_id: str, update_data: Dict[str, Any]) -> httpx.Response: | ||
| return self.put(f"/ui/nodes/{node_id}", json=update_data) | ||
| def schedule_delete_node(self, node_id: str) -> httpx.Response: | ||
| operational_node_id = node_id.lower() | ||
| response = self.post(f"/v1/ui/nodes/{node_id}/schedule-delete") | ||
| if response.status_code == 200 and operational_node_id in self.nodes_list: | ||
| self.nodes_list.remove(operational_node_id) | ||
| return response |
There was a problem hiding this comment.
Inconsistent case handling between create_node and schedule_delete_node.
create_node stores deployment_id as-is (line 159), but schedule_delete_node converts node_id to lowercase before checking/removing from the list (line 169-172). If the API returns mixed-case IDs, the removal logic may fail to find the item.
🔧 Proposed fix for consistent case handling
def create_node(self, preset_instance_id: str, preset_override_values: Optional[Dict[str, Any]] = None) -> httpx.Response:
payload = {"preset_instance_id": preset_instance_id}
if preset_override_values:
payload["preset_override_values"] = preset_override_values
response = self.post("/v1/ui/nodes", json=payload)
if response.status_code == 201:
- self.nodes_list.append(response.json()["deployment_id"])
+ self.nodes_list.append(response.json()["deployment_id"].lower())
return response🤖 Prompt for AI Agents
In `@clients/api_client.py` around lines 153 - 173, create_node appends the
API-returned deployment_id as-is but schedule_delete_node lowercases node_id
before checking self.nodes_list, causing mismatches for mixed-case IDs; make
case handling consistent by normalizing IDs the same way in both places (e.g.,
change create_node to append response.json()["deployment_id"].lower() or change
schedule_delete_node to compare/remove using the exact casing returned by the
API), and ensure both create_node and schedule_delete_node use the same
normalization logic when adding/removing from self.nodes_list.
There was a problem hiding this comment.
API nor returns mixed case IDs, my tests use mixed case IDs therefore to delete node_id from container i transform it to lowercase
| def _teardown(self): | ||
| for node_id in self.nodes_list: | ||
| self.schedule_delete_node(node_id) |
There was a problem hiding this comment.
_teardown called once after all tests gone, so nod list modifications
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrate the list modification bug with a simple Python example
python3 << 'EOF'
# Simulate the current _teardown behavior
print("=== Current Implementation (Buggy) ===")
nodes_list = ['node1', 'node2', 'node3']
print(f"Initial list: {nodes_list}")
deleted_nodes = []
for node_id in nodes_list:
print(f"Processing: {node_id}")
deleted_nodes.append(node_id)
nodes_list.remove(node_id) # Simulates schedule_delete_node removing from list
print(f" List after removal: {nodes_list}")
print(f"\nDeleted nodes: {deleted_nodes}")
print(f"Remaining nodes (should be empty!): {nodes_list}")
print(f"❌ Problem: node2 was skipped and remains in the list!\n")
print("=== Fixed Implementation (Iterate over copy) ===")
nodes_list = ['node1', 'node2', 'node3']
print(f"Initial list: {nodes_list}")
deleted_nodes = []
for node_id in nodes_list[:]: # Iterate over a copy
print(f"Processing: {node_id}")
deleted_nodes.append(node_id)
nodes_list.remove(node_id)
print(f" List after removal: {nodes_list}")
print(f"\nDeleted nodes: {deleted_nodes}")
print(f"Remaining nodes: {nodes_list}")
print(f"✅ All nodes successfully deleted!")
EOF
Length of output: 666
@0xc7c5, I understand your concern, but the issue occurs during the single execution of _teardown, not across multiple calls.
When you modify a list while iterating over it, Python's iterator skips elements because the indices shift. Here's what happens with your current code:
The Problem (demonstrated above):
Initial list: ['node1', 'node2', 'node3']
Processing: node1
List after removal: ['node2', 'node3']
Processing: node3 ← node2 was skipped!
List after removal: ['node2']
Remaining nodes: ['node2'] ← This node is leaked!
As you can see, node2 is never deleted because:
- Iterator processes
node1at index 0 schedule_delete_noderemovesnode1→ list becomes['node2', 'node3']- Iterator moves to index 1, which is now
node3(skippingnode2)
Impact: Every other node will remain in self.nodes_list and won't be cleaned up during teardown, potentially leaving orphaned resources.
Fix: Iterate over a copy of the list:
def _teardown(self):
for node_id in self.nodes_list[:]: # Iterate over a shallow copy
self.schedule_delete_node(node_id)This ensures all nodes are properly deleted regardless of how many exist in the list.
🧬 Code Graph Analysis Results
[clients/api_client.py] _teardown method
def _teardown(self):
for node_id in self.nodes_list:
self.schedule_delete_node(node_id)
[control_panel/node.py] NodeState class (context for node statuses used in teardown/wait logic)
class NodeState:
PENDING = "pending"
RUNNING = "running"
STOPPED = "stopped"
ERROR = "error"
DELETING = "deleting"
DELETED = "deleted"
[tests/api/test_nodes_concurrency.py] create_node usage (relevant to how nodes_list is populated for teardown)
def create_node():
try:
response = authenticated_nodes_client.create_node(
preset_instance_id=valid_eth_preset_instance_id
)
with lock:
if response.status_code == 201:
created_nodes.append(response.json()["deployment_id"])
else:
errors.append(f"Status {response.status_code}: {response.text}")
except Exception as e:
with lock:
errors.append(str(e))
[tests/api/test_nodes_concurrency.py] get_node usage (relevant to teardown behavior affecting node state checks)
def get_node():
response = authenticated_nodes_client.get_node(existing_node_id)
with lock:
responses.append(response.json() if response.status_code == 200 else None)
There was a problem hiding this comment.
The what? nodes_list remains persistent during teardown.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrate the exact bug in the user's code by simulating the _teardown behavior
python3 << 'EOF'
print("=== Simulating the current _teardown implementation ===\n")
class NodesAPIClient:
def __init__(self):
self.nodes_list = []
def create_node(self, node_id):
"""Simulate node creation"""
self.nodes_list.append(node_id)
print(f"Created node: {node_id}")
def schedule_delete_node(self, node_id):
"""Simulate the schedule_delete_node method"""
print(f" Attempting to delete: {node_id}")
operational_node_id = node_id.lower()
# Simulate successful deletion (status_code == 200)
if operational_node_id in self.nodes_list:
self.nodes_list.remove(operational_node_id)
print(f" ✓ Removed {node_id} from nodes_list")
print(f" Current nodes_list: {self.nodes_list}")
else:
print(f" ✗ {node_id} not found in nodes_list")
def _teardown(self):
"""Current implementation - iterates while modifying"""
print(f"Starting teardown with nodes_list: {self.nodes_list}\n")
iteration_count = 0
for node_id in self.nodes_list:
iteration_count += 1
print(f"Iteration {iteration_count}:")
self.schedule_delete_node(node_id)
print()
print(f"Teardown complete. Total iterations: {iteration_count}")
print(f"Remaining nodes_list: {self.nodes_list}\n")
# Test with 4 nodes
print("Test 1: 4 nodes")
print("-" * 50)
client1 = NodesAPIClient()
client1.create_node('node-1')
client1.create_node('node-2')
client1.create_node('node-3')
client1.create_node('node-4')
print()
client1._teardown()
print(f"❌ BUG: {len(client1.nodes_list)} nodes were NOT deleted: {client1.nodes_list}\n")
# Test with 3 nodes
print("\nTest 2: 3 nodes")
print("-" * 50)
client2 = NodesAPIClient()
client2.create_node('alpha')
client2.create_node('beta')
client2.create_node('gamma')
print()
client2._teardown()
print(f"❌ BUG: {len(client2.nodes_list)} node(s) were NOT deleted: {client2.nodes_list}\n")
# Show the fixed version
print("\n" + "="*50)
print("=== FIXED VERSION (iterate over copy) ===")
print("="*50 + "\n")
class FixedNodesAPIClient(NodesAPIClient):
def _teardown(self):
"""Fixed implementation - iterate over copy"""
print(f"Starting teardown with nodes_list: {self.nodes_list}\n")
iteration_count = 0
for node_id in self.nodes_list[:]: # Iterate over a COPY
iteration_count += 1
print(f"Iteration {iteration_count}:")
self.schedule_delete_node(node_id)
print()
print(f"Teardown complete. Total iterations: {iteration_count}")
print(f"Remaining nodes_list: {self.nodes_list}\n")
client3 = FixedNodesAPIClient()
client3.create_node('node-1')
client3.create_node('node-2')
client3.create_node('node-3')
client3.create_node('node-4')
print()
client3._teardown()
print(f"✅ SUCCESS: All nodes deleted. Remaining: {client3.nodes_list}")
EOF
Length of output: 2104
@0xc7c5, you're absolutely correct that nodes_list remains persistent during teardown—and that's precisely why the bug occurs!
The script output above proves the bug happens in a single call to _teardown:
🔴 The Evidence
Test 1: Started with 4 nodes
- Only 2 iterations occurred (should be 4!)
- Result: 2 nodes leaked (
['node-2', 'node-4'])
Test 2: Started with 3 nodes
- Only 2 iterations occurred (should be 3!)
- Result: 1 node leaked (
['beta'])
🔍 Why Persistence Causes the Bug
Here's exactly what happens when you iterate over the same persistent list you're modifying:
# Initial state
nodes_list = ['node-1', 'node-2', 'node-3', 'node-4']
iterator_index = 0
# Iteration 1: iterator at index 0
node_id = nodes_list[0] # 'node-1'
schedule_delete_node('node-1')
# nodes_list.remove('node-1') happens
# nodes_list is now: ['node-2', 'node-3', 'node-4']
iterator_index = 1 # Python moves to next index
# Iteration 2: iterator at index 1
node_id = nodes_list[1] # 'node-3' (node-2 was SKIPPED!)
schedule_delete_node('node-3')
# nodes_list is now: ['node-2', 'node-4']
iterator_index = 2 # Python moves to next index
# Iteration stops because index 2 >= len(nodes_list)
# Result: ['node-2', 'node-4'] remain forever ❌✅ The Fix
Iterate over a copy so the original list can be safely modified:
def _teardown(self):
for node_id in self.nodes_list[:]: # Create a shallow copy
self.schedule_delete_node(node_id)The fixed version successfully processes all 4 nodes in 4 iterations, leaving nodes_list = [].
Impact: Without this fix, half your nodes will leak during teardown, leaving orphaned cloud resources. 🚨
🧬 Code Graph Analysis Results
[filename: clients/api_client.py]
-
Summary: NodesAPIClient class (relevant to nodes_list persistence across teardown)
docstring-style description:
-
Class: NodesAPIClient(APIClient)
-
Attributes:
- nodes_list: List[str] - stores deployment_id strings of created nodes.
-
Methods:
- init(settings: Settings, token: Optional[str] = None)
- Initializes base API client with cp_nodes_api_url and token/api_token.
- Initializes self.nodes_list = [].
- create_node(preset_instance_id: str, preset_override_values: Optional[Dict[str, Any]] = None) -> httpx.Response
- POSTs to /v1/ui/nodes with payload containing preset_instance_id and optional preset_override_values.
- If response status_code is 201, appends response.json()["deployment_id"] to self.nodes_list.
- Returns the HTTP response.
- list_nodes() -> httpx.Response
- GET /v1/ui/nodes.
- get_node(node_id: str) -> httpx.Response
- GET /v1/ui/nodes/{node_id}.
- schedule_delete_node(node_id: str) -> httpx.Response
- POST /v1/ui/nodes/{node_id}/schedule-delete.
- If response.status_code == 200 and operational_node_id (lowercased node_id) is in self.nodes_list, removes it from the list.
- Returns the HTTP response.
- _teardown()
- Iterates over self.nodes_list and calls schedule_delete_node(node_id) for each entry.
- Note: nodes_list is used to drive teardown deletions and may persist if _teardown is not invoked or if entries are not properly removed.
- _wait_node_until_status(node_id: str, expected_status: NodeState, timeout: int = 60)
- Repeatedly GETs the node until its status equals expected_status or timeout occurs.
- Raises Exception if timeout expires.
- init(settings: Settings, token: Optional[str] = None)
-
Important implementation details:
- nodes_list growth occurs on successful node creation (status 201) by appending deployment_id.
- nodes_list removal occurs on a successful schedule-delete (status 200) if the entry matches the lowercased node_id.
- _teardown relies on nodes_list to perform deletions; if _teardown is not called, nodes may remain and persist beyond test teardown.
-
-
Filename: control_panel/node.py
Snippet (relevant to NodeState values used by NodesAPIClient):-
File: control_panel/node.py
-
Lines: 1-7
-
Content:
class NodeState:
PENDING = "pending"
RUNNING = "running"
STOPPED = "stopped"
ERROR = "error"
DELETING = "deleting"
DELETED = "deleted" -
This provides the possible statuses used by _wait_node_until_status in NodesAPIClient to compare against expected_status.
-
There was a problem hiding this comment.
@0xc7c5, excellent! Thank you for fixing the iteration bug. Your nodes will now be properly cleaned up during teardown. 🎉
🐰✨
/)/)
(^.^)
c(")(")
happy hop
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| class Node: | ||
| def __init__(self, state: NodeState): | ||
| self.state = state |
There was a problem hiding this comment.
Misleading type hint for state parameter.
The type hint state: NodeState suggests an instance of NodeState, but the actual usage passes string constants like NodeState.RUNNING. If keeping NodeState as a plain class, the type hint should be str. If you adopt the Enum approach, the type hint becomes correct.
🔧 Proposed fix if keeping plain class
class Node:
- def __init__(self, state: NodeState):
+ def __init__(self, state: str):
self.state = state📝 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.
| class Node: | |
| def __init__(self, state: NodeState): | |
| self.state = state | |
| class Node: | |
| def __init__(self, state: str): | |
| self.state = state |
🤖 Prompt for AI Agents
In `@control_panel/node.py` around lines 9 - 11, The __init__ signature for class
Node incorrectly types the state parameter as NodeState while callers pass
string constants like NodeState.RUNNING; update this by either changing the
Node.__init__(...) type hint to state: str and keep NodeState as a plain class
of string constants, or convert NodeState into an enum (from enum import Enum)
and update Node.__init__ to state: NodeState and ensure all callers pass the
Enum members (e.g., NodeState.RUNNING) consistently; locate and edit the Node
class __init__ and the NodeState definition/usage to apply one of these two
consistent fixes.
| @pytest.fixture(scope="session") | ||
| def authenticated_nodes_client(config: Settings): | ||
|
|
||
| auth_client = AuthAPIClient(config) | ||
| response = auth_client.login(config.admin_log, config.admin_pass) | ||
|
|
||
| if response.status_code == 200: | ||
| token = response.json().get("access_token") | ||
| client = NodesAPIClient(config, token=token) | ||
| yield client | ||
| client._teardown() | ||
| else: | ||
| raise Exception(f"Failed to authenticate for nodes client: {response.status_code}") | ||
| auth_client.close() |
There was a problem hiding this comment.
Resource leak: auth_client is not closed when authentication fails.
When response.status_code != 200, the exception is raised but auth_client.close() is never called. Move auth_client.close() to a finally block or restructure the code.
🐛 Proposed fix
`@pytest.fixture`(scope="session")
def authenticated_nodes_client(config: Settings):
-
auth_client = AuthAPIClient(config)
- response = auth_client.login(config.admin_log, config.admin_pass)
-
- if response.status_code == 200:
- token = response.json().get("access_token")
- client = NodesAPIClient(config, token=token)
- yield client
- client._teardown()
- else:
- raise Exception(f"Failed to authenticate for nodes client: {response.status_code}")
- auth_client.close()
+ try:
+ response = auth_client.login(config.admin_log, config.admin_pass)
+ if response.status_code != 200:
+ raise RuntimeError(f"Failed to authenticate for nodes client: {response.status_code}")
+ token = response.json().get("access_token")
+ client = NodesAPIClient(config, token=token)
+ try:
+ yield client
+ finally:
+ client._teardown()
+ finally:
+ auth_client.close()This also addresses the static analysis hints (TRY002/TRY003) by using a more specific RuntimeError instead of a bare Exception.
🧰 Tools
🪛 Ruff (0.14.14)
138-138: Create your own exception
(TRY002)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@fixtures/api_fixtures.py` around lines 126 - 139, The fixture
authenticated_nodes_client currently raises a bare Exception on failed
authentication and never closes the AuthAPIClient (auth_client) leading to a
resource leak; fix by wrapping the login/use logic in try/finally so
auth_client.close() is always called, instantiate and use
NodesAPIClient(token=token) only when response.status_code == 200 (yielding it
and calling client._teardown() after yield), and raise a more specific
RuntimeError with the response.status_code when authentication fails.
There was a problem hiding this comment.
If auth fails there is nothing to close.
| - name: id | ||
| in: path | ||
| required: true | ||
| description: Deployment ID | ||
| schema: | ||
| type: string | ||
| format: uuid |
There was a problem hiding this comment.
Parameter description says "Deployment ID" but this is a Node endpoint.
The description should say "Node ID" for consistency with the endpoint purpose.
📝 Suggested fix
parameters:
- name: id
in: path
required: true
- description: Deployment ID
+ description: Node ID
schema:
type: string
format: uuid📝 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.
| - name: id | |
| in: path | |
| required: true | |
| description: Deployment ID | |
| schema: | |
| type: string | |
| format: uuid | |
| - name: id | |
| in: path | |
| required: true | |
| description: Node ID | |
| schema: | |
| type: string | |
| format: uuid |
🤖 Prompt for AI Agents
In `@openapi.yaml` around lines 614 - 620, The path parameter "id" currently has a
description "Deployment ID" but this endpoint refers to a Node resource; update
the parameter description for the path parameter named "id" in openapi.yaml to
read "Node ID" (leave name: id, in: path, required: true, schema/type/format
unchanged) so the documentation matches the endpoint semantics.
There was a problem hiding this comment.
Lol, not my fault, i've just copied that file. Ignore it.
There was a problem hiding this comment.
@0xc7c5, understood! I'll skip this one. 👍
(\\_/)
(o.o)
( > < Noted!
| def test_get_node_invalid_method(self, authenticated_nodes_client, existing_node_id, method): | ||
| response = authenticated_nodes_client.send_custom_request(endpoint=f"/nodes/{existing_node_id}", method=method) | ||
|
|
||
| assert response.status_code == 405, f"Expected 405, got {response.status_code}" |
There was a problem hiding this comment.
Incorrect endpoint path - missing /v1/ui prefix.
The endpoint /nodes/{existing_node_id} is missing the /v1/ui prefix. Other tests and the API client use /v1/ui/nodes/{id}. This test will hit the wrong endpoint.
Proposed fix
- response = authenticated_nodes_client.send_custom_request(endpoint=f"/nodes/{existing_node_id}", method=method)
+ response = authenticated_nodes_client.send_custom_request(endpoint=f"/v1/ui/nodes/{existing_node_id}", method=method)📝 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.
| def test_get_node_invalid_method(self, authenticated_nodes_client, existing_node_id, method): | |
| response = authenticated_nodes_client.send_custom_request(endpoint=f"/nodes/{existing_node_id}", method=method) | |
| assert response.status_code == 405, f"Expected 405, got {response.status_code}" | |
| def test_get_node_invalid_method(self, authenticated_nodes_client, existing_node_id, method): | |
| response = authenticated_nodes_client.send_custom_request(endpoint=f"/v1/ui/nodes/{existing_node_id}", method=method) | |
| assert response.status_code == 405, f"Expected 405, got {response.status_code}" |
🤖 Prompt for AI Agents
In `@tests/api/test_nodes_get.py` around lines 168 - 171, The test
test_get_node_invalid_method is calling
authenticated_nodes_client.send_custom_request with the wrong endpoint; update
the endpoint string to include the missing /v1/ui prefix so it matches the rest
of tests (use "/v1/ui/nodes/{existing_node_id}" instead of
"/nodes/{existing_node_id}"); modify the call in test_get_node_invalid_method
(and ensure it still uses the existing_node_id and method variables) so the
request targets the correct API path.
| def test_list_nodes_without_auth_token(self, authenticated_nodes_client): | ||
| token = authenticated_nodes_client.token | ||
| authenticated_nodes_client.token = None | ||
|
|
||
| response = authenticated_nodes_client.list_nodes() | ||
|
|
||
| assert response.status_code == 401, f"Expected 401, got {response.status_code}" | ||
| ErrorResponse(**response.json()) | ||
| authenticated_nodes_client.token = token |
There was a problem hiding this comment.
Token restoration should use try/finally pattern.
If the assertion fails, the token won't be restored. This pattern is repeated in multiple tests in this class. Consider using a context manager or try/finally for token manipulation tests.
🐛 Proposed fix
def test_list_nodes_without_auth_token(self, authenticated_nodes_client):
token = authenticated_nodes_client.token
- authenticated_nodes_client.token = None
-
- response = authenticated_nodes_client.list_nodes()
-
- assert response.status_code == 401, f"Expected 401, got {response.status_code}"
- ErrorResponse(**response.json())
- authenticated_nodes_client.token = token
+ try:
+ authenticated_nodes_client.token = None
+ response = authenticated_nodes_client.list_nodes()
+ assert response.status_code == 401, f"Expected 401, got {response.status_code}"
+ ErrorResponse(**response.json())
+ finally:
+ authenticated_nodes_client.token = tokenThe same pattern applies to test_list_nodes_with_invalid_token, test_list_nodes_with_wrong_auth_type, and test_list_nodes_with_too_long_token.
🤖 Prompt for AI Agents
In `@tests/api/test_nodes_list.py` around lines 75 - 83, The tests
(test_list_nodes_without_auth_token, test_list_nodes_with_invalid_token,
test_list_nodes_with_wrong_auth_type, test_list_nodes_with_too_long_token)
temporarily mutate authenticated_nodes_client.token and currently restore it
only after assertions, which can leave the token mutated if an assertion fails;
wrap the token modification and request/assertion in a try/finally (or implement
a small context manager) so that authenticated_nodes_client.token is always
restored in the finally block, ensuring token restoration regardless of test
failures.
There was a problem hiding this comment.
Nope, tests designed to fail. Absolutely no need to turn tests into spaghetti-code.
There was a problem hiding this comment.
@0xc7c5, understood! I respect your decision to keep the test code straightforward. The simpler pattern works fine for your use case.
✏️ Learnings added
Learnt from: 0xc7c5
Repo: chainstack/cp-autotests PR: 4
File: tests/api/test_nodes_list.py:75-83
Timestamp: 2026-01-27T15:25:15.282Z
Learning: In the cp-autotests repository, tests that temporarily modify client state (like authenticated_nodes_client.token) should use a simple save-modify-restore pattern without try/finally blocks, as the team prefers to keep test code simple and considers try/finally unnecessary complexity ("spaghetti-code") for these scenarios.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: 0xc7c5
Repo: chainstack/cp-autotests PR: 1
File: tests/api/test_auth_refresh.py:187-195
Timestamp: 2026-01-12T13:38:48.476Z
Learning: In tests/api/test_auth_refresh.py, the header manipulation pattern `headers = client.headers.copy()` followed by `client.headers["Authorization"] = ...` and later `client.headers = headers` is correct: it backs up, modifies the client's headers directly, then restores them.
| @allure.title("Delete node endpoint has security headers") | ||
| @allure.severity(allure.severity_level.NORMAL) | ||
| def test_delete_node_security_headers(self, authenticated_nodes_client, existing_node_id): | ||
| response = authenticated_nodes_client.schedule_delete_node(existing_node_id) | ||
|
|
||
| self._verify_security_headers(response) |
There was a problem hiding this comment.
Missing status code assertion before verifying headers.
Unlike other tests in this class, test_delete_node_security_headers doesn't assert the response status code before verifying security headers. This makes it unclear whether the test is validating success or error response headers.
🐛 Proposed fix
def test_delete_node_security_headers(self, authenticated_nodes_client, existing_node_id):
response = authenticated_nodes_client.schedule_delete_node(existing_node_id)
+ assert response.status_code == 200, f"Expected 200, got {response.status_code}"
self._verify_security_headers(response)📝 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.
| @allure.title("Delete node endpoint has security headers") | |
| @allure.severity(allure.severity_level.NORMAL) | |
| def test_delete_node_security_headers(self, authenticated_nodes_client, existing_node_id): | |
| response = authenticated_nodes_client.schedule_delete_node(existing_node_id) | |
| self._verify_security_headers(response) | |
| def test_delete_node_security_headers(self, authenticated_nodes_client, existing_node_id): | |
| response = authenticated_nodes_client.schedule_delete_node(existing_node_id) | |
| assert response.status_code == 200, f"Expected 200, got {response.status_code}" | |
| self._verify_security_headers(response) |
🤖 Prompt for AI Agents
In `@tests/api/test_nodes_security.py` around lines 36 - 41, Add an explicit
status code assertion before verifying headers in
test_delete_node_security_headers: after calling
authenticated_nodes_client.schedule_delete_node(existing_node_id) assert the
expected HTTP status (e.g., assert response.status_code == 204 — match the
status used by other delete tests) and then call
self._verify_security_headers(response); update the asserted value to whatever
other tests in this file use for deletes if different.
There was a problem hiding this comment.
Because test designed to validate header, not status code. Status code checked in another test.
| def test_unauthorized_response_security_headers(self, authenticated_nodes_client): | ||
| token = authenticated_nodes_client.token | ||
| authenticated_nodes_client.token = None | ||
|
|
||
| response = authenticated_nodes_client.list_nodes() | ||
|
|
||
| assert response.status_code == 401 | ||
| self._verify_security_headers(response) | ||
| authenticated_nodes_client.token = token |
There was a problem hiding this comment.
Token restoration not guaranteed if assertion fails.
If response.status_code is not 401 or _verify_security_headers raises an assertion error, the original token is never restored, leaving authenticated_nodes_client in a broken state for subsequent tests (though since this is a session-scoped fixture, it could affect other tests in the session).
🐛 Proposed fix using try/finally
def test_unauthorized_response_security_headers(self, authenticated_nodes_client):
token = authenticated_nodes_client.token
- authenticated_nodes_client.token = None
-
- response = authenticated_nodes_client.list_nodes()
-
- assert response.status_code == 401
- self._verify_security_headers(response)
- authenticated_nodes_client.token = token
+ try:
+ authenticated_nodes_client.token = None
+ response = authenticated_nodes_client.list_nodes()
+ assert response.status_code == 401
+ self._verify_security_headers(response)
+ finally:
+ authenticated_nodes_client.token = token🤖 Prompt for AI Agents
In `@tests/api/test_nodes_security.py` around lines 52 - 60, The test
test_unauthorized_response_security_headers can leave
authenticated_nodes_client.token unset if an assertion fails; wrap the token
removal and assertions in a try/finally so the original token is always
restored: save token from authenticated_nodes_client.token, set
authenticated_nodes_client.token = None, perform response =
authenticated_nodes_client.list_nodes() and the assertions (status code and call
to _verify_security_headers) inside the try, and in the finally block restore
authenticated_nodes_client.token = token to guarantee restoration even on
failures.
| def _teardown(self): | ||
| for node_id in self.nodes_list: | ||
| self.schedule_delete_node(node_id) |
There was a problem hiding this comment.
_teardown called once after all tests gone, so nod list modifications
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/api/test_nodes_get.py`:
- Around line 206-214: The test_get_node_with_wrong_auth_type currently mutates
authenticated_nodes_client.token which leads client code to send "Bearer Basic
..." — instead, copy authenticated_nodes_client.headers, replace its
"Authorization" value with "Basic " +
base64.b64encode(original_authorization.encode()).decode(), call
authenticated_nodes_client.get_node(existing_node_id), assert 401 and validate
via ErrorResponse(**response.json()), then restore
authenticated_nodes_client.headers to the original copy; this ensures the
Authorization header is properly formed and isolated for the test.
🧹 Nitpick comments (4)
tests/api/test_nodes_list.py (2)
100-108: Preserve other headers when testing Basic auth.The current implementation replaces the entire headers dictionary, which could remove other necessary headers (e.g.,
Content-Type, custom headers). Instead, update only theAuthorizationkey:♻️ Proposed fix
def test_list_nodes_with_wrong_auth_type(self, authenticated_nodes_client): headers = authenticated_nodes_client.headers.copy() - authenticated_nodes_client.headers = {"Authorization": "Basic " + base64.b64encode(headers["Authorization"].encode()).decode()} + authenticated_nodes_client.headers["Authorization"] = "Basic " + base64.b64encode(headers["Authorization"].encode()).decode() response = authenticated_nodes_client.list_nodes() assert response.status_code == 401, f"Expected 401, got {response.status_code}" ErrorResponse(**response.json()) authenticated_nodes_client.headers = headers
110-120: Response assertion may be brittle.The string match
"Request Header Or Cookie Too Large"depends on the specific server's error message format. Different servers or configurations may return different messages (e.g., nginx vs. application-level error). Consider making the assertion more flexible, or at minimum, ensure this matches your server's documented behavior.Additionally, some servers return
431 Request Header Fields Too Largeor413 Payload Too Largeinstead of400for oversized headers.tests/api/test_nodes_get.py (2)
26-26: Remove extraneousfprefix from string literal.The f-string has no placeholders, so the
fprefix is unnecessary.Proposed fix
- assert node.id == existing_node_id, f"Node ID mismatch" + assert node.id == existing_node_id, "Node ID mismatch"
149-162: Test includes XSS payload but is named SQL injection.The test
test_get_node_sql_injectionincludes an XSS payload (<script>alert('xss')</script>) which isn't SQL injection. Consider renaming totest_get_node_malicious_inputor splitting into separate tests for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/api/test_nodes_create.py`:
- Around line 126-150: The test shares authenticated_nodes_client across threads
but NodesAPIClient.create_node mutates self.nodes_list without synchronization;
add thread-safety by introducing a threading.Lock on the client and guarding all
mutations of self.nodes_list. Specifically, in the NodesAPIClient class add
self._nodes_list_lock = threading.Lock() in __init__ (import threading) and wrap
the code paths in create_node (and any other methods that append/remove from
self.nodes_list) with with self._nodes_list_lock: to ensure atomic updates to
self.nodes_list.
🧹 Nitpick comments (3)
control_panel/node.py (1)
20-22: Remove trailing blank lines.There are extra blank lines at the end of the file that should be cleaned up.
clients/api_client.py (1)
179-186: Misleading type hint and generic exception usage.
The type hint
expected_status: NodeStateis incorrect - callers pass string constants likeNodeState.RUNNING, notNodeStateinstances. Should bestr.Line 186 raises a generic
Exceptioninstead of a custom or built-in timeout exception.♻️ Proposed fix
- def _wait_node_until_status(self, node_id: str, expected_status: NodeState, timeout: int = 60): + def _wait_node_until_status(self, node_id: str, expected_status: str, timeout: int = 60): start_time = time.time() while time.time() - start_time < timeout: response = self.get_node(node_id) if response.json()["status"] == expected_status: return time.sleep(1) - raise Exception(f"Node {node_id} is not {expected_status} after {timeout} seconds") + raise TimeoutError(f"Node {node_id} did not reach status '{expected_status}' within {timeout}s")tests/api/test_nodes_create.py (1)
69-96: Remove unused variables.
create_responseis assigned but never used in both tests, as flagged by static analysis.♻️ Proposed fix
assert response.status_code == 201, f"Expected 201, got {response.status_code}" - create_response = CreateNodeResponse(**response.json()) + CreateNodeResponse(**response.json()) # Validates schema `@allure.title`("Create node with nested override values") `@allure.severity`(allure.severity_level.NORMAL) def test_create_node_with_nested_overrides(self, authenticated_nodes_client, valid_eth_preset_instance_id): # ... override_values setup ... assert response.status_code == 201, f"Expected 201, got {response.status_code}" - create_response = CreateNodeResponse(**response.json()) + CreateNodeResponse(**response.json()) # Validates schema
|
Hey! Couple of questions. |
For now its just for local run, but there is a task in my backlog to implement tests into CI piplines. Thus in the near future we'll have a two sets of tests -> smoke for PR's validation and nightlies for a full regression. |
clients/api_client.py
Outdated
| for node_id in self.nodes_list: | ||
| self.schedule_delete_node(node_id) | ||
|
|
||
| def _wait_node_until_status(self, node_id: str, expected_status: NodeState, timeout: int = 60): |
There was a problem hiding this comment.
do we want to add @allure.step ? + timeout logging
p.s. consider extracting sleep(1) and timeout=60 to class constants for configurability
There was a problem hiding this comment.
do we want to add @allure.step ? + timeout logging p.s. consider extracting sleep(1) and timeout=60 to class constants for configurability
That method is kinda service method, so TBH see no need to logging it. Since our nodes bootstrapping ~30 secs or so we risk getting a log full of wait messages which is not so good for reading it.
The rest is updated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@clients/api_client.py`:
- Around line 181-189: The method _wait_node_until_status currently uses
self.node_status_timeout as a default argument which raises a NameError at class
definition; change the signature to set timeout: int | None = None (or simply
timeout=None) and inside _wait_node_until_status initialize timeout =
self.node_status_timeout if timeout is None before using it, keeping the rest of
the logic the same; update any type hinting accordingly and ensure you reference
_wait_node_until_status and the attribute node_status_timeout when making the
change.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.