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.

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

Use mock.patch.object to avoid protected access errors. #2253

Closed

Conversation

jameshilliard
Copy link
Contributor

No description provided.

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.

A few comments, but the most important one:

You already submitted #2251 to solve this problem, seems you did not complete your work. Having 2 PR solving the same problem in the same file is over the top, and wasting my time, since I have to review them.

Please do not submit more PR to test_transaction.py unless they contain more than cosmetic changes, meaning real code change.

@@ -90,7 +91,9 @@ def test_calculate_exception_length(self):
)

@mock.patch("pymodbus.transaction.time")
def test_execute(self, mock_time):
@mock.patch.object(SyncModbusTransactionManager, "_recv")
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 wrong, it change the scope from class to local.

@@ -353,19 +353,18 @@ def callback(data):
# for name in ("transaction_id", "protocol_id", "slave_id"):
# assert getattr(expected, name) == getattr(actual, name)

def test_tcp_framer_packet(self):
@mock.patch.object(ModbusRequest, "encode")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems a number of the patch objects could be done at class level.

@jameshilliard
Copy link
Contributor Author

Having 2 PR solving the same problem in the same file is over the top, and wasting my time, since I have to review them.

Um, it's a different file right? Path for this PR looks to be test/framers/test_tbc_transaction.py while #2251 is for test/test_transaction.py. So test/framers/test_tbc_transaction.py is rather similar to test/test_transaction.py, are we supposed to have both of these tests or just one?

@janiversen
Copy link
Collaborator

janiversen commented Jul 22, 2024

If you had a look at the files you would see it’s the same file, just copied to prepare for major changes.

tbc == to be changed

@jameshilliard
Copy link
Contributor Author

If you had a look at the files you would see it’s the same file,

Yeah...I missed that somehow, had assumed it wasn't an exact duplicate so had applied the same fixes from the other one.

just copied to prepare for major changes.

Um, so I should make sure changes are duplicated between the files?

@janiversen janiversen marked this pull request as draft July 22, 2024 08:37
@janiversen
Copy link
Collaborator

No you should not touch a file marked "to be changed" unless you want to change it as intended.

@jameshilliard
Copy link
Contributor Author

change it as intended.

How is it intended to be changed?

@janiversen
Copy link
Collaborator

Read the issues, it will be integrated in framers.

Copy link

github-actions bot commented Sep 6, 2024

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

Copy link

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants