Test for pull request #304 (Added try-catch to notification close in symbol destructor) #369
Conversation
chhinze
changed the title
|
Stumbling across this again. Issue #303 is still relevant, thanks @chhinze for adding a test! @stlehmann could we merge this? |
tests/test_testserver.py
Outdated
| 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}" | ||
| ) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you for pointing it out and for the clarification. I changed it now with the recent commit cbff3e8.
|
I have some plans this week or next to take a look at some of the Open PRs, as I have a few that I have "marked" in my own head as ready to merge so was going to spend some time doing it. |
tests/test_testserver.py
Outdated
| # `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: | ||
| self.fail(not raised_error, f"Closing server connection raised: {e}") |
There was a problem hiding this comment.
Thanks for updating. The syntax isn't correct though, self.fail() doesn't take in a verification parameter, only the string message:
self.fail(f"Closing server connection raised: {e}")There was a problem hiding this comment.
Sorry and thanks for spotting the mistake. I updated it now and specifically tested again that it raises corectly for pre #303 code (without try except-wrapped in AdsSymbol.__del__()).
Use self.fail() now instead of self.assert(). Thanks to @RobtertoRoos for pointing it out.
chhinze
force-pushed
the
bugfix/adssymbol_del
branch
from
cbff3e8 to
22d0318
Compare
Pull Request Test Coverage Report for Build 10158424620Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
This is an extension to #304 (fixing #303) adding a test for the specific behavior. I have this problem repeatedly, when using
auto_update=trueforAdsSymbolsand the server disconnects and, afterwards theAdsSymbolsinstances get garbage-collected.I would love to see pull request #304 merged, so I added the requested test. The additional commit is directly added on top of the commit in the branch in #304. Without the try-catch in #304 the test fails. I hope the test is useful.
Is this what you had in mind @stlehmann?