PYTHON-5781 Increase code coverage for network_layer.py#2774
PYTHON-5781 Increase code coverage for network_layer.py#2774aclark4life wants to merge 6 commits intomongodb:masterfrom
Conversation
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.
| def test_returns_op_code_and_compressor_id(self): | ||
| async def _test(): | ||
| proto = await _make_protocol() | ||
| # op_code=2013, unknown int=0, compressor_id=1 (snappy) |
There was a problem hiding this comment.
The comment describing the OP_COMPRESSED header layout is inaccurate: the second int in <iiB is the uncompressed message size (per the wire protocol), not an “unknown int”. Updating the comment will avoid confusion for future readers maintaining these tests.
| # op_code=2013, unknown int=0, compressor_id=1 (snappy) | |
| # op_code=2013, uncompressed message size=0, compressor_id=1 (snappy) |
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
| def test_sock_returns_socket(self): | ||
| self.assertIs(self.network_interface.sock, self.mock_socket) | ||
|
|
||
|
|
||
| if not _IS_SYNC: | ||
|
|
||
| class TestNetworkingInterface(UnitTest): | ||
| def _make_network_interface(self): | ||
| mock_transport = MagicMock() |
PYTHON-5781
Changes in this PR
Adds
test/test_network_layer.pywith unit tests forpymongo/network_layer.py. Tests coverPyMongoProtocolstate machine (process_headerincluding allProtocolErrorpaths,process_compression_header,get_bufferall state branches,buffer_updated,close,connection_lost),NetworkingInterfaceBaseabstract methods,NetworkingInterfacesocket delegation,AsyncNetworkingInterfacetransport/protocol delegation,sendall, and_async_socket_receive.Test Plan
Added unit tests using mock transports and
asyncio.run()to exercise the asyncio-dependent protocol code. No live socket or MongoDB server required.Checklist
Checklist for Author
Checklist for Reviewer