From 527e5d00ae36db09f5affacd4e3e75b9c140d03c Mon Sep 17 00:00:00 2001 From: jan iversen Date: Sun, 10 Nov 2024 16:52:46 +0100 Subject: [PATCH] pdu, 100% coverage. (#2450) --- doc/source/roadmap.rst | 8 +-- pymodbus/pdu/diag_message.py | 9 +-- pymodbus/pdu/file_message.py | 6 +- pymodbus/pdu/mei_message.py | 17 ++--- pymodbus/pdu/other_message.py | 29 ++++----- test/pdu/test_bit.py | 11 ---- test/pdu/test_diag_messages.py | 25 +++++-- test/pdu/test_file_message.py | 10 +++ test/pdu/test_mei_messages.py | 6 ++ test/pdu/test_pdu.py | 87 ++++++++++++++----------- test/pdu/test_register_read_messages.py | 24 +++---- 11 files changed, 120 insertions(+), 112 deletions(-) diff --git a/doc/source/roadmap.rst b/doc/source/roadmap.rst index 77a38cc78..3c168be4d 100644 --- a/doc/source/roadmap.rst +++ b/doc/source/roadmap.rst @@ -18,17 +18,13 @@ The following bullet points are what the maintainers focus on: - 3.7.X, bug fix release, hopefully with: - Not planned - 3.8.0, with: - - on dev: - - skip_encode, zero_mode parameters removed - - Simplify PDU bit classes - - Simplify PDU classes - - Simplify transaction manager (central control point) + - all on dev - Remove ModbusControlBlock - new transaction handling - - pdu 100% coverage - transaction 100% coverage - client 100% coverage - 4.0.0, with: + - all on dev - client async with sync/async API - Only one datastore, but with different API`s - Simulator standard in server diff --git a/pymodbus/pdu/diag_message.py b/pymodbus/pdu/diag_message.py index db871447a..3b8301800 100644 --- a/pymodbus/pdu/diag_message.py +++ b/pymodbus/pdu/diag_message.py @@ -21,18 +21,15 @@ class DiagnosticBase(ModbusPDU): sub_function_code: int = 9999 rtu_frame_size = 8 - def __init__(self, message: bytes | int | str | list | tuple | None = 0, slave_id: int = 1, transaction_id: int = 0) -> None: + def __init__(self, message: bytes | int | list | tuple | None = 0, slave_id: int = 1, transaction_id: int = 0) -> None: """Initialize a diagnostic request.""" super().__init__(transaction_id=transaction_id, slave_id=slave_id) - self.message: bytes | int | str | list | tuple | None = message + self.message: bytes | int | list | tuple | None = message def encode(self) -> bytes: """Encode a diagnostic response.""" packet = struct.pack(">H", self.sub_function_code) if self.message is not None: - if isinstance(self.message, str): - packet += self.message.encode() - return packet if isinstance(self.message, bytes): packet += self.message return packet @@ -43,7 +40,7 @@ def encode(self) -> bytes: for piece in self.message: packet += struct.pack(">H", piece) return packet - raise RuntimeError(f"UNKNOWN DIAG message type: {type(self.message)}") + raise TypeError(f"UNKNOWN DIAG message type: {type(self.message)}") return packet def decode(self, data: bytes) -> None: diff --git a/pymodbus/pdu/file_message.py b/pymodbus/pdu/file_message.py index 35ae64b34..85f1197ac 100644 --- a/pymodbus/pdu/file_message.py +++ b/pymodbus/pdu/file_message.py @@ -24,9 +24,9 @@ def __post_init__(self) -> None: if self.record_length: raise ModbusException("Use either record_data= or record_length=") self.record_length = len(self.record_data) - if self.record_length % 2: - raise ModbusException("length of record_data must be a multiple of 2") - self.record_length //= 2 + if self.record_length % 2: + raise ModbusException("length of record_data must be a multiple of 2") + self.record_length //= 2 class ReadFileRecordRequest(ModbusPDU): diff --git a/pymodbus/pdu/mei_message.py b/pymodbus/pdu/mei_message.py index 0b8d39872..17f5a0d43 100644 --- a/pymodbus/pdu/mei_message.py +++ b/pymodbus/pdu/mei_message.py @@ -77,16 +77,13 @@ def calculateRtuFrameSize(cls, buffer: bytes) -> int: return 999 count = int(buffer[7]) - try: - while count > 0: - if data_len < size+2: - return 998 - _, object_length = struct.unpack(">BB", buffer[size : size + 2]) - size += object_length + 2 - count -= 1 - return size + 2 - except struct.error as exc: - raise IndexError from exc + while count > 0: + if data_len < size+2: + return 998 + _, object_length = struct.unpack(">BB", buffer[size : size + 2]) + size += object_length + 2 + count -= 1 + return size + 2 def __init__(self, read_code: int | None = None, information: dict | None = None, slave_id: int = 1, transaction_id: int = 0) -> None: """Initialize a new instance.""" diff --git a/pymodbus/pdu/other_message.py b/pymodbus/pdu/other_message.py index 834d03733..ec7ed2db5 100644 --- a/pymodbus/pdu/other_message.py +++ b/pymodbus/pdu/other_message.py @@ -159,24 +159,19 @@ def encode(self) -> bytes: def decode(self, _data: bytes) -> None: """Decode data part of the message.""" - async def update_datastore(self, context: ModbusSlaveContext) -> ModbusPDU: + async def update_datastore(self, _context: ModbusSlaveContext) -> ModbusPDU: """Run a report slave id request against the store.""" - report_slave_id_data = None - if context: - report_slave_id_data = getattr(context, "reportSlaveIdData", None) - if not report_slave_id_data: - information = DeviceInformationFactory.get(_MCB) - id_data = [] - for v_item in information.values(): - if isinstance(v_item, bytes): - id_data.append(v_item) - else: - id_data.append(v_item.encode()) - - identifier = b"-".join(id_data) - identifier = identifier or b"Pymodbus" - report_slave_id_data = identifier - return ReportSlaveIdResponse(identifier=report_slave_id_data, slave_id=self.slave_id, transaction_id=self.transaction_id) + information = DeviceInformationFactory.get(_MCB) + id_data = [] + for v_item in information.values(): + if isinstance(v_item, bytes): + id_data.append(v_item) + else: + id_data.append(v_item.encode()) + + identifier = b"-".join(id_data) + identifier = identifier or b"Pymodbus" + return ReportSlaveIdResponse(identifier=identifier, slave_id=self.slave_id, transaction_id=self.transaction_id) class ReportSlaveIdResponse(ModbusPDU): diff --git a/test/pdu/test_bit.py b/test/pdu/test_bit.py index 01380407e..bfb142547 100644 --- a/test/pdu/test_bit.py +++ b/test/pdu/test_bit.py @@ -43,17 +43,6 @@ async def test_bit_read_update_datastore_address_errors(self, mock_context): ): await pdu.update_datastore(context) - async def test_bit_read_update_datastore_success(self, mock_context): - """Test bit read request encoding.""" - context = mock_context() - context.validate = lambda a, b, c: True - for pdu in ( - (bit_msg.ReadCoilsRequest(address=1, count=5)), - (bit_msg.ReadDiscreteInputsRequest(address=1, count=5)), - ): - result = await pdu.update_datastore(context) - assert result.bits == [True] * 5 - def test_bit_read_get_response_pdu(self): """Test bit read message get response pdu.""" for pdu, expected in ( diff --git a/test/pdu/test_diag_messages.py b/test/pdu/test_diag_messages.py index f5be838a4..fed1bac1e 100644 --- a/test/pdu/test_diag_messages.py +++ b/test/pdu/test_diag_messages.py @@ -1,4 +1,5 @@ """Test diag messages.""" +import pytest from pymodbus.constants import ModbusPlusOperation, ModbusStatus from pymodbus.pdu.diag_message import ( @@ -116,12 +117,24 @@ class TestDataStore: def test_diagnostic_encode_decode(self): """Testing diagnostic request/response can be decoded and encoded.""" - for msg in (DiagnosticBase, DiagnosticBase): - msg_obj = msg() - data = b"\x00\x01\x02\x03" - msg_obj.decode(data) - result = msg_obj.encode() - assert data == result + msg_obj = DiagnosticBase() + data = b"\x00\x01\x02\x03" + msg_obj.decode(data) + result = msg_obj.encode() + assert data == result + + def test_diagnostic_encode_error(self): + """Testing diagnostic request/response can be decoded and encoded.""" + msg_obj = DiagnosticBase() + msg_obj.message = "not allowed" + with pytest.raises(TypeError): + msg_obj.encode() + + def test_diagnostic_decode_error(self): + """Testing diagnostic request/response can be decoded and encoded.""" + msg_obj = DiagnosticBase() + data = b"\x00\x01\x02\x03a" + msg_obj.decode(data) def test_diagnostic_requests_decode(self): """Testing diagnostic request messages encoding.""" diff --git a/test/pdu/test_file_message.py b/test/pdu/test_file_message.py index cfb3a4691..8f4383e78 100644 --- a/test/pdu/test_file_message.py +++ b/test/pdu/test_file_message.py @@ -6,6 +6,9 @@ * Read/Write Discretes * Read Coils """ +import pytest + +from pymodbus.exceptions import ModbusException from pymodbus.pdu.file_message import ( FileRecord, ReadFifoQueueRequest, @@ -72,6 +75,13 @@ def test_file_record_length(self): ) assert record.record_length == 0x02 + def test_file_record_errors(self): + """Test file record length generation.""" + with pytest.raises(ModbusException): + FileRecord(record_length=12, record_data=b"\x00\x01\x02\x04") + with pytest.raises(ModbusException): + FileRecord(record_length=11) + def test_read_file_record_request_encode(self): """Test basic bit message encoding/decoding.""" records = [FileRecord(file_number=0x01, record_number=0x02)] diff --git a/test/pdu/test_mei_messages.py b/test/pdu/test_mei_messages.py index 75185d94f..40153f3a3 100644 --- a/test/pdu/test_mei_messages.py +++ b/test/pdu/test_mei_messages.py @@ -75,6 +75,12 @@ async def test_read_device_information_request_error(self): handle.object_id = 0x100 assert (await handle.update_datastore(None)).function_code == 0xAB + def test_read_device_information_calc1(self): + """Test calculateRtuFrameSize, short buffer.""" + handle = ReadDeviceInformationResponse() + assert handle.calculateRtuFrameSize(b"\x0e\x01\x83") == 999 + assert handle.calculateRtuFrameSize(b"\x0e\x01\x83\x00\x00\x03\x01\x03") == 998 + def test_read_device_information_encode(self): """Test that the read fifo queue response can encode.""" message = b"\x0e\x01\x83\x00\x00\x03" diff --git a/test/pdu/test_pdu.py b/test/pdu/test_pdu.py index f5e515018..31679d141 100644 --- a/test/pdu/test_pdu.py +++ b/test/pdu/test_pdu.py @@ -157,25 +157,19 @@ def test_pdu_instance(self, pdutype): assert pdu @pytest.mark.parametrize(("pdutype", "args", "kwargs", "frame"), requests + responses) - @pytest.mark.usefixtures("frame") - def test_pdu_instance_args(self, pdutype, args, kwargs): + @pytest.mark.usefixtures("frame", "args") + def test_pdu_instance_args(self, pdutype, kwargs): """Test that all PDU types can be created.""" - if args: - pdu = pdutype() - pdu.setData(*args) - else: - pdu = pdutype(*args, **kwargs) + pdu = pdutype(**kwargs) assert pdu assert pdutype.__name__ in str(pdu) @pytest.mark.parametrize(("pdutype", "args", "kwargs", "frame"), requests + responses) - @pytest.mark.usefixtures("frame") - def test_pdu_instance_extras(self, pdutype, args, kwargs): + @pytest.mark.usefixtures("frame", "args") + def test_pdu_instance_extras(self, pdutype, kwargs): """Test that all PDU types can be created.""" tid = 9112 slave_id = 63 - if args: - return try: pdu = pdutype(transaction=tid, slave=slave_id, **kwargs) except TypeError: @@ -186,48 +180,65 @@ def test_pdu_instance_extras(self, pdutype, args, kwargs): assert pdu.transaction_id == tid assert pdu.function_code > 0 + def test_pdu_register_as_byte(self): + """Test validate functions.""" + registers =[b'ab', b'cd'] + req = reg_msg.ReadHoldingRegistersRequest(address=117, registers=registers, count=3) + assert len(req.registers) == 2 + assert req.registers[0] == 24930 + assert req.registers[1] == 25444 + + def test_pdu_validate_address(self): + """Test validate functions.""" + req = reg_msg.ReadHoldingRegistersRequest(address=10, count=3) + req.address = -1 + with pytest.raises(ValueError): # noqa: PT011 + req.validateAddress() + req.address = 66000 + with pytest.raises(ValueError): # noqa: PT011 + req.validateAddress() + + def test_pdu_validate_count(self): + """Test validate functions.""" + req = reg_msg.ReadHoldingRegistersRequest(address=2, count=0) + req.count = 0 + with pytest.raises(ValueError): # noqa: PT011 + req.validateCount(100) + req.count = 101 + with pytest.raises(ValueError): # noqa: PT011 + req.validateCount(100) + with pytest.raises(ValueError): # noqa: PT011 + req.validateCount(100, count=0) + + @pytest.mark.parametrize(("pdutype", "args", "kwargs", "frame"), requests + responses) - def test_pdu_instance_encode(self, pdutype, args, kwargs, frame): + @pytest.mark.usefixtures("args") + def test_pdu_instance_encode(self, pdutype, kwargs, frame): """Test that all PDU types can be created.""" - if args: - pdu = pdutype() - pdu.setData(*args) - else: - pdu = pdutype(**kwargs) + pdu = pdutype(**kwargs) res_frame = pdutype.function_code.to_bytes(1,'big') + pdu.encode() assert res_frame == frame @pytest.mark.parametrize(("pdutype", "args", "kwargs", "frame"), requests) - @pytest.mark.usefixtures("frame") - def test_get_response_pdu_size2(self, pdutype, args, kwargs): + @pytest.mark.usefixtures("frame", "args") + def test_get_response_pdu_size2(self, pdutype, kwargs): """Test that all PDU types can be created.""" - if args: - pdu = pdutype() - pdu.setData(*args) - else: - pdu = pdutype(**kwargs) + pdu = pdutype(**kwargs) pdu.get_response_pdu_size() #FIX size > 0 !! @pytest.mark.parametrize(("pdutype", "args", "kwargs", "frame"), requests + responses) - def test_pdu_decode(self, pdutype, args, kwargs, frame): - """Test that all PDU types can be created.""" - if args: - pdu = pdutype() - pdu.setData(*args) - else: - pdu = pdutype(**kwargs) + @pytest.mark.usefixtures("args") + def test_pdu_decode(self, pdutype, kwargs, frame): + """Test pdu decode.""" + pdu = pdutype(**kwargs) pdu.decode(frame[1:]) @pytest.mark.parametrize(("pdutype", "args", "kwargs", "frame"), requests) - @pytest.mark.usefixtures("frame") - async def test_pdu_datastore(self, pdutype, args, kwargs, mock_context): + @pytest.mark.usefixtures("frame", "args") + async def test_pdu_datastore(self, pdutype, kwargs, mock_context): """Test that all PDU types can be created.""" - if args: - pdu = pdutype() - pdu.setData(*args) - else: - pdu = pdutype(**kwargs) + pdu = pdutype(**kwargs) context = mock_context() context.validate = lambda a, b, c: True assert await pdu.update_datastore(context) diff --git a/test/pdu/test_register_read_messages.py b/test/pdu/test_register_read_messages.py index 4575f86fe..14d2069d6 100644 --- a/test/pdu/test_register_read_messages.py +++ b/test/pdu/test_register_read_messages.py @@ -1,4 +1,7 @@ """Test register read messages.""" +import pytest + +from pymodbus.exceptions import ModbusIOException from pymodbus.pdu import ModbusExceptions from pymodbus.pdu.register_message import ( ReadHoldingRegistersRequest, @@ -75,6 +78,12 @@ def test_register_read_response_decode(self): response.decode(packet) assert response.registers == self.values + def test_register_read_response_decode_error(self): + """Test register read response.""" + reg = ReadHoldingRegistersResponse(count = 5) + with pytest.raises(ModbusIOException): + reg.decode(b'\x14\x00\x03\x00\x11') + async def test_register_read_requests_count_errors(self): """This tests that the register request messages. @@ -151,21 +160,6 @@ async def test_read_write_multiple_registers_validate(self, mock_context): await request.update_datastore(context) #assert response.exception_code == ModbusExceptions.ILLEGAL_VALUE - def test_read_write_multiple_registers_request_decode(self): - """Test read/write multiple registers.""" - request, response = next( - (k, v) - for k, v in self.request_read.items() - if getattr(k, "function_code", 0) == 23 - ) - request.decode(response) - assert request.read_address == 0x01 - assert request.write_address == 0x01 - assert request.read_count == 0x05 - assert request.write_count == 0x05 - assert request.write_byte_count == 0x0A - assert request.write_registers == [0x00] * 5 - def test_serializing_to_string(self): """Test serializing to string.""" for request in iter(self.request_read.keys()):