Raise and handle exceptions in SNMP module #76
Conversation
stveit
force-pushed
the
snmp-exception-handling
branch
from
ad78a59
to
8e6c4ff
Compare
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
==========================================
+ Coverage 94.06% 94.28% +0.23%
==========================================
Files 22 22
Lines 1144 1137 -7
==========================================
- Hits 1076 1072 -4
+ Misses 68 65 -3
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Vendortask cant continue if a timeouterror occurs, so it should just let it go further up the chain
stveit
force-pushed
the
snmp-exception-handling
branch
from
8e6c4ff
to
31a45b5
Compare
There was a problem hiding this comment.
This looks like a very nice improvement on the SNMP adapter code!
On some level, I kind of like the way you've grouped the various Exception-related tests in classes, but I also find it makes it somewhat hard to reason about which tests that exercise a function I'm looking at. Maybe not a big problem with modern IDEs (also, since I've taken to look at mutmut, I've had to switch on dynamic contexts for coverage measurements, so that mutmut can trace which tests cover which code lines)
| try: | ||
| await self._get_sysuptime() | ||
| except TimeoutError: |
There was a problem hiding this comment.
A note for future reference (not to be fixed by this PR): Depending on the original Zino implementation, this function should probably record the device boot time to its state. I've added a utility function to DeviceState for this in one of the PRs I'm currently working on, as the original link state checker did this.
for "unreachable_task", a mock raises timeouterror instantly so setting low timeout value has no purpose. for "snmp_client", its not supposed to timeout so setting a low timeout to speed things up does not make sense
This reverts commit f68677b.
As demonstrated in the code, error_indication can apparantly be a errind.RequestTimedOut object, which is a subclass of errind.ErrorIndication. So im guessing any type of errind.ErrorIndication could be used there. Im leaving string as union, because the docstring for getCmd does state it is supposed to be a str value, so im leaving it open for now
|
Kudos, SonarCloud Quality Gate passed!
|
|
Fixed the stuff mentioned in the review, and also updated typing for I have a pretty strong suspicion all the return values from the various SNMP commands (getCmd, bulkCmd) etc, all have misleading docstrings. I am pretty sure I am leaving any more fixes for this to a separate issue #82 |
Yeah, the library is confusing enough at certain points. It could do with some proper type annotations, at least. I did read somewhere in those docs just yesterday that
Great :) |
Fixes #73
This PR makes snmp functions raise exceptions if something goes wrong.
A hierarchy of exceptions for snmp related errors is added for this purpose.
This will cause tasks to fail unless its natural for the task to handle certain exceptions.
For example, it will be natural for ReachableTask to handle TimeoutError, since detecting
a device not answering is the purpose of the task. VendorTask on the other hand can not function if
a timeout occurs, so it will be natural for this task to let the exception raise further up the chain.
Currently this will cause zino to crash, so a new issue will be needed for handling tasks failing.