Update net.py and bgp.py #55421
Update net.py and bgp.py #55421
Conversation
Changed NAPALM_BASE to NAPALM (napalm_base to napalm.base)
1.- Reflect new module name napalm instead of napalm-base.
2.- Catch key-exception for IPv6 if the interface does not have IPv6 enabled
3.- Support for IP secondary address
4.- the module was trowing an exemption because a variable list named "compare" was initiated with [None]. After adding items to the list there was a max(compare), and None is not supported for comparing of the list with types None and IPNetwork. So changing the initial value to the worst value as IPNetwork('0.0.0.0/0'), solved the issue.
ghost
requested a review
from 
aegiacometti
left a comment
aegiacometti
left a comment
There was a problem hiding this comment.
updated based on comments from Akm0d
|
@aegiacometti Is it possible to add some tests for these runners? |
dwoz
added
the
needs-testcase
|
I don't have enough experience to build the test cases from scratch, considering that there aren't any for these modules yet. |
|
@dwoz hi, just following up this PR. Is there anything else I can do? |
|
Hi guys! Could you please tell me what to do? Should i wait? Should i do something else? Thanks in advance |
|
I'll help write some tests for this |
Akm0d
removed
the
needs-testcase
|
|
||
| def test_neighbors(self): | ||
| ret = bgp.neighbors() | ||
| self.assertEqual(ret, []) |
There was a problem hiding this comment.
Before anything, thank you @Akm0d for helping out here. (the following comments are not aiming you or any individual, rather SaltStack's views on this community and contributors)
I'd like however to raise some questions, just for my understanding of how this process works: what does this test? What specific purpose does it serve? If I were to submit a PR now that entirely changes the behaviour of this function, would this test catch it?
I could be wrong, but I'm seeing that it doesn't test anything at all, it barely just confirms the existence of a function. So have we blocked this PR for over 6 months for tests that don't test anything?
Note, I'm not questioning the importance of the tests, but the rule itself: I understand that everything needs a test, I get that; but it's disappointing the way this rule is followed: blindly, robotically, asking contributors to toss in tests when just making a small adjustment (asking to write tests from scratch, when there isn't any) - but everyone's completely happy when throwing some non-sense tests, right? That's just silly. It may make sense to SS as "hey, our code is X% tested", but to contributors like me, it doesn't, it only makes me feel more and more loathe to contribute to this project, I'm disappointed - to put it mildly.
If SS would ever gave a damn about the community as it's lately started trumpeting left and right, would think these decisions thoroughly, also from the perspective of this community, but sadly, it doesn't.
There was a problem hiding this comment.
For what it's worth, we realized that many of our contributors are inexperienced with writing tests, so I've started a Test Clinic, streaming live on Tuesday afternoon and Tuesday mornings (Central/US Time zones).
We have been encouraging contributors to come and learn more about how to write meaningful tests, and especially bring their PRs that need tests, and I'm happy to walk through what tests would actually be meaningful, and even start writing tests.
When it comes to blindly following this rule, one thing that has stuck out to me is that code missing tests have significantly more bugs, in some cases an entire state just wouldn't even work because they're relying on functions or function signatures that literally don't even exist in the module!
And this is code that had been merged into other branches 😬.
Based on this experience, I will take code with blindly added tests that just check that a function even exists, over code without tests, any day. Granted, I'd prefer they have meaningful test coverage -- and I'm more than happy to discuss how to achieve that for any PR, during the test clinic, on IRC, Slack, or anywhere else.
We absolutely do want meaningful tests, and we realize that's difficult, and are doing as much as we can to support writing tests.
If there's something that you see that we could be doing, or doing better, please do bring that to our attention 🙏. Besides the Test Clinic, is there anything we could be doing better to support contributors in writing tests for contributions?
|
@mirceaulinic The original change was difficult to test on it's own as it was mostly a change in the imports. Another part of the difficulty in writing tests for this module is that there were no existing tests. My goal in writing "tests that don't do anything" was to make it easier to write tests for future changes to the module -- so that future contributors wouldn't be intimidated by the need to write the entire test suite for this module from scratch to make a small change. You will also notice that your PR gained significant traction once it was related to a critical issue. PRs don't get triaged and have their progress tracked the same way as issues. If an issue is created -- or linked to a PR then it gets through our process much quicker. We value your contribution so much and appreciate your involvement in the community. I personally apologize for any frustration you have felt with regards to this and hope we can have even better interactions going forward. @waynew and I have been streaming our workflow so that you can see exactly what goes through the SS core team's head as we do things like triage, review PRs, and write tests. Hopefully that helps :) |
8 participants
What does this PR do?
1.- Reflect new module name napalm instead of napalm-base. (runners/net.py and bgp.py)
2.- Catch key-exception for IPv6 if the interface does not have IPv6 enabled (runners/net.py)
3.- Net.find not working because there was an dict_key error at line 228
AttributeError: 'dict_keys' object has no attribute 'extend'.4.- Support for IP secondary address (runners/net.py)
5.- the module was trowing an exemption because a variable list named "compare" was initiated with [None]. After adding items to the list there was a max(compare), and None is not supported for comparing of the list with types None and IPNetwork. So changing the initial value to the worst value as IPNetwork('0.0.0.0/0'), solved the issue.
fixes #55222
Previous Behavior
New Behavior