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

Improve coverage of get_network_driver() #263

Merged
merged 4 commits into from
Jun 27, 2017

Conversation

bewing
Copy link
Member

@bewing bewing commented Jun 8, 2017

Not sure if using napalm-logs to test a real module not having NetworkDriver is correct, or if we should create a specific dummy module to test. Also test a non-string type to hit 86, napalm_eos to hit cover 94->96 jump.

@bewing
Copy link
Member Author

bewing commented Jun 8, 2017

Looks like a specific package would be better, as napalm-logs isn't Python3 ready. :(

@mirceaulinic mirceaulinic added this to the BLOCKED milestone Jun 8, 2017
@mirceaulinic
Copy link
Member

@bewing please remove napalm-logs from the requirements - this is not a regular driver: see the readme (although there's no proper documentation): https://github.com/napalm-automation/napalm-logs

@bewing
Copy link
Member Author

bewing commented Jun 8, 2017

@mirceaulinic The point was to have a napalm_ module that does NOT contain a NetworkDriver() method, to improve test coverage in __init__.py. If it's just navel-gazing, I'm OK with rejecting the PR.

@dbarrosop
Copy link
Member

@bewing what branch of the code you want to test?

@bewing
Copy link
Member Author

bewing commented Jun 8, 2017

@dbarrosop
Copy link
Member

Ok, you can probably use any module already imported. Like the sys module. It doesn't necessarily have to belong to napalm.

@bewing
Copy link
Member Author

bewing commented Jun 9, 2017

Tried that already - if the module doesn't start with napalm_, it's prepended, so passing "sys" means we hit the non-existent module check for "napalm_sys" instead of the "real module, no driver" check

@dbarrosop
Copy link
Member

if the module doesn't start with napalm_

We should probably fix that. Maybe we can add a kw argument to avoid prepending?

get_network_driver(name, prepend=True)

@bewing
Copy link
Member Author

bewing commented Jun 22, 2017

Sorry for the delay -- had some spam folder issues.

@dbarrosop
Copy link
Member

LGTM @ktbyers @mirceaulinic any comments?

Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane

@mirceaulinic mirceaulinic modified the milestones: 0.25.0, BLOCKED Jun 26, 2017
@dbarrosop dbarrosop merged commit 44ccecc into napalm-automation:develop Jun 27, 2017
@bewing bewing deleted the coverage branch June 27, 2017 21:05
dbarrosop added a commit that referenced this pull request Jul 3, 2017
* Fix mac address format

* Documentation: Fix example mac addresses to be proper form

* Correctly check all keys comply (#266)

* Improve coverage of get_network_driver() (#263)

* Improve coverage of get_network_driver

* Revert import of napalm_logs for tests

* Driver prepending optional

* Test more branches in get_network_driver

* Added AttributeError Exception to __del__ function in NetworkDriver (#268)

* Added AttributeError Exception to __del__ function in NetworkDriver class

* Added AttributeError Exception to __del__ function in NetworkDriver

* Dumb driver (#269)

* Context manager propagates exceptions properly

* Implemented generic mock driver

* Add possibility to raise Exceptions on demand

* Mock cli commands individually

* Map junos' _rpc to cli

* Mocking configuration management methods

* Added tests for configuration methods

* Bump version to 0.24.2 (#270)
bewing added a commit to bewing/napalm-base that referenced this pull request Jul 18, 2017
…to tox

* 'develop' of github.com:napalm-automation/napalm-base:
  Bump version to 0.24.3 (napalm-automation#275)
  Validate string before evaluating (napalm-automation#274)
  Bump version to 0.24.2 (napalm-automation#270)
  Dumb driver (napalm-automation#269)
  Added AttributeError Exception to __del__ function in NetworkDriver (napalm-automation#268)
  Improve coverage of get_network_driver() (napalm-automation#263)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants