WIP: Added pytest tests

[ogenstad]

I don't know if anyone is already looking at this, but after the discussion in the chat about the bgp issues when parsing IOS devices I started to look at the test framework.

This PR only has mock data from one test, and even that should probably be changed from the old mock data I just copied. I just wanted to get pytest up and running.

[coveralls]

Coverage remained the same at 68.859% when pulling 129b5b770c07aed78e061c8aa814d3598f05431b on ogenstad:pytest into b6c386f149d915fc6f116d64c854f188221af331 on napalm-automation:develop.

[mirceaulinic]

Thanks @ogenstad! From here on, in order to complete the migration, we only need to move the location of the existing mock files and/or rename. Can you do that too?

[ktbyers]

@mirceaulinic @dbarrosop One of you should review/approve this as I have been out of the loop on the changes for migrating to the new testing framework.

[ogenstad]

@mirceaulinic I didn't dive too deeply into the old test framework. But it looked like that only verified that the models were correct and didn't care about the data. I don't think there is any expected data for the existing mock files?

I think it has to be created and I was thinking that it would be easier to just create new scrubbed real data for that and just leave the existing mock data as is. Then it could be deleted in a later PR. Thoughts on this?

[dbarrosop]

Nice! We need two things to get started with this now you did all the boilerplate : )

1. Change .travis.ymlto do two things: do a curl after a success and replace the nosetests commands with a single py.test. Check napalm-eos for reference.
2. Remove the tox.ini, please. That's a discussion we still have to have.

[ogenstad]

You mean this one? if [ $TRAVIS_TAG ]; then curl -X POST https://readthedocs.org/build/napalm; fi ?

Regarding tox.ini, is that really a problem as long as Travis doesn't use it? I find that it helps with local testing.

[dbarrosop]

You mean this one?

Yes

Regarding tox.ini, is that really a problem as long as Travis doesn't use it? I find that it helps with local testing.

It is not a problem but I'd prefer not to have it for consistency and because if we decide on something else in the future we have to remember that we had this here. An alternative is to discuss on this PR if that's what we want and how we want it. @mirceaulinic @ktbyers do you want to take this opportunity to look into this?

[coveralls]

Coverage remained the same at 68.859% when pulling 30495e5295dcdbdaefdc95776ca8b728cc10d8b5 on ogenstad:pytest into b6c386f149d915fc6f116d64c854f188221af331 on napalm-automation:develop.

[mirceaulinic]

@ogenstad

I don't think there is any expected data for the existing mock files?

I don't know much about how it works the existing framework for ios (read: I have no idea), but I can see there are some files under https://github.com/napalm-automation/napalm-ios/tree/develop/test/unit/ios/mock_data and I was wondering if they could be re-used? E.g.: test/unit/ios/mock_data/show_lldp_neighbors.txt could probably be moved under test/unit/mocked_data/test_get_lldp_neighbors/normal/show_lldp_neighbors.txt for the new framework rather than providing new mock. This is what we've done, the most recent example is https://github.com/napalm-automation/napalm-junos/pull/77

Do you think that could work here too?

[coveralls]

Coverage decreased (-3.6%) to 65.261% when pulling 8a4e088df85230cc1ce08f9720858cfea84639fb on ogenstad:pytest into b6c386f149d915fc6f116d64c854f188221af331 on napalm-automation:develop.

[ogenstad]

One example of the unicode/str issue is with these lines

                try:
                    snmp_dict['community'][name].update({'mode': fields[3].lower()})
                except IndexError:
                    snmp_dict['community'][name].update({'mode': u'N/A'})
                try:
                    snmp_dict['community'][name].update({'acl': fields[4]})
                except IndexError:
                    snmp_dict['community'][name].update({'acl': u'N/A'})

The tests doesn't actually fail for the N/A key. Haven't figured out where the format change happens though.

[coveralls]

Coverage decreased (-3.6%) to 65.261% when pulling 351c52462c520253cdbed77b35d0601b7fbed3f7 on ogenstad:pytest into b6c386f149d915fc6f116d64c854f188221af331 on napalm-automation:develop.

[coveralls]

Coverage decreased (-3.6%) to 65.261% when pulling 351c52462c520253cdbed77b35d0601b7fbed3f7 on ogenstad:pytest into b6c386f149d915fc6f116d64c854f188221af331 on napalm-automation:develop.

[coveralls]

Coverage decreased (-3.6%) to 65.261% when pulling 351c52462c520253cdbed77b35d0601b7fbed3f7 on ogenstad:pytest into b6c386f149d915fc6f116d64c854f188221af331 on napalm-automation:develop.

[ogenstad]

I saw nosetest was using napalm_base.utils.py23_compat.text_type() on all the return values. When I added that to the new framework it worked for all data.

@dbarrosop, @mirceaulinic, @ktbyers - I think this is ready for review now. Let me know if I should remove the tox.ini file (still think it would be good to have in the future), or if there are any other things you would want me to change.

[mirceaulinic]

Fantastic!

Let me know if I should remove the tox.ini file

I would say to leave, I like that!

I don't have anything to add here, thanks very much @ogenstad!

[ktbyers]

@ogenstad Okay, let me "test" this in my environment and see if I see any issues.

[dbarrosop]

Ok, so everything looks good to me. Thanks for the hard work.

The only comment I have is that I still think we should remove the tox.ini file from this PR or decide that we want it and create issues to make sure we add them to the rest of the drivers.

[ktbyers]

I agree we should remove the tox.ini and decide whether we want it or not for all repositories.

[dbarrosop]

Let's remove it then to avoid blocking this PR and then take the conversation on a dedicated PR/issue. @ogenstad can you do the honors? : )

[ktbyers]

Okay, looks good to me. Once the tox.ini is removed we can merge.

[coveralls]

Coverage decreased (-3.6%) to 65.261% when pulling 79a575503823931f1ef0bbfc30116587674f245a on ogenstad:pytest into b6c386f149d915fc6f116d64c854f188221af331 on napalm-automation:develop.

[coveralls]

Coverage decreased (-3.6%) to 65.261% when pulling 79a575503823931f1ef0bbfc30116587674f245a on ogenstad:pytest into b6c386f149d915fc6f116d64c854f188221af331 on napalm-automation:develop.

[ogenstad]

Thanks. I got rid of the tox.ini file.

[dbarrosop]

Thanks again @ogenstad for this. It was a shit-ton of not so funny work : )

[ogenstad]

Thanks @dbarrosop. I've been using Napalm for some time but never contributed. Since I saw that that BGP issue would be hard to handle with the old framework I figured it was time to give something back.

[ktbyers]

Agreed...thanks @ogenstad