Uh oh!
There was an error while loading. Please reload this page.
test: move TcpProxy to tests/tcp_proxy.py to fix collection - #966
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
dawmd
commented
Aug 4, 2026
The failure looks like #580: Failure===================================FAILURES===================================___________HostConnectionTests.test_successful_wait_for_connection____________self=<tests.unit.test_host_connection_pool.HostConnectionTeststestMethod=test_successful_wait_for_connection>deftest_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=connpool=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)
assert1==conn.in_flightdefget_second_conn():
c, request_id=pool.borrow_connection(1.0)
assertconniscpool.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.HostConnectionobjectat0x000000004310d948>connection=<HashableMockname='mock.cluster.connection_factory()'spec='Connection'id='1125178344'>stream_was_orphaned=Falsedefreturn_connection(self, connection, stream_was_orphaned=False):
ifnotstream_was_orphaned:
withconnection.lock:
connection.in_flight-=1withself._stream_available_condition:
self._stream_available_condition.notify()
ifconnection.is_defunctorconnection.is_closed:
ifconnection.signaled_errorandnotself.shutdown_on_error:
returnis_down=Falseifnotconnection.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=Trueifself.shutdown_on_errorandnotis_down:
is_down=Trueifis_down:
self.shutdown()
self._session.cluster.on_down(self.host, is_host_addition=False)
else:
connection.close()
withself._lock:
ifself.is_shutdown:
returnself._connections.pop(connection.features.shard_id, None)
ifself._is_replacing:
returnself._is_replacing=Trueself._session.submit(self._replace, connection)>elifconnectioninself._trash:
^^^^^^^^^^^^^^^^^^^^^^^^^ETypeError: __hash__methodshouldreturnanintegernot'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
dawmd
commented
Aug 4, 2026
Sent an empty update to retrigger CI. |
dawmd
commented
Aug 4, 2026
@scylladb/python-driver-maint please consider merging |
Uh oh!
There was an error while loading. Please reload this page.
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.