Add as checks with tests #192
Conversation
Add check_as_completion Add tests for both
Add BaseExceptions for other possible thrown errors
Add exception block to catch PendingErrors, which are logged as errors
Replace bare exception block with better ones
Add missing as_ prefix to as_db_get test.
|
Because all |
Add better description to check_as_completion
|
Any other thoughts? |
remove obsolete try_except block to catch other socket error remove obsolete logging in test
|
Hmm is travis not running, because review is still pending? 😕 |
…thrown Add tries for test_wait_as_completion_timeout, because timeout can't be evoked sometimes. Add skipTest to test_wait_as_completion_timeout, in case no timeout evoked after x tries.
|
Does anyone have a clue why Travis is not triggered anymore? EDIT: ...Strange ... well I got a report sended to me, but it is not appearing here. EDIT2: Now even Travis is happy. ^^ |
|
the builds are triggered but somehow travis does not report back to github anymore https://travis-ci.org/github/gijzelaerr/python-snap7/builds/735967984 |
test/test_client.py
Outdated
| return | ||
| except BaseException as pyt_err: | ||
| self.fail(f"Exception was thrown: {pyt_err}") | ||
| self.skipTest(f"After {tries} tries, no timout could be envoked. Skip test.") |
There was a problem hiding this comment.
you don't want to skip on timeout, just fail
test/test_client.py
Outdated
| for i in range(tries): | ||
| self.client.as_db_read(dbnumber, start, size) | ||
| try: | ||
| self.client.wait_as_completion(ctypes.c_ulong(0)) |
There was a problem hiding this comment.
the try except can be removed here, the test will then just fail if an exception is raised, no reason to capture it and manually fail
There was a problem hiding this comment.
The method itself is raising an Snap7Exception in case of a timeout, which is the original behaviour of the snap7 version as well.
The goal of this test is to catch this Snap7Exception"CLI : Job Timeout" so that the behaviour of wait_as_completion(...) in case of a Job Timeout can be tested. It is possible that other Snap7Exceptions may be raised, so these have to be caught and checked.
I agree with the BaseException below in this case.
test/test_client.py
Outdated
| self.client.wait_as_completion(ctypes.c_ulong(timeout)) | ||
| except Snap7Exception as s7_err: | ||
| if s7_err.args[0] == b'CLI : Job Timeout': | ||
| self.fail(f"Request was timeouted after {timeout} seconds - FAIL") |
There was a problem hiding this comment.
the try except can be removed here, the test will then just fail if an exception is raised, no reason to capture it and manually fail
Remove unnecessary exceptions
| except Snap7Exception as s7_err: | ||
| if not s7_err.args[0] == b'CLI : Job Timeout': | ||
| self.fail(f"While waiting another error appeared: {s7_err}") | ||
| return |
There was a problem hiding this comment.
why do you return here? shouldn't it just continue?
There was a problem hiding this comment.
Otherwise I would stuck in the loop, testing the behaviour multiple times(which is unnecessarry) and then, if the loop would be done, the test would fail.
So if once a Job Timeout was raised correctly, the test shall stop here and pass (via return).
| except Snap7Exception as s7_err: | ||
| if not s7_err.args[0] == b'CLI : Job Timeout': | ||
| self.fail(f"While waiting another error appeared: {s7_err}") | ||
| return |
Adds two checks for as request purposes as sync/wrapper method, Tests included.
I want to mention, that check_as_completion is triggering errors, because a PendingError is logged as "Other Socker error(1)" in logger.error level. So these will appear always, if a job is pending (which is annoying).
Either there is a way we may "ignore" this error in case it is just a pending job or we have to live with those unecessarry logs.
The logger.level for the "ignore that" message is error, so it appears with the PendingError log as well (otherwise this may be confusing when errors are logged but the "don't worry" log doesn't show up).