Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix mypy type checking errors in test_transaction.py #2249

Closed
wants to merge 1 commit into from
Closed

Fix mypy type checking errors in test_transaction.py #2249

wants to merge 1 commit into from

Conversation

jameshilliard
Copy link
Contributor

This fixes a number of issues with tests using the wrong types that was flagged by mypy.

@jameshilliard
Copy link
Contributor Author

Errors on dev branch this fixes for reference:

$ mypy test/test_transaction.py 
test/test_transaction.py:25: error: Need type annotation for "client" (hint: "client: <type> | None = ...")  [var-annotated]
test/test_transaction.py:26: error: Need type annotation for "decoder" (hint: "decoder: <type> | None = ...")  [var-annotated]
test/test_transaction.py:27: error: Need type annotation for "_tcp" (hint: "_tcp: <type> | None = ...")  [var-annotated]
test/test_transaction.py:28: error: Need type annotation for "_tls" (hint: "_tls: <type> | None = ...")  [var-annotated]
test/test_transaction.py:29: error: Need type annotation for "_rtu" (hint: "_rtu: <type> | None = ...")  [var-annotated]
test/test_transaction.py:30: error: Need type annotation for "_ascii" (hint: "_ascii: <type> | None = ...")  [var-annotated]
test/test_transaction.py:31: error: Need type annotation for "_manager" (hint: "_manager: <type> | None = ...")  [var-annotated]
test/test_transaction.py:32: error: Need type annotation for "_tm" (hint: "_tm: <type> | None = ...")  [var-annotated]
test/test_transaction.py:45: error: Argument 1 to "SyncModbusTransactionManager" has incompatible type "None"; expected "ModbusBaseSyncClient"  [arg-type]
test/test_transaction.py:53: error: Item "None" of "Any | None" has no attribute "client"  [union-attr]
test/test_transaction.py:54: error: Item "None" of "Any | None" has no attribute "client"  [union-attr]
test/test_transaction.py:55: error: Item "None" of "Any | None" has no attribute "_set_adu_size"  [union-attr]
test/test_transaction.py:56: error: Item "None" of "Any | None" has no attribute "_calculate_response_length"  [union-attr]
test/test_transaction.py:59: error: Item "None" of "Any | None" has no attribute "base_adu_size"  [union-attr]
test/test_transaction.py:61: error: Item "None" of "Any | None" has no attribute "_calculate_response_length"  [union-attr]
test/test_transaction.py:74: error: Item "None" of "Any | None" has no attribute "client"  [union-attr]
test/test_transaction.py:76: error: Item "None" of "Any | None" has no attribute "client"  [union-attr]
test/test_transaction.py:78: error: Item "None" of "Any | None" has no attribute "client"  [union-attr]
test/test_transaction.py:80: error: Item "None" of "Any | None" has no attribute "client"  [union-attr]
test/test_transaction.py:82: error: Item "None" of "Any | None" has no attribute "client"  [union-attr]
test/test_transaction.py:84: error: Item "None" of "Any | None" has no attribute "client"  [union-attr]
test/test_transaction.py:86: error: Item "None" of "Any | None" has no attribute "_set_adu_size"  [union-attr]
test/test_transaction.py:88: error: Item "None" of "Any | None" has no attribute "_calculate_exception_length"  [union-attr]
test/test_transaction.py:99: error: Item "None" of "Any | None" has no attribute "_buffer"  [union-attr]
test/test_transaction.py:100: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:101: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:102: error: Item "None" of "Any | None" has no attribute "buildPacket"  [union-attr]
test/test_transaction.py:103: error: Item "None" of "Any | None" has no attribute "buildPacket"  [union-attr]
test/test_transaction.py:104: error: Item "None" of "Any | None" has no attribute "sendPacket"  [union-attr]
test/test_transaction.py:105: error: Item "None" of "Any | None" has no attribute "sendPacket"  [union-attr]
test/test_transaction.py:106: error: Item "None" of "Any | None" has no attribute "decode_data"  [union-attr]
test/test_transaction.py:107: error: Item "None" of "Any | None" has no attribute "decode_data"  [union-attr]
test/test_transaction.py:117: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:123: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:128: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:132: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:139: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:146: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:157: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:164: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:167: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:182: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:191: error: Item "None" of "Any | None" has no attribute "getNextTID"  [union-attr]
test/test_transaction.py:192: error: Item "None" of "Any | None" has no attribute "getNextTID"  [union-attr]
test/test_transaction.py:193: error: Item "None" of "Any | None" has no attribute "reset"  [union-attr]
test/test_transaction.py:194: error: Item "None" of "Any | None" has no attribute "getNextTID"  [union-attr]
test/test_transaction.py:202: error: Item "None" of "Any | None" has no attribute "reset"  [union-attr]
test/test_transaction.py:204: error: "Request" has no attribute "transaction_id"  [attr-defined]
test/test_transaction.py:205: error: Item "None" of "Any | None" has no attribute "getNextTID"  [union-attr]
test/test_transaction.py:207: error: "Request" has no attribute "message"  [attr-defined]
test/test_transaction.py:208: error: Item "None" of "Any | None" has no attribute "addTransaction"  [union-attr]
test/test_transaction.py:209: error: Item "None" of "Any | None" has no attribute "getTransaction"  [union-attr]
test/test_transaction.py:209: error: "Request" has no attribute "transaction_id"  [attr-defined]
test/test_transaction.py:210: error: "Request" has no attribute "message"  [attr-defined]
test/test_transaction.py:218: error: Item "None" of "Any | None" has no attribute "reset"  [union-attr]
test/test_transaction.py:220: error: "Request" has no attribute "transaction_id"  [attr-defined]
test/test_transaction.py:221: error: Item "None" of "Any | None" has no attribute "getNextTID"  [union-attr]
test/test_transaction.py:223: error: "Request" has no attribute "message"  [attr-defined]
test/test_transaction.py:225: error: Item "None" of "Any | None" has no attribute "addTransaction"  [union-attr]
test/test_transaction.py:226: error: Item "None" of "Any | None" has no attribute "delTransaction"  [union-attr]
test/test_transaction.py:226: error: "Request" has no attribute "transaction_id"  [attr-defined]
test/test_transaction.py:227: error: Item "None" of "Any | None" has no attribute "getTransaction"  [union-attr]
test/test_transaction.py:227: error: "Request" has no attribute "transaction_id"  [attr-defined]
test/test_transaction.py:243: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:244: error: Item "None" of "Any | None" has no attribute "_buffer"  [union-attr]
test/test_transaction.py:258: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:259: error: Item "None" of "Any | None" has no attribute "function_code"  [union-attr]
test/test_transaction.py:259: error: Item "None" of "Any | None" has no attribute "encode"  [union-attr]
test/test_transaction.py:273: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:275: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:291: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:293: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:309: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:311: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:328: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:330: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:349: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:359: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:366: error: Item "None" of "Any | None" has no attribute "buildPacket"  [union-attr]
test/test_transaction.py:368: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:384: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:386: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:400: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:414: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:416: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:430: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:432: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:439: error: Item "None" of "Any | None" has no attribute "decode_data"  [union-attr]
test/test_transaction.py:441: error: Item "None" of "Any | None" has no attribute "decode_data"  [union-attr]
test/test_transaction.py:457: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:508: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:514: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:518: error: Item "None" of "Any | None" has no attribute "buildPacket"  [union-attr]
test/test_transaction.py:520: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:536: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:538: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:552: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:566: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:568: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:582: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:583: error: Item "None" of "Any | None" has no attribute "_header"  [union-attr]
test/test_transaction.py:591: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:596: error: Item "None" of "Any | None" has no attribute "buildPacket"  [union-attr]
test/test_transaction.py:598: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:611: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:625: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:641: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:658: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:672: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:686: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:688: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
test/test_transaction.py:694: error: Item "None" of "Any | None" has no attribute "populateResult"  [union-attr]
test/test_transaction.py:700: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:705: error: Item "None" of "Any | None" has no attribute "buildPacket"  [union-attr]
test/test_transaction.py:707: error: Cannot assign to a method  [method-assign]
test/test_transaction.py:720: error: Item "None" of "Any | None" has no attribute "processIncomingPacket"  [union-attr]
Found 116 errors in 1 file (checked 1 source file)

janiversen
janiversen requested changes Jul 20, 2024
View reviewed changes
Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want type checking in test, as written to you before.

It does not bring any benefits to the production code, and you are sure to run into bigger problems, since we use advanced Pytest, which mypy by far do not support.

@janiversen
Copy link
Collaborator

Closing pull request as being not wanted. Thanks for your initiative, but it do help if you did read what 2 of use wrote to you before.

@janiversen janiversen closed this Jul 20, 2024
@jameshilliard
Copy link
Contributor Author

We do not want type checking in test, as written to you before.

I thought you didn't want type hinting in the tests, this is fixing actual bugs in the tests.

@jameshilliard jameshilliard mentioned this pull request Jul 20, 2024
Closed
alexrudd2
alexrudd2 reviewed Jul 20, 2024
View reviewed changes
Copy link
Collaborator

@alexrudd2 alexrudd2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of the changes in here are good, and lead to a more robust test. Some are just noise.

test/test_transaction.py
_ascii = None
_manager = None
_tm = None
client: ModbusBaseSyncClient
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a need for type hinting the class attributes here at all; they will be defined in setup_method(). Just removing the lines here satisfies mypy.

test/test_transaction.py
self._ascii = ModbusAsciiFramer(decoder=self.decoder, client=None)
self._manager = SyncModbusTransactionManager(self.client, 0.3, False, False, 3)
self._tcp = ModbusSocketFramer(
decoder=self.decoder, client=ModbusTcpClient("127.0.0.1")
Copy link
Collaborator

@alexrudd2 alexrudd2 Jul 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why initialize a client here? I don't see how it improves anything over client = None

EDIT: client = self.client

Copy link
Contributor Author

@jameshilliard jameshilliard Jul 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why initialize a client here? I don't see how it improves anything over client = None

None isn't an allowable type for client: ModbusBaseSyncClient. Not sure if it's better to use a real client or a mock client here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to use mock.create_autospec for creating mock client objects.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why initialize a client here? I don't see how it improves anything over client = None

None isn't an allowable type for client: ModbusBaseSyncClient. Not sure if it's better to use a real client or a mock client here.

Actually, only _manager needs a client (mock or real). The others all accept a default argument of client = None

test/test_transaction.py
trans._recv = mock.MagicMock( # pylint: disable=protected-access
return_value=b"abcdef"
)
setattr(trans, "_recv", mock.MagicMock(return_value=b"abcdef"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just tricking mypy and pylint which AFAIK don't process setattr().

Cute, but not much advantage over # type: ignore.

test/test_transaction.py
response = trans.execute(request)
assert response == "response"
assert response == b"response"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

test/test_transaction.py
)
client.comm_params.handle_local_echo = True
trans.retry_on_empty = False
trans.retry_on_invalid = False
assert trans.execute(request).message == "[Input/Output] Wrong local echo"
response = trans.execute(request)
assert isinstance(response, ModbusIOException)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially (via email) I didn't like these asserts, but now I do think they are valid additions.

test/test_transaction.py
handle = Request()
handle.transaction_id = ( # pylint: disable=attribute-defined-outside-init
self._manager.getNextTID()
handle = ModbusRequest(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good improvement.

@alexrudd2 alexrudd2 mentioned this pull request Jul 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrudd2 alexrudd2 alexrudd2 left review comments

@janiversen janiversen janiversen requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jameshilliard @janiversen @alexrudd2