imbajin commented on code in PR #320:
URL: https://github.com/apache/hugegraph-ai/pull/320#discussion_r3111000770
##########
hugegraph-python-client/src/tests/api/test_traverser.py:
##########
@@ -226,18 +226,16 @@ def test_traverser_operations(self):
edge_id = self.graph.getEdgeByPage("created", josh, "BOTH")[0][0]
edges_result = self.traverser.edges(edge_id.id)
- self.assertEqual(
- edges_result["edges"],
- [
- {
- "id": "S1:josh>2>>S2:lop",
- "label": "created",
- "type": "edge",
- "outV": "1:josh",
- "outVLabel": "person",
- "inV": "2:lop",
- "inVLabel": "software",
- "properties": {"city": "Beijing", "date": "2016-01-10
00:00:00.000"},
- }
- ],
- )
+ # Use the dynamically retrieved edge ID instead of hardcoding format
+ # to ensure compatibility with different HugeGraph versions (1.3.0,
1.7.0, etc.)
+ expected_edge = {
+ "id": edge_id.id,
+ "label": "created",
+ "type": "edge",
+ "outV": "1:josh",
+ "outVLabel": "person",
+ "inV": "2:lop",
+ "inVLabel": "software",
+ "properties": {"city": "Beijing", "date": "2016-01-10
00:00:00.000"},
+ }
+ self.assertEqual(edges_result["edges"], [expected_edge])
Review Comment:
⚠️ **EdgeId format changes in 1.7.0 with sub-edge labels — test may be
insufficient**
This fix correctly uses the dynamic `edge_id.id` instead of hardcoding
`"S1:josh>2>>S2:lop"`. Good.
However, HugeGraph 1.7.0 changed the EdgeId structure for **sub-edge
labels**: it now encodes both `labelId` (parent) and `subLabelId` (child) in
the ID string. For a regular edge label like `"created"` (used here), the
format is unchanged — so this specific test remains valid.
But this means: if someone later creates a test using a sub-edge label (via
the new `edge_label.parent()` API), the `id` field format will differ from the
old `S{srcV}>{labelId}>{sortKey}>S{dstV}` pattern. Consider adding a comment
noting this limitation so future contributors are aware.
##########
hugegraph-python-client/src/tests/api/test_gremlin.py:
##########
@@ -26,21 +26,40 @@
class TestGremlin(unittest.TestCase):
client = None
gremlin = None
+ skip_gremlin_tests = False
@classmethod
def setUpClass(cls):
- cls.client = ClientUtils()
- cls.client.clear_graph_all_data()
- cls.gremlin = cls.client.gremlin
- cls.client.init_property_key()
- cls.client.init_vertex_label()
- cls.client.init_edge_label()
+ try:
+ cls.client = ClientUtils()
+ cls.gremlin = cls.client.gremlin
+ cls.client.clear_graph_all_data()
+ cls.client.init_property_key()
+ cls.client.init_vertex_label()
+ cls.client.init_edge_label()
+ # Test if gremlin endpoint is available by executing a simple query
+ cls.gremlin.exec("1 + 1")
+ except NotFoundError as e:
+ # Skip gremlin tests if endpoint not available in server
+ if "404" in str(e) or "Not Found" in str(e):
+ cls.skip_gremlin_tests = True
+ else:
+ raise
+ except Exception as e:
+ # Also skip if any other exception occurs during setup
+ if "404" in str(e) or "Not Found" in str(e):
+ cls.skip_gremlin_tests = True
+ else:
+ raise
@classmethod
Review Comment:
⚠️ **Overly broad exception catching silently swallows setup failures**
The second `except Exception` block catches ALL exceptions and only skips if
"404" appears in the string. This means errors from `init_property_key()`,
`init_vertex_label()`, `init_edge_label()` (e.g., auth failures, schema
conflicts, network errors) will be silently swallowed and all gremlin tests
will appear as "skipped" instead of "failed".
```suggestion
except NotFoundError as e:
# Skip only if the gremlin endpoint itself is unavailable (404)
if "404" in str(e) or "Not Found" in str(e):
cls.skip_gremlin_tests = True
else:
raise
# Let all other setup errors (auth, schema, network) propagate as
real failures
```
Remove the second broad `except Exception` block entirely — only catch
`NotFoundError` for the gremlin endpoint 404 case.
##########
.github/workflows/check-dependencies.yml:
##########
@@ -13,7 +13,9 @@ jobs:
- name: 'Checkout Repository'
uses: actions/checkout@v4
- name: 'Dependency Review'
+ if: github.event.pull_request.head.repo.full_name == github.repository
uses: actions/dependency-review-action@v4
+ continue-on-error: true
Review Comment:
🧹 **`continue-on-error: true` makes the security gate ineffective**
The `if: github.event.pull_request.head.repo.full_name == github.repository`
guard already skips the step for fork PRs. Adding `continue-on-error: true` on
top means the dependency check will pass even when it detects vulnerable
dependencies in first-party PRs — defeating the purpose of the security gate.
If the intent is purely to handle forks, the `if` condition is sufficient.
Remove `continue-on-error: true`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
