Refactor(eos_designs): Improve pytest coverage for network_services/route_maps.py #5317
Conversation
|
Review docs on Read the Docs To test this pull request: # Create virtual environment for this testing below the current directory
python -m venv test-avd-pr-5317
# Activate the virtual environment
source test-avd-pr-5317/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/MaheshGSLAB/ansible-avd.git@route-maps-coverage#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/MaheshGSLAB/ansible-avd.git#/ansible_collections/arista/avd/,route-maps-coverage --force
# Optional: Install AVD examples
cd test-avd-pr-5317
ansible-playbook arista.avd.install_examples |
github-actions
bot
added
the
state: CI Updated
| @@ -81,9 +81,6 @@ def _route_maps_vrf_default(self: AvdStructuredConfigNetworkServicesProtocol) -> | |||
| self._redistribute_static_to_bgp_route_map() | |||
|
|
|||
| def _route_maps_vrf_default_check(self: AvdStructuredConfigNetworkServicesProtocol) -> bool: | |||
| if not self._vrf_default_evpn: | |||
There was a problem hiding this comment.
In network_services/router_bgp.py the method _route_maps_vrf_default_check is call and with that we are already checking it self._vrf_default_evpn so this condition is useless and never met.
| @@ -158,8 +155,7 @@ def _evpn_export_vrf_default_route_map(self: AvdStructuredConfigNetworkServicesP | |||
| type="permit", | |||
| match=EosCliConfigGen.RouteMapsItem.SequenceNumbersItem.Match(["ip address prefix-list PL-STATIC-VRF-DEFAULT"]), | |||
| ) | |||
| if not sequence_numbers: | |||
There was a problem hiding this comment.
There will be always sequence number because to be reach this method one of the if condition always required
There was a problem hiding this comment.
What if I have vrf default without any subnets or static routes? or if I only have ipv6?
There was a problem hiding this comment.
This condition, defined in the _evpn_export_vrf_default_route_map method, is guaranteed to be true because the preceding checks in the _route_maps_vrf_default method ensure the host is either a WAN router, has a default VRF with a subnet, or has a default VRF with a static route. Otherwise, the code would have returned earlier.
There was a problem hiding this comment.
Also i believe we can optimize this module later on because we are iterating the filtered_tenants more then 2 times
There was a problem hiding this comment.
I agree with Mahesh regarding the fact that right now the function is only called when one of the three conditions (cf line 62) is true and so there will always be one sequence number.
That being said the code was built like this before a split I think and in case we are tented to reuse the method in another context, maybe we should mention it in the docstring?
| @@ -188,9 +184,6 @@ def _bgp_underlay_peers_route_map(self: AvdStructuredConfigNetworkServicesProtoc | |||
| match=EosCliConfigGen.RouteMapsItem.SequenceNumbersItem.Match(["ip address prefix-list PL-STATIC-VRF-DEFAULT"]), | |||
| ) | |||
|
|
|||
| if not sequence_numbers: | |||
There was a problem hiding this comment.
There will be always sequence number because to reach this method one of the if condition always required
There was a problem hiding this comment.
I agree with this statement based on where the function is being called from today.
MaheshGSLAB
changed the title
MaheshGSLAB
changed the title
Shivani-gslab
force-pushed
the
route-maps-coverage
branch
from
bdf8617 to
58d2ffd
Compare
...avd/molecule/eos_designs_unit_tests/inventory/host_vars/missing-vrf-default-ipv4-subnets.yml
Outdated
Show resolved
Hide resolved
| @@ -158,8 +155,7 @@ def _evpn_export_vrf_default_route_map(self: AvdStructuredConfigNetworkServicesP | |||
| type="permit", | |||
| match=EosCliConfigGen.RouteMapsItem.SequenceNumbersItem.Match(["ip address prefix-list PL-STATIC-VRF-DEFAULT"]), | |||
| ) | |||
| if not sequence_numbers: | |||
There was a problem hiding this comment.
What if I have vrf default without any subnets or static routes? or if I only have ipv6?
| @@ -81,9 +81,6 @@ def _route_maps_vrf_default(self: AvdStructuredConfigNetworkServicesProtocol) -> | |||
| self._redistribute_static_to_bgp_route_map() | |||
|
|
|||
| def _route_maps_vrf_default_check(self: AvdStructuredConfigNetworkServicesProtocol) -> bool: | |||
There was a problem hiding this comment.
| def _route_maps_vrf_default_check(self: AvdStructuredConfigNetworkServicesProtocol) -> bool: | |
| def _route_maps_vrf_default_check(self: AvdStructuredConfigNetworkServicesProtocol) -> bool: | |
| """This should only be called when self._vrf_default_evpn is true.""" |
There was a problem hiding this comment.
similar for the others where we rely on other logic
laxmikantchintakindi
force-pushed
the
route-maps-coverage
branch
from
562d70f to
e0ca560
Compare
|
Change Summary
Improve the pytest coverage for network_services/route_maps.py
Related Issue(s)
Fixes #
Component(s) name
arista.avd.eos_designsProposed changes
Improve the pytest coverage for network_services/route_maps.py. Coverage is 100% now
How to test
Run tox
Checklist
User Checklist
Repository Checklist