search

napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

Improve coverage of get_network_driver() #263

Closed bewing closed 7 years ago

bewing commented 7 years ago

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 commented 7 years ago

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

mirceaulinic commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

bewing commented 7 years ago

@dbarrosop https://github.com/napalm-automation/napalm-base/blob/develop/napalm_base/__init__.py#L109

dbarrosop commented 7 years ago

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

bewing commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

dbarrosop commented 7 years ago

LGTM @ktbyers @mirceaulinic any comments?