test: move TcpProxy to tests/tcp_proxy.py to fix collection - #966
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe TCP proxy moved to Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Moves TcpProxy into a dependency-free test helper module, preventing unit-test collection from importing CCM-dependent integration code and resolving issue #965.
Changes:
- Extracts
TcpProxyintotests/tcp_proxy.py. - Updates unit and integration tests to import the shared helper.
- Removes obsolete environment setup and unused imports.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/tcp_proxy.py |
Adds the standalone TCP proxy helper. |
tests/unit/test_tcp_proxy.py |
Imports the helper without integration dependencies. |
tests/integration/standard/test_client_routes.py |
Uses the extracted helper and removes its former implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/tcp_proxy.py`:
- Around line 88-91: Make retarget() update target_host and target_port while
holding _lock, and update _handle_new_connection() to capture both values under
the same lock before calling connect(). Release the lock before any connection
attempt so the snapshot is consistent without blocking connection establishment.
- Around line 72-80: Update _handle_new_connection so the backend
target_sock.connect attempt is bounded and observes shutdown state, rather than
blocking indefinitely on the listener thread. Ensure a shutdown can interrupt or
terminate the pending connection and that the socket is tracked or closed
consistently before stop() joins the listener and client connections. Apply the
same behavior to the corresponding connection path around the second referenced
location.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 06d1f484-c8e7-4f61-8746-91297e1874c4
📒 Files selected for processing (3)
tests/integration/standard/test_client_routes.pytests/tcp_proxy.pytests/unit/test_tcp_proxy.py
Author
|
The failure looks like #580: Failure =================================== FAILURES ===================================
___________ HostConnectionTests.test_successful_wait_for_connection ____________
self = <tests.unit.test_host_connection_pool.HostConnectionTests testMethod=test_successful_wait_for_connection>
def test_successful_wait_for_connection(self):
host = Mock(spec=Host, address='ip1')
session = self.make_session()
conn = HashableMock(spec=Connection, in_flight=0, is_defunct=False, is_closed=False, max_request_id=100,
lock=Lock())
session.cluster.connection_factory.return_value = conn
pool = self.PoolImpl(host, HostDistance.LOCAL, session)
session.cluster.connection_factory.assert_called_once_with(host.endpoint, on_orphaned_stream_released=pool.on_orphaned_stream_released)
pool.borrow_connection(timeout=0.01)
assert 1 == conn.in_flight
def get_second_conn():
c, request_id = pool.borrow_connection(1.0)
assert conn is c
pool.return_connection(c)
t = Thread(target=get_second_conn)
t.start()
> pool.return_connection(conn)
/project/tests/unit/test_host_connection_pool.py:105:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <cassandra.pool.HostConnection object at 0x000000004310d948>
connection = <HashableMock name='mock.cluster.connection_factory()' spec='Connection' id='1125178344'>
stream_was_orphaned = False
def return_connection(self, connection, stream_was_orphaned=False):
if not stream_was_orphaned:
with connection.lock:
connection.in_flight -= 1
with self._stream_available_condition:
self._stream_available_condition.notify()
if connection.is_defunct or connection.is_closed:
if connection.signaled_error and not self.shutdown_on_error:
return
is_down = False
if not connection.signaled_error:
log.debug("Defunct or closed connection (%s) returned to pool, potentially "
"marking host %s as down", id(connection), self.host)
is_down = self.host.signal_connection_failure(connection.last_error)
connection.signaled_error = True
if self.shutdown_on_error and not is_down:
is_down = True
if is_down:
self.shutdown()
self._session.cluster.on_down(self.host, is_host_addition=False)
else:
connection.close()
with self._lock:
if self.is_shutdown:
return
self._connections.pop(connection.features.shard_id, None)
if self._is_replacing:
return
self._is_replacing = True
self._session.submit(self._replace, connection)
> elif connection in self._trash:
^^^^^^^^^^^^^^^^^^^^^^^^^
E TypeError: __hash__ method should return an integer not 'MagicMock' |
tests/unit/test_tcp_proxy.py, added in d99dc46, imported its subject (TcpProxy) from tests/integration/standard/test_client_routes.py, which transitively imports tests/integration/__init__.py. That module guards its ccmlib imports with try/except ImportError, but then unconditionally declares `class Cassandra41CCMCluster(CCMCluster)` at module level, so on any environment without ccmlib installed the import fails with: NameError: name 'CCMCluster' is not defined This broke test collection consistently on the windows-2022 job, where ccmlib is absent. The latent defect in tests/integration/__init__.py predates d99dc46; that commit merely became the first unit test to import tests.integration and thus the first to expose it. TcpProxy is a plain socket-based helper -- it depends only on socket, select and threading, and needs neither CCM nor a running Cassandra/Scylla cluster -- so it does not belong behind that import. Move it verbatim into a new tests/tcp_proxy.py and import it from both call sites: - tests/integration/standard/test_client_routes.py now imports TcpProxy from tests.tcp_proxy; its `select` and `socket` imports, used only by the moved class, are dropped. - tests/unit/test_tcp_proxy.py imports from tests.tcp_proxy and no longer needs its os.environ.setdefault("CASSANDRA_VERSION", ...) shim, which existed solely to get tests.integration's module-level version parsing to succeed. The shim and the docstring paragraph explaining it are removed. The class body is byte-identical to the original; only the new module's license header, docstring and imports are new. No driver code is touched and no test behavior changes. Validation: - pytest tests/unit/test_tcp_proxy.py: 2 passed with neither CASSANDRA_VERSION nor SCYLLA_VERSION set, i.e. the unit test no longer imports tests.integration at all. - tests/integration/standard/test_client_routes.py compiles clean with no imports left unused. Fixes: scylladb#965
Author
|
Sent an empty update to retrigger CI. |
Author
|
@scylladb/python-driver-maint please consider merging |
Lorak-mmk
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tests/unit/test_tcp_proxy.py, added in d99dc46, imported its subject (TcpProxy) from tests/integration/standard/test_client_routes.py, which transitively imports tests/integration/init.py. That module guards its ccmlib imports with try/except ImportError, but then unconditionally declares
class Cassandra41CCMCluster(CCMCluster)at module level, so on any environment without ccmlib installed the import fails with:This broke test collection consistently on the windows-2022 job, where ccmlib is absent. The latent defect in tests/integration/init.py predates d99dc46; that commit merely became the first unit test to import tests.integration and thus the first to expose it.
TcpProxy is a plain socket-based helper -- it depends only on socket, select and threading, and needs neither CCM nor a running Cassandra/Scylla cluster -- so it does not belong behind that import. Move it verbatim into a new tests/tcp_proxy.py and import it from both call sites:
selectandsocketimports, used only by the moved class, are dropped.The class body is byte-identical to the original; only the new module's license header, docstring and imports are new. No driver code is touched and no test behavior changes.
Validation:
Fixes: #965
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.