[serve] move test from test_grpc to test_proxy #53933
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR reorganizes gRPC proxy tests by moving the draining-node scenario into test_proxy.py and cleaning up/renaming related tests in test_grpc.py.
- Adds a new
test_grpc_proxy_on_draining_nodesintest_proxy.pyto cover draining behavior. - Removes the same test from
test_grpc.pyand renames existing gRPC test functions to drop “proxy” suffix. - Cleans up imports in
test_grpc.pyto reflect the moved test and updated names.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/ray/serve/tests/test_proxy.py | Adds test_grpc_proxy_on_draining_nodes with supporting imports and logic. |
| python/ray/serve/tests/test_grpc.py | Deletes the moved test, renames functions (dropping “proxy”), and updates imports. |
Comments suppressed due to low confidence (1)
python/ray/serve/tests/test_grpc.py:121
- [nitpick] The docstring still refers to the 'gRPC proxy' after renaming the test; update it to match the new function name (
test_grpc_routing_without_metadata) for consistency.
"""Test metadata are not required when calling gRPC proxy with only one app.
|
|
||
| # Setup worker gRPC proxy to be pointing to port 9001. Head node gRPC proxy will | ||
| # continue to be pointing to the default port 9000. | ||
| os.environ["TEST_WORKER_NODE_GRPC_PORT"] = str(worker_node_grpc_port) |
There was a problem hiding this comment.
Consider using pytest's monkeypatch.setenv to set this environment variable and ensure it’s automatically restored after the test, avoiding side effects on other tests.
| os.environ["TEST_WORKER_NODE_GRPC_PORT"] = str(worker_node_grpc_port) | |
| monkeypatch.setenv("TEST_WORKER_NODE_GRPC_PORT", str(worker_node_grpc_port)) |
Copilot uses AI. Check for mistakes.
| @@ -89,5 +99,134 @@ def test_set_timeout_keep_alive_in_both_config_and_env( | |||
| ) | |||
|
|
|||
|
|
|||
| def test_grpc_proxy_on_draining_nodes(ray_cluster): | |||
There was a problem hiding this comment.
The ray_cluster fixture is not used inside the test body; removing it from the signature will prevent confusion about unused fixtures.
| def test_grpc_proxy_on_draining_nodes(ray_cluster): | |
| def test_grpc_proxy_on_draining_nodes(): |
Copilot uses AI. Check for mistakes.
Why are these changes needed?
Move serve test to a different file.