PYTHON-5781 Coverage increase for network_layer.py#2774
PYTHON-5781 Coverage increase for network_layer.py#2774aclark4life wants to merge 7 commits intomongodb:masterfrom
network_layer.py#2774Conversation
There was a problem hiding this comment.
Pull request overview
Adds a dedicated unit test suite to increase coverage of pymongo/network_layer.py, focusing on logic that can be validated without live sockets or a running MongoDB server.
Changes:
- Introduces
test/test_network_layer.pywith unit tests forPyMongoProtocolstate transitions and message parsing helpers. - Adds delegation/behavior tests for
NetworkingInterfaceBase,NetworkingInterface, andAsyncNetworkingInterface. - Adds tests for small helper functions (
sendall,_async_socket_receive) using mocks.
aclark4life
changed the title
aclark4life
force-pushed
the
PYTHON-5781
branch
7 times, most recently
from
c6991e0 to
72fd1a4
Compare
|
|
||
|
|
||
| class TestSendall(AsyncUnitTest): | ||
| def test_delegates_to_sock_sendall(self): |
There was a problem hiding this comment.
What's the purpose of this test? Testing that our wrapper calls what it wraps doesn't seem to test useful behavior.
|
|
||
|
|
||
| class TestNetworkingInterfaceBase(AsyncUnitTest): | ||
| def setUp(self): |
There was a problem hiding this comment.
| def setUp(self): | |
| def asyncSetUp(self): |
| mock_socket.sendall.assert_called_once_with(b"hello") | ||
|
|
||
|
|
||
| class TestNetworkingInterfaceBase(AsyncUnitTest): |
There was a problem hiding this comment.
This class doesn't need to be synchro'd since it's testing code common to both APIs.
| _ = self.base.sock | ||
|
|
||
|
|
||
| class TestNetworkingInterface(AsyncUnitTest): |
There was a problem hiding this comment.
Are we gaining useful coverage by testing that all of NetworkingInterface's methods delegate to the underlying socket instead of testing the actual result of these calls?
| self.assertIs(self.network_interface.sock, self.mock_socket) | ||
|
|
||
|
|
||
| if not _IS_SYNC: |
There was a problem hiding this comment.
All of this code will be duplicated but unused in the synchronized version of this file. A better pattern is to put all of the async-specific tests that don't produce a meaningful synchronous version into a test/asynchronous/test_async_network_layer.py file that is not synchro'd.
This file would then become tests common to both APIs (like the NetworkingInterfaceBase tests) as well as the synchronous tests. Then there would be no code duplication and more isolated test modules.
| self.assertIsNone(protocol.gettimeout) | ||
|
|
||
| async def test_normal_op_msg(self): | ||
| header = _make_header(32, 1, 99, 2013) |
There was a problem hiding this comment.
Using keyword arguments for these _make_header calls would be more readable (less magic numbers).
| self.assertEqual(op_code, 1) | ||
| self.assertFalse(expecting_compression) | ||
|
|
||
| async def test_compression_header_returns_op_code_and_compressor_id(self): |
There was a problem hiding this comment.
Should this be named something like test_compression_header_snappy_compressor_id for consistency with the following test?
| with self.assertRaises(OSError) as ctx: | ||
| await future | ||
| self.assertIn("connection reset", str(ctx.exception)) |
There was a problem hiding this comment.
| with self.assertRaises(OSError) as ctx: | |
| await future | |
| self.assertIn("connection reset", str(ctx.exception)) | |
| with self.assertRaisesRegex(OSError, "connection reset"): | |
| await future |
| self.assertIn("connection reset", str(ctx.exception)) | ||
|
|
||
| class TestAsyncSocketReceive(AsyncUnitTest): | ||
| async def test_reads_full_data_in_one_call(self): |
There was a problem hiding this comment.
This seems to be primarily testing the behavior of loop.sock_recv_into(), which is a Python built-in method. Are we gaining useful coverage with this test?
| result = await _async_socket_receive(mock_socket, length, loop) | ||
| self.assertEqual(bytes(result), data) | ||
|
|
||
| async def test_reads_data_in_multiple_chunks(self): |
Add test/test_network_layer.py with 56 unit tests covering: - PyMongoProtocol: process_header (including ProtocolError paths), process_compression_header, get_buffer (all state branches), buffer_updated (state machine), close/connection_lost lifecycle - NetworkingInterfaceBase: abstract method NotImplementedError raises - NetworkingInterface: socket delegation methods - AsyncNetworkingInterface: transport/protocol delegation - sendall: trivial delegation - _async_socket_receive: success and connection-closed paths
- Fix mypy type error in test_network_layer.py - Use 'from test import unittest', remove self-evident docstrings from helpers - Fix inaccurate comment: second field in compression header is uncompressed_size
- Add test/asynchronous/test_network_layer.py as the async source of truth with AsyncUnitTest and _IS_SYNC = False - Regenerate test/test_network_layer.py via synchro from the async source - Register test_network_layer.py in synchro converted_tests (alpha order) - Add "AsyncMock": "MagicMock" replacement after AsyncMockPool to avoid substring collision in translate_docstrings
Applying feedback from PYTHON-5784 to all open codecov PRs
aclark4life
changed the title
aclark4life
changed the title
aclark4life
changed the title
network_layer.py
aclark4life
changed the title
network_layer.pynetwork_layer.py
aclark4life
changed the title
network_layer.pynetwork_layer.py
aclark4life
changed the title
network_layer.pynetwork_layer.py
PYTHON-5781
Changes in this PR
Adds unit tests for
pymongo/network_layer.py, split into two files:test/asynchronous/test_network_layer.py(synchro'd totest/test_network_layer.py) — tests for code shared between the sync and async APIs.test/asynchronous/test_async_network_layer.py(not synchro'd; added toasync_only_test()intools/synchro.py) — tests for asyncio-specific code (PyMongoProtocol,_async_socket_receive).Tests cover:
Shared (
NetworkingInterfaceBase)NotImplementedErrorongettimeout,settimeout,close,is_closing,get_conn,sock.Async-only — wire-protocol parsing (
PyMongoProtocol)process_header()— OP_MSG, OP_REPLY, OP_COMPRESSED, and the threeProtocolErrorpaths (compressed length ≤ 25, uncompressed length ≤ 16, length > max message size).process_compression_header()— snappy and zlib compressor IDs.buffer_updated()— completes pending future on full message.close()/connection_lost()— abort transport, exception propagation to pending futures, idempotency on repeatedconnection_lost.gettimeout/settimeout.Async-only —
_async_socket_receive()OSError("connection closed")when the socket returns 0 bytes.Test Plan
Added unit tests using
MagicMocktransports/sockets and theAsyncUnitTestevent-loop harness for the asyncio-dependent protocol code. No live socket or MongoDB server required.Checklist
Checklist for Author
Checklist for Reviewer