Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unit tests: add pyroute2 interface dict test #41533

Merged
merged 4 commits into from Jun 1, 2017

Conversation

Projects
None yet
3 participants
@svinota
Copy link
Contributor

svinota commented May 31, 2017

Bug-Url: #41487
Bug-Url: #41461

def test_interface_dict_fields(self):
with IPDB() as ipdb:
for attr in ATTRS:
self.assertIn(attr, ipdb.interfaces[1])

This comment has been minimized.

@isbm

isbm May 31, 2017

Contributor

I would add also ipdb.interfaces[x] not in ATTRS test.

This comment has been minimized.

@svinota

svinota May 31, 2017

Author Contributor

I would not, definitively. All the interface attrs are several hundreds, and the list differs for different interface types. And the number increases with time. I don't think that the salt framework developers will be happy to maintain the full list.

This comment has been minimized.

@isbm

isbm May 31, 2017

Contributor

Point is, to find out if ATTRS contains something that is unused and thus stale.

This comment has been minimized.

@svinota

svinota May 31, 2017

Author Contributor

Just to elaborate on the code, since I'm not sure I understand the comment correctly.

The issue #41461 was filed after an exception — there was change attr in ATTRS, removed from the IPDB high-level logic.

The condition on line 12 checks, if each and every attr in ATTRS is available via the Interface dict in IPDB (here ipdb.interfaces[1] in 99% will mean the loopback interface, I believe it's OK)

Thus we can be sure that ATTRS is a subset of the pyroute2.ipdb.interfaces.Interface API.

Excuse me, if I miss something :)

This comment has been minimized.

@isbm

isbm May 31, 2017

Contributor

ah, the .interfaces increases. OK. We're good to go. Thanks!

This comment has been minimized.

@svinota

svinota Jun 1, 2017

Author Contributor

ipdb.interfaces is a dictionary-like structure that contains Interface objects, referenced by indices and names. Thus there (almost) always will be an interface with index 1 — the loopback interface: ipdb.interfaces['lo'] == ipdb.interfaces[1].

This comment has been minimized.

@svinota

svinota Jun 1, 2017

Author Contributor

Maybe it's a good idea to add a comment in the code also…


from pyroute2 import IPDB
from tests.support.unit import TestCase
from salt.beacons.network_settings import ATTRS

This comment has been minimized.

@isbm

isbm May 31, 2017

Contributor

Absolute import will fail and I usually would avoid this. Add at the beginning the following, if you want still stick to it:

from __future__ import absolute_import

This comment has been minimized.

@svinota

svinota May 31, 2017

Author Contributor

@isbm thanks, I do that way.

This comment has been minimized.

@svinota

svinota May 31, 2017

Author Contributor

done

@isbm

This comment has been minimized.

Copy link
Contributor

isbm commented May 31, 2017

@svinota you still forgot to tell -*- coding: UTF-8 -*- as per PEP0263. Then we're good to go.

@svinota

This comment has been minimized.

Copy link
Contributor Author

svinota commented Jun 1, 2017

@isbm done

@isbm

This comment has been minimized.

Copy link
Contributor

isbm commented Jun 1, 2017

@svinota but pyroute2 is optional. You shoult try/except it and skip the test, if none found. Please do it with @skipif decorator (see examples all around in our tests/unit/...). This is why it fails at the moment.

@isbm

This comment has been minimized.

Copy link
Contributor

isbm commented Jun 1, 2017

@cachedout it is a very good idea to have a some sort of script that would detect what 3rd party modules are imported and actually pip-install them during your testing sessions, so all tests are running and as minimum as possible skipped.

Just a suggestion that I would do.

@svinota

This comment has been minimized.

Copy link
Contributor Author

svinota commented Jun 1, 2017

@isbm ok, I add the decorator.

unit tests: test_pyroute2 -- add skipIf
... and comments

Bug-Url: #41533
@svinota

This comment has been minimized.

Copy link
Contributor Author

svinota commented Jun 1, 2017

@isbm

  • the decorator: done
  • comments: done
  • manual run: works
@isbm

isbm approved these changes Jun 1, 2017

Copy link
Contributor

isbm left a comment

@svinota thanks for all changes.
@cachedout I think we are green here (another integration failing test is unrelated here)

.oO(Hmm. Why merging is still blocked after approval?)

@rallytime

This comment has been minimized.

Copy link
Contributor

rallytime commented Jun 1, 2017

@isbm I think the approval must come from a salt admin in order for the GH check to go green. Also, should this test and the associated fix in #41487 go into the 2016.11 branch? Looks like that might be a good bug fix to get in.

I can back-port it easily, just wanted to double check.

@rallytime
Copy link
Contributor

rallytime left a comment

Thanks for adding this @svinota!

@rallytime rallytime merged commit 361fbe6 into saltstack:develop Jun 1, 2017

3 of 6 checks passed

default Pull Requests » Salt PR - Main Build #4718
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt Linode Ubuntu14.04 #11655 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #11767 — FAILURE
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #14275 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #7089 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #11625 — SUCCESS
Details
@isbm

This comment has been minimized.

Copy link
Contributor

isbm commented Jun 2, 2017

@rallytime I would certainly backport it everywhere else.

rallytime added a commit to rallytime/salt that referenced this pull request Jun 5, 2017

rallytime added a commit to rallytime/salt that referenced this pull request Jun 5, 2017

rallytime added a commit to rallytime/salt that referenced this pull request Jun 5, 2017

rallytime added a commit that referenced this pull request Jun 6, 2017

isbm added a commit to openSUSE/salt that referenced this pull request Jun 6, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack/salt#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack/salt#41487
Bug-Url: saltstack/salt#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack/salt#41533

isbm added a commit to openSUSE/salt that referenced this pull request Jun 7, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack/salt#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack/salt#41487
Bug-Url: saltstack/salt#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack/salt#41533

Update new pyroute2 unit test to conform with 2016.11 branch standards

dincamihai added a commit to openSUSE/salt that referenced this pull request Jun 16, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack/salt#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack/salt#41487
Bug-Url: saltstack/salt#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack/salt#41533

Update new pyroute2 unit test to conform with 2016.11 branch standards

dincamihai added a commit to openSUSE/salt that referenced this pull request Jun 16, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack/salt#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack/salt#41487
Bug-Url: saltstack/salt#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack/salt#41533

Update new pyroute2 unit test to conform with 2016.11 branch standards

isbm added a commit to isbm/salt that referenced this pull request Jun 16, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack#41487
Bug-Url: saltstack#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack#41533

Update new pyroute2 unit test to conform with 2016.11 branch standards
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.