imbajin commented on code in PR #325:
URL: https://github.com/apache/hugegraph-ai/pull/325#discussion_r3207330378


##########
hugegraph-python-client/src/tests/api/test_auth.py:
##########
@@ -40,6 +41,9 @@ def setUpClass(cls):
                 cls.skip_auth_tests = True
             else:
                 raise
+        except requests.exceptions.RequestException:

Review Comment:
   ‼️ With the auth-path fix in this PR, the whole `try: list_users() / except 
NotFoundError: skip_auth_tests = True` probe (L36-42) is now dead code against 
a correctly-configured HugeGraph 1.7.0+ server — and the new `RequestException 
→ skip` branch below extends the same anti-pattern to connection errors.
   
   The failure mode this creates: if a future change regresses the auth paths 
(e.g. somebody reverts to `/auth/users`), the server returns 404, 
`skip_auth_tests` flips to `True`, every integration test in this class calls 
`self.skipTest(...)` in `setUp`, and CI reports `5 skipped` instead of `5 
failed`. Green CI, silently broken auth.
   
   Suggested cleanup (pick one):
   
   ```suggestion
       @classmethod
       def setUpClass(cls):
           cls.client = ClientUtils()
           cls.auth = cls.client.auth
   
       @classmethod
       def tearDownClass(cls):
           cls.client.clear_graph_all_data()
   
       def setUp(self):
   ```
   
   This removes the probe entirely and lets `list_users()` in individual tests 
fail loudly with the actual 404/connection error. If you want to keep a 
preflight for clearer diagnostics, replace the skip with an assertion:
   
   ```python
   except NotFoundError as e:
       if "404" in str(e) or "Not Found" in str(e):
           raise AssertionError(
               "Auth path regression: server returned 404 on list_users(). "
               "Check graphspace-scoped routing in huge_router.py."
           ) from e
       raise
   except requests.exceptions.RequestException as e:
       raise AssertionError(f"HugeGraph server unreachable: {e}") from e
   ```
   
   Either way, also drop the `skip_auth_tests` class attribute and its 
reference in `setUp`/`tearDownClass`.



-- 
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]

Reply via email to