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

Test for pull request #304 (Added try-catch to notification close in symbol destructor) #369

Merged
merged 5 commits into from
Jul 30, 2024
Merged

Test for pull request #304 (Added try-catch to notification close in symbol destructor) #369

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6a1d656
Added try-catch to notification close in symbol destructor
RobertoRoos Jan 12, 2022
e14dc5e
Add test for try-catch to notification close in symbol destructor
chhinze Feb 8, 2024
7979953
Update changelog
chhinze Feb 8, 2024
22d0318
More elegant way to test for error not raised.
chhinze Jul 25, 2024
72ffa65
Merge branch 'master' into bugfix/adssymbol_del
chrisbeardy Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
* [#293](https://github.com/stlehmann/pyads/pull/2939) Support WSTRINGS in structures

### Changed
* [#369](https://github.com/stlehmann/pyads/pull/304) Add test for [#304](https://github.com/stlehmann/pyads/pull/304) in `tests/test_testserver.py`
* [#304](https://github.com/stlehmann/pyads/pull/304) Implemented try-catch when closing ADS notifications in AdsSymbol destructor
* [#292](https://github.com/stlehmann/pyads/pull/292) Improve performance of get_value_from_ctype_data for arrays

### Removed
Expand Down
7 changes: 5 additions & 2 deletions pyads/symbol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

from . import constants # To access all constants, use package notation
from .constants import PLCDataType
from .pyads_ex import adsGetSymbolInfo
from .pyads_ex import adsGetSymbolInfo, ADSError
from .structs import NotificationAttrib

# ads.Connection relies on structs.AdsSymbol (but in type hints only), so use
Expand Down Expand Up @@ -229,7 +229,10 @@ def __repr__(self) -> str:

def __del__(self) -> None:
"""Destructor"""
self.clear_device_notifications()
try:
self.clear_device_notifications()
except ADSError:
pass # Quietly continue, without a connection no cleanup could be done

def add_device_notification(
self,
Expand Down
41 changes: 41 additions & 0 deletions tests/test_testserver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,47 @@ def test_context(self):

time.sleep(0.1) # Give server a moment to spin down

def test_server_disconnect_then_del_device_notification(self):
"""Test no error thown, when ADS symbol with device_notification is cleaned up after the server went offline.

Tests fix of issue [#303](https://github.com/stlehmann/pyads/issues/303), original pull request: [#304](https://github.com/stlehmann/pyads/pull/304)
"""
# 1. spin up the server
handler = BasicHandler()
test_server = AdsTestServer(handler=handler, logging=False)
test_server.start()
time.sleep(0.1) # Give server a moment to spin up

# 2. open a plc connection to the test server:
plc = pyads.Connection(TEST_SERVER_AMS_NET_ID, TEST_SERVER_AMS_PORT)
plc.open()

# 3. add a variable, register a device notification with auto_update=True
test_int = pyads.AdsSymbol(plc, "TestSymbol", symbol_type=pyads.PLCTYPE_INT)
test_int.plc_type = pyads.PLCTYPE_INT
test_int.auto_update = True
time.sleep(0.1) # Give server a moment

raised_error: str = ""

try:
# 4. stop the test server
test_server.stop()
time.sleep(0.1) # Give server a moment

# some code, where test_int is cleared by the Garbage collector after the server was stopped
# (e.g. the machine with ADS Server disconnected)
# this raised an ADSError up to commit [a7af674](https://github.com/stlehmann/pyads/tree/a7af674b49b1c91966f2bac1f00f86273cbd9af8)
# `clear_device_notifications()` failed, if not wrapped in try-catch as the server is no longer present.
test_int.__del__()
except pyads.ADSError as e:
raised_error = str(e)

# check no error was raised:
self.assertTrue(
not raised_error, f"Closing server connection raised: {raised_error}"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you could be bothered, I find something like:

test_server.stop()
time.sleep(0.1)  # Give server a moment

try:
    test_int.__del__()
except ExceptionType as err:
    self.fail(f"Closing server connection raised: {raised_error}")

more neat (see SO).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I would be happy to consider your suggestions. Just to be clear: do you mean by the comment that you find the comments too cluttering, or do you refer to the self.assertTrue(...) check, or both?

Copy link
Contributor

@RobertoRoos RobertoRoos Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, your in-code comments are fine. I mean the way of asserting an exception does not get raised. I think the except self.fail(...) is cleaner than an self.assert... after the clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing it out and for the clarification. I changed it now with the recent commit cbff3e8.



if __name__ == "__main__":
unittest.main()