Copilot commented on code in PR #342:
URL: https://github.com/apache/hugegraph-ai/pull/342#discussion_r3328816621
##########
hugegraph-python-client/src/tests/api/test_metric.py:
##########
@@ -70,10 +105,35 @@ def test_metrics_operations(self):
self.assertIsInstance(statistics, dict)
backend_metrics = self.metrics.get_backend_metrics()
- # In HugeGraph 1.7.0+, the backend_metrics structure changed
- # It's still a dict, but the "hugegraph" key may not exist in the same
format
- self.assertIsInstance(backend_metrics, dict)
- self.assertTrue(backend_metrics, "backend metrics should not be empty")
- # Only assert on the "hugegraph" key if it exists (for backward
compatibility)
- if "hugegraph" in backend_metrics:
- self.assertGreater(len(backend_metrics["hugegraph"]), 1)
+
+ # HugeGraph 1.7.0 backend_metrics shape:
+ # { "DEFAULT-hugegraph": { "backend": str, "nodes": int, "cluster_id":
str,
+ # "servers": { "<server_name>": { <metrics> }
} } }
+ self.assertIsInstance(backend_metrics, dict, "backend_metrics should
be a dict")
+ self.assertTrue(backend_metrics, "backend_metrics should not be empty")
+
+ # Assert top-level graph key exists
+ graph_keys = list(backend_metrics.keys())
+ self.assertEqual(len(graph_keys), 1, f"Expected 1 graph key, got:
{graph_keys}")
+
+ graph_entry = backend_metrics[graph_keys[0]]
+ self.assertIsInstance(graph_entry, dict)
+
+ # Assert required top-level fields in graph entry
+ self.assertIn("backend", graph_entry, "Missing 'backend' field")
+ self.assertIn("nodes", graph_entry, "Missing 'nodes' field")
+ self.assertIn("cluster_id", graph_entry, "Missing 'cluster_id' field")
+ self.assertIn("servers", graph_entry, "Missing 'servers' field")
+ self.assertIsInstance(graph_entry["backend"], str)
+ self.assertIsInstance(graph_entry["nodes"], int)
+ self.assertIsInstance(graph_entry["servers"], dict)
+
+ # Assert server entry contains expected rocksdb metric keys
+ servers = graph_entry["servers"]
Review Comment:
Only the first server entry is validated, so a shape regression affecting
any other server in the `servers` map would be missed. Iterating over all
server entries also lets you assert each entry is a dict and produce a clearer
failure message that includes the server name.
##########
hugegraph-python-client/src/tests/api/test_metric.py:
##########
@@ -70,10 +105,35 @@ def test_metrics_operations(self):
self.assertIsInstance(statistics, dict)
backend_metrics = self.metrics.get_backend_metrics()
- # In HugeGraph 1.7.0+, the backend_metrics structure changed
- # It's still a dict, but the "hugegraph" key may not exist in the same
format
- self.assertIsInstance(backend_metrics, dict)
- self.assertTrue(backend_metrics, "backend metrics should not be empty")
- # Only assert on the "hugegraph" key if it exists (for backward
compatibility)
- if "hugegraph" in backend_metrics:
- self.assertGreater(len(backend_metrics["hugegraph"]), 1)
+
+ # HugeGraph 1.7.0 backend_metrics shape:
+ # { "DEFAULT-hugegraph": { "backend": str, "nodes": int, "cluster_id":
str,
+ # "servers": { "<server_name>": { <metrics> }
} } }
+ self.assertIsInstance(backend_metrics, dict, "backend_metrics should
be a dict")
+ self.assertTrue(backend_metrics, "backend_metrics should not be empty")
+
+ # Assert top-level graph key exists
+ graph_keys = list(backend_metrics.keys())
Review Comment:
`/metrics/backend` is a server-level endpoint (absolute path), so the
response may legitimately include *multiple* graph keys (e.g., multiple
graphs/graphspaces). Asserting `len(graph_keys) == 1` makes the test brittle
and can fail even when the backend_metrics shape is correct. Prefer asserting
the expected graph key exists (or at least one key) and selecting the entry
corresponding to the configured graph.
##########
hugegraph-python-client/src/tests/api/test_metric.py:
##########
@@ -70,10 +105,35 @@ def test_metrics_operations(self):
self.assertIsInstance(statistics, dict)
backend_metrics = self.metrics.get_backend_metrics()
- # In HugeGraph 1.7.0+, the backend_metrics structure changed
- # It's still a dict, but the "hugegraph" key may not exist in the same
format
- self.assertIsInstance(backend_metrics, dict)
- self.assertTrue(backend_metrics, "backend metrics should not be empty")
- # Only assert on the "hugegraph" key if it exists (for backward
compatibility)
- if "hugegraph" in backend_metrics:
- self.assertGreater(len(backend_metrics["hugegraph"]), 1)
+
+ # HugeGraph 1.7.0 backend_metrics shape:
+ # { "DEFAULT-hugegraph": { "backend": str, "nodes": int, "cluster_id":
str,
+ # "servers": { "<server_name>": { <metrics> }
} } }
+ self.assertIsInstance(backend_metrics, dict, "backend_metrics should
be a dict")
+ self.assertTrue(backend_metrics, "backend_metrics should not be empty")
+
+ # Assert top-level graph key exists
+ graph_keys = list(backend_metrics.keys())
+ self.assertEqual(len(graph_keys), 1, f"Expected 1 graph key, got:
{graph_keys}")
+
+ graph_entry = backend_metrics[graph_keys[0]]
+ self.assertIsInstance(graph_entry, dict)
+
+ # Assert required top-level fields in graph entry
+ self.assertIn("backend", graph_entry, "Missing 'backend' field")
+ self.assertIn("nodes", graph_entry, "Missing 'nodes' field")
+ self.assertIn("cluster_id", graph_entry, "Missing 'cluster_id' field")
+ self.assertIn("servers", graph_entry, "Missing 'servers' field")
Review Comment:
The comment above documents `cluster_id` as a `str`, but the test currently
only asserts presence and not type. Adding a type assertion makes the
structural contract deterministic and aligns with the documented shape.
##########
hugegraph-python-client/src/tests/api/test_metric.py:
##########
@@ -70,10 +105,35 @@ def test_metrics_operations(self):
self.assertIsInstance(statistics, dict)
backend_metrics = self.metrics.get_backend_metrics()
- # In HugeGraph 1.7.0+, the backend_metrics structure changed
- # It's still a dict, but the "hugegraph" key may not exist in the same
format
- self.assertIsInstance(backend_metrics, dict)
- self.assertTrue(backend_metrics, "backend metrics should not be empty")
- # Only assert on the "hugegraph" key if it exists (for backward
compatibility)
- if "hugegraph" in backend_metrics:
- self.assertGreater(len(backend_metrics["hugegraph"]), 1)
+
+ # HugeGraph 1.7.0 backend_metrics shape:
+ # { "DEFAULT-hugegraph": { "backend": str, "nodes": int, "cluster_id":
str,
+ # "servers": { "<server_name>": { <metrics> }
} } }
+ self.assertIsInstance(backend_metrics, dict, "backend_metrics should
be a dict")
+ self.assertTrue(backend_metrics, "backend_metrics should not be empty")
+
+ # Assert top-level graph key exists
+ graph_keys = list(backend_metrics.keys())
Review Comment:
`/metrics/backend` is a server-level endpoint (absolute path), so the
response may legitimately include *multiple* graph keys (e.g., multiple
graphs/graphspaces). Asserting `len(graph_keys) == 1` makes the test brittle
and can fail even when the backend_metrics shape is correct. Prefer asserting
the expected graph key exists (or at least one key) and selecting the entry
corresponding to the configured graph.
##########
hugegraph-python-client/src/tests/api/test_metric.py:
##########
@@ -70,10 +105,35 @@ def test_metrics_operations(self):
self.assertIsInstance(statistics, dict)
backend_metrics = self.metrics.get_backend_metrics()
- # In HugeGraph 1.7.0+, the backend_metrics structure changed
- # It's still a dict, but the "hugegraph" key may not exist in the same
format
- self.assertIsInstance(backend_metrics, dict)
- self.assertTrue(backend_metrics, "backend metrics should not be empty")
- # Only assert on the "hugegraph" key if it exists (for backward
compatibility)
- if "hugegraph" in backend_metrics:
- self.assertGreater(len(backend_metrics["hugegraph"]), 1)
+
+ # HugeGraph 1.7.0 backend_metrics shape:
+ # { "DEFAULT-hugegraph": { "backend": str, "nodes": int, "cluster_id":
str,
+ # "servers": { "<server_name>": { <metrics> }
} } }
+ self.assertIsInstance(backend_metrics, dict, "backend_metrics should
be a dict")
+ self.assertTrue(backend_metrics, "backend_metrics should not be empty")
+
+ # Assert top-level graph key exists
+ graph_keys = list(backend_metrics.keys())
+ self.assertEqual(len(graph_keys), 1, f"Expected 1 graph key, got:
{graph_keys}")
+
+ graph_entry = backend_metrics[graph_keys[0]]
+ self.assertIsInstance(graph_entry, dict)
+
+ # Assert required top-level fields in graph entry
+ self.assertIn("backend", graph_entry, "Missing 'backend' field")
+ self.assertIn("nodes", graph_entry, "Missing 'nodes' field")
+ self.assertIn("cluster_id", graph_entry, "Missing 'cluster_id' field")
+ self.assertIn("servers", graph_entry, "Missing 'servers' field")
Review Comment:
The comment above documents `cluster_id` as a `str`, but the test currently
only asserts presence and not type. Adding a type assertion makes the
structural contract deterministic and aligns with the documented shape.
##########
hugegraph-python-client/src/tests/api/test_metric.py:
##########
@@ -70,10 +105,35 @@ def test_metrics_operations(self):
self.assertIsInstance(statistics, dict)
backend_metrics = self.metrics.get_backend_metrics()
- # In HugeGraph 1.7.0+, the backend_metrics structure changed
- # It's still a dict, but the "hugegraph" key may not exist in the same
format
- self.assertIsInstance(backend_metrics, dict)
- self.assertTrue(backend_metrics, "backend metrics should not be empty")
- # Only assert on the "hugegraph" key if it exists (for backward
compatibility)
- if "hugegraph" in backend_metrics:
- self.assertGreater(len(backend_metrics["hugegraph"]), 1)
+
+ # HugeGraph 1.7.0 backend_metrics shape:
+ # { "DEFAULT-hugegraph": { "backend": str, "nodes": int, "cluster_id":
str,
+ # "servers": { "<server_name>": { <metrics> }
} } }
+ self.assertIsInstance(backend_metrics, dict, "backend_metrics should
be a dict")
+ self.assertTrue(backend_metrics, "backend_metrics should not be empty")
+
+ # Assert top-level graph key exists
+ graph_keys = list(backend_metrics.keys())
+ self.assertEqual(len(graph_keys), 1, f"Expected 1 graph key, got:
{graph_keys}")
+
+ graph_entry = backend_metrics[graph_keys[0]]
+ self.assertIsInstance(graph_entry, dict)
+
+ # Assert required top-level fields in graph entry
+ self.assertIn("backend", graph_entry, "Missing 'backend' field")
+ self.assertIn("nodes", graph_entry, "Missing 'nodes' field")
+ self.assertIn("cluster_id", graph_entry, "Missing 'cluster_id' field")
+ self.assertIn("servers", graph_entry, "Missing 'servers' field")
+ self.assertIsInstance(graph_entry["backend"], str)
+ self.assertIsInstance(graph_entry["nodes"], int)
+ self.assertIsInstance(graph_entry["servers"], dict)
+
+ # Assert server entry contains expected rocksdb metric keys
+ servers = graph_entry["servers"]
Review Comment:
Only the first server entry is validated, so a shape regression affecting
any other server in the `servers` map would be missed. Iterating over all
server entries also lets you assert each entry is a dict and produce a clearer
failure message that includes the server name.
--
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]
