Skip to content
This repository has been archived by the owner on Sep 17, 2019. It is now read-only.

__enter__ was swallowing the exception and returning False #299

Merged
merged 4 commits into from
Aug 10, 2017

Conversation

dbarrosop
Copy link
Member

@dbarrosop dbarrosop commented Aug 8, 2017

Solves #297

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

@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Aug 9, 2017

Coverage Status

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

@bewing
Copy link
Member

bewing commented Aug 9, 2017

wooo 73 (rounded)

@bewing
Copy link
Member

bewing commented Aug 9, 2017

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
Copy link
Member

bewing commented Aug 9, 2017

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
Copy link

coveralls commented Aug 9, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Aug 9, 2017

Coverage Status

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

@dbarrosop
Copy link
Member Author

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

Great work with the test :)

@coveralls
Copy link

coveralls commented Aug 9, 2017

Coverage Status

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

@bewing
Copy link
Member

bewing commented Aug 9, 2017

LGTM

@dbarrosop dbarrosop merged commit cb9ecd4 into develop Aug 10, 2017
@bewing bewing deleted the cm-issue-297 branch August 10, 2017 11:15
@Mierdin
Copy link

Mierdin commented Aug 24, 2017

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. :)

dbarrosop pushed a commit that referenced this pull request Aug 26, 2017
* Validate string before evaluating (#274)

* Bump version to 0.24.3 (#275)

* Fix napalm validate so identical strings will work (#288)

* Correct link under "compliance_report" function (#289)

Fixing a bad link in the documentation for "compliance_report" function.

* Convert napalm_base testing to tox (#284)

* Convert napalm_base testing to tox

* Update requirements and travis config

* @ogenstad to the rescue

* Remove unnecessary conftest.py

* Omit unit tests from coverage report

* py23 text_type

* Search parent modules in load_template

If an explicit template_path is not specified, build a list of
directories from the class MRO and pass them to FileSystemLoader

* load_template unit test fixes

* Test search path building

* If tox was merged I would've caught this

* PEP8

* Didn't use this subclass

* pylama

* Rename template directory

* Add requested value to the validate report (#291)

* Clean up search_path creation

* __enter__ was swallowing the exception and returning False (#299)

* __enter__ was swallowing the exception and returning False

* Unit test for context manager exceptions

* Test exception handling in __open__

* Refactoring context manager

* Remove constraint on _rpc method for MockDriver that requires junos profile (#301)

* Implemented generic napalm tool with debugging capabilities (#292)

* Implemented cl_napalm_show_tech

* Typo

* Typo

* Hide password

* Generalize napalm command tool

* Fix help

* Added deprecation warnings and rename to napalm

* unnecessary

* Minor fix

* Make pylama happy

* py3 requires you to cast keys and values into a list (#300)

* Version 0.25.0 (#302)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants