Copilot commented on code in PR #336:
URL: https://github.com/apache/hugegraph-ai/pull/336#discussion_r3263463530
##########
hugegraph-python-client/src/tests/api/test_gremlin.py:
##########
@@ -125,12 +125,28 @@ def test_set_up_class_reraises_non_probe_failures(self):
self.assertFalse(TestGremlin.skip_gremlin_tests)
- def test_set_up_class_skips_when_gremlin_probe_returns_not_found(self):
+ def test_set_up_class_reraises_probe_errors(self):
+ # When the gremlin client raises NotFoundError during operations,
+ # do not silently skip — surface the error so tests fail.
with mock.patch(f"{TestGremlin.__module__}.ClientUtils") as
client_utils_cls:
client = client_utils_cls.return_value
client.gremlin = mock.Mock()
client.gremlin.exec.side_effect = NotFoundError("404 Not Found")
- TestGremlin.setUpClass()
+ with self.assertRaises(NotFoundError):
+ TestGremlin.setUpClass()
- self.assertTrue(TestGremlin.skip_gremlin_tests)
+ self.assertFalse(TestGremlin.skip_gremlin_tests)
Review Comment:
This test sets `client.gremlin.exec.side_effect = NotFoundError(...)`, but
after this PR `setUpClass` no longer calls `gremlin.exec` at all (the probe was
removed). The mocked side effect is therefore never triggered, `setUpClass`
completes without raising, and `assertRaises(NotFoundError)` will fail. The
test name and intent ("reraises probe errors") also no longer match reality
since there is no probe. Consider either removing this test or rewriting it to
assert that `setUpClass` no longer performs a gremlin probe (e.g., assert
`client.gremlin.exec` is not called).
##########
hugegraph-python-client/src/tests/api/test_gremlin.py:
##########
@@ -125,12 +125,28 @@ def test_set_up_class_reraises_non_probe_failures(self):
self.assertFalse(TestGremlin.skip_gremlin_tests)
- def test_set_up_class_skips_when_gremlin_probe_returns_not_found(self):
+ def test_set_up_class_reraises_probe_errors(self):
+ # When the gremlin client raises NotFoundError during operations,
+ # do not silently skip — surface the error so tests fail.
with mock.patch(f"{TestGremlin.__module__}.ClientUtils") as
client_utils_cls:
client = client_utils_cls.return_value
client.gremlin = mock.Mock()
client.gremlin.exec.side_effect = NotFoundError("404 Not Found")
- TestGremlin.setUpClass()
+ with self.assertRaises(NotFoundError):
+ TestGremlin.setUpClass()
- self.assertTrue(TestGremlin.skip_gremlin_tests)
+ self.assertFalse(TestGremlin.skip_gremlin_tests)
+
+ def test_set_up_class_skips_when_env_var_set(self):
+ # Explicit opt-in skip via environment variable is supported.
+ with mock.patch(f"{TestGremlin.__module__}.ClientUtils") as
client_utils_cls:
+ client = client_utils_cls.return_value
+ client.gremlin = mock.Mock()
+ # Ensure the env var causes a SkipTest regardless of client
behavior.
+ os.environ["SKIP_GREMLIN_TESTS"] = "true"
+ try:
+ with self.assertRaises(unittest.SkipTest):
+ TestGremlin.setUpClass()
+ finally:
+ os.environ.pop("SKIP_GREMLIN_TESTS", None)
Review Comment:
Directly mutating `os.environ` can leak state if an unexpected exception
occurs between the assignment and the `try` block (and is generally discouraged
in tests). Prefer `unittest.mock.patch.dict(os.environ, {"SKIP_GREMLIN_TESTS":
"true"})` to guarantee cleanup and isolation from other tests.
--
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]
