Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add as checks with tests #192
Add as checks with tests #192
Changes from 14 commits
17c3817
0889fda
2af69ee
269b6b1
588bffa
2ec55fa
4e41d59
fa1f068
1a4cf21
31d353f
7e697e1
3554715
0d8e647
51a1fd0
d235aa7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofwait_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you return here? shouldn't it just continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, skimmed too quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't want to skip on timeout, just fail