napalm-automation / napalm-base

Apache License 2.0
33 stars 48 forks source link

__enter__ was swallowing the exception and returning False #299

Closed dbarrosop closed 6 years ago

dbarrosop commented 6 years ago

Solves #297

Do not merge yet. Looks like old implementation was good for py3.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.09%) to 72.0% when pulling b45a7695834605efe73742fe7662bf1c8f67ecf6 on cm-issue-297 into 42729d5129fdaaf02b45be614d2449425c91daed on develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.2%) to 72.338% when pulling 52f7d8c2a145670f6864454a3df2590c5f4a18a8 on cm-issue-297 into 42729d5129fdaaf02b45be614d2449425c91daed on develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.6%) to 72.727% when pulling 6ffe8ab222c527c39290cd0e02f08100a25781e2 on cm-issue-297 into 42729d5129fdaaf02b45be614d2449425c91daed on develop.

bewing commented 6 years ago

wooo 73 (rounded)

bewing commented 6 years ago

On further reflection, the additional unit test isn't all that effective -- it looks like only open() was eating exceptions, and we can't test exceptions in MockDriver.open() without defining an optional_arg that FORCES MD.open() to throw something.

bewing commented 6 years ago

cherry-picked the new unittest stuff into develop and confirmed that it fails there, so we are actually testing something new this time:

[bewing:/mnt/d/PyCharm/napalm-base] [napalm] develop ± git checkout -b test_regression
[bewing:/mnt/d/PyCharm/napalm-base] [napalm] test_regression ± git cherry-pick 6ffe8ab222c527c39290cd0e02f08100a25781e2
[test_regression 6549b16] Unit test for context manager exceptions
 Author: bewing <nicotine@warningg.com>
 Date: Tue Aug 8 19:42:01 2017 -0500
 1 file changed, 4 insertions(+), 1 deletion(-)
[bewing:/mnt/d/PyCharm/napalm-base] [napalm] test_regression ± git cherry-pick 1f62ad1efcab796bf792d46d257aa4c2cad099d5
[test_regression 1b32943] Test exception handling in __open__
 Author: bewing <nicotine@warningg.com>
 Date: Tue Aug 8 20:09:20 2017 -0500
 2 files changed, 12 insertions(+)
[bewing:/mnt/d/PyCharm/napalm-base] [napalm] test_regression ± tox
====================================================== FAILURES =======================================================
_________________________________________ TestMockDriver.test_context_manager _________________________________________

self = <test_mock_driver.TestMockDriver object at 0x7f3ec6886cf8>

    def test_context_manager(self):
        with pytest.raises(napalm_base.exceptions.ConnectionException) as e, \
                driver("blah", "bleh", "blih", optional_args=fail_args) as d:
>           pass
E           Failed: DID NOT RAISE <class 'napalm_base.exceptions.ConnectionException'>

test/unit/test_mock_driver.py:53: Failed
=================================== 1 failed, 66 passed, 37 skipped in 3.37 seconds ===================================
ERROR: InvocationError: '/mnt/d/PyCharm/napalm-base/.tox/py36/bin/py.test'
_______________________________________________________ summary _______________________________________________________
ERROR:   py27: commands failed
ERROR:   py34: commands failed
ERROR:   py35: commands failed
ERROR:   py36: commands failed
coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.7%) to 72.833% when pulling 1f62ad1efcab796bf792d46d257aa4c2cad099d5 on cm-issue-297 into 42729d5129fdaaf02b45be614d2449425c91daed on develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.5%) to 72.562% when pulling c682560bb6237e187976797b800592f98e9b3c8c on cm-issue-297 into 42729d5129fdaaf02b45be614d2449425c91daed on develop.

dbarrosop commented 6 years ago

Did some more refactoring, I think this one is good to go.

Great work with the test :)

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.5%) to 72.562% when pulling c682560bb6237e187976797b800592f98e9b3c8c on cm-issue-297 into 42729d5129fdaaf02b45be614d2449425c91daed on develop.

bewing commented 6 years ago

LGTM

Mierdin commented 6 years ago

Thanks for this - I noticed this behavior a few weeks ago in https://github.com/StackStorm-Exchange/stackstorm-napalm - came this close to ripping out our with statements. :)