Add loopback action test cases #5871
Conversation
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
0f2fc0d
to
457b543
Compare
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
2 times, most recently
from
62d207c
to
0cd7e83
Compare
|
This pull request introduces 1 alert when merging 0cd7e83b19ebafea906e0219022403e58733cf5e into 23c0aee - view on LGTM.com new alerts:
|
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
0cd7e83
to
cf7f747
Compare
|
This pull request introduces 1 alert when merging cf7f747d4353f348e460436f2c8871e42f6cc2b6 into 6da07f8 - view on LGTM.com new alerts:
|
|
@nhe-NV could you please handle LGTM new alerts and build failures? |
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
cf7f747
to
b5ebcf2
Compare
|
This pull request introduces 1 alert when merging b5ebcf29c911ee533218102d45b9bd0b45fb812d into ee04112 - view on LGTM.com new alerts:
|
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
b5ebcf2
to
192c2e9
Compare
|
This pull request introduces 1 alert when merging 192c2e93744e596409747f94365e89d5f31a8d0e into 7ae74e4 - view on LGTM.com new alerts:
|
| @@ -0,0 +1,8 @@ | |||
| import logging | |||
There was a problem hiding this comment.
well done, we should start using it for any new test, please inform the team
There was a problem hiding this comment.
All in all looking good, Nana. Please review the minor comments. What is the test runtime?
It takes around 15 mins to finish all the test cases
| :param ori_ports_configuration: original ports configuration parameters | ||
| :param ports_configuration: ports configuration parameters | ||
| """ | ||
| remove_ori_dut_port_config(duthost, ori_ports_configuration) |
There was a problem hiding this comment.
remove_ori_dut_port_config
orig sounds more natural to me than ori
There was a problem hiding this comment.
remove_ori_dut_port_config
orig sounds more natural to me than ori
Fixed
|
|
||
|
|
||
|
|
||
| @pytest.fixture(scope="package", autouse=True) |
There was a problem hiding this comment.
why wasn't it merged with 'setup' fixture?
There was a problem hiding this comment.
it can not be merged, if merged together, then teardown for the fixture backup_and_restore_config_db_package will be executed late then the recover_config, it is not expected.
| verify_traffic(duthost, ptfadapter, rif_interfaces, ports_configuration, [ACTION_FORWARD] * intf_count) | ||
| with allure.step("Configure the loopback action to {}".format(ACTION_DROP)): | ||
| config_loopback_action(duthost, rif_interfaces, [ACTION_DROP] * intf_count) | ||
| with allure.step("Verify the loopback acton is configured to drop"): |
Hi, I have added "# lgtm[py/unused-import]" for the fixture which is used in the conftest.py, but is detected as "Unused import" , but it still pop up this alerts. |
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
192c2e9
to
a124695
Compare
|
This pull request introduces 1 alert when merging a1246958b307a0237a9263f99243b4982663203c into b1866a1 - view on LGTM.com new alerts:
|
|
@roysr-nv could you please help to review following the changes requested? |
|
/azp run Azure.sonic-mgmt |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/easycla |
|
@roysr-nv please review |
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
a124695
to
006b8d6
Compare
|
This pull request introduces 1 alert when merging 006b8d6339f3cadd8f1b8cfc5e96b0e7697e1497 into 1d50696 - view on LGTM.com new alerts:
|
|
|
||
| def get_vlan_of_port(config_facts, port): | ||
| """ | ||
| Check if the port is a member of vlan, if it is then return the vlan, else return Noe |
| for vlan_name, vlan in vlan_dict: | ||
| if port in config_facts['VLAN_MEMBER'][vlan_name].keys(): | ||
| return vlan['vlanid'] | ||
|
|
There was a problem hiding this comment.
Consider add return None to make it more clear.
|
|
||
| def get_portchannel_of_port(config_facts, port): | ||
| """ | ||
| Check if the port is a member of port channel, if it is then return the portchannel, else return Noe |
| portchannel_members = config_facts['PORTCHANNEL'][portchannel].get('members') | ||
| if port in portchannel_members: | ||
| return portchannel | ||
|
|
There was a problem hiding this comment.
Consider add return None to make it more clear.
| port_type = port_conf['type'] | ||
| ip_addr = port_conf['ip_addr'] | ||
|
|
||
| if port_type == 'ethernet': |
There was a problem hiding this comment.
Consider use a define for port types.
| :param port: port name | ||
| :param ip_addr: ip address | ||
| """ | ||
| duthost.shell('config interface ip add {} {}'.format(port, ip_addr)) |
There was a problem hiding this comment.
Do we have modules that contain CLI commands implementations that we can import from?
Same comment for the following 10 functions.
There was a problem hiding this comment.
I did not find such module
| } | ||
| """ | ||
| res = duthost.shell("show ip interfaces loopback-action") | ||
| interfaces_loopback_actions = res['stdout'].split("\n")[2:] |
There was a problem hiding this comment.
Consider use splitlines() instead of split("\n")
| duthost.shell("sonic-clear rifcounters") | ||
|
|
||
|
|
||
| def show_loopback_action(duthost): |
There was a problem hiding this comment.
I suggest to change to show_and_parse_loopback_action
| for rif_interface, expected_action in zip(rif_interfaces, expected_actions): | ||
| loopback_action = interface_loopback_action_map[rif_interface] | ||
| pytest_assert(loopback_action == expected_action, | ||
| "The loopback action on {} is {}, expect action is {}".format(rif_interface, loopback_action, |
There was a problem hiding this comment.
"expect action" -> "expected action"
|
|
||
|
|
||
| @pytest.fixture(scope="package") | ||
| def ori_ports_configuration(request, duthost, ptfhost, tbinfo): |
There was a problem hiding this comment.
Consider change ori_ports_configuration to orig_ports_configuration
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
006b8d6
to
e523065
Compare
|
This pull request introduces 1 alert when merging e5230654406aec7d55606c8b41838491feb6267c into 976bb15 - view on LGTM.com new alerts:
|
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
e523065
to
a505ea3
Compare
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
a505ea3
to
00ae02e
Compare
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
|
This pull request fixes 3 alerts when merging 00ae02e369df2d64061b23084dd194808a24893c into 5bf8ec8 - view on LGTM.com fixed alerts:
|
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
2 times, most recently
from
ef47238
to
c277fea
Compare
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
c277fea
to
526cb53
Compare
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
| def test_loopback_action_basic(duthost, ptfadapter, ports_configuration): | ||
| rif_interfaces = list(ports_configuration.keys()) | ||
| intf_count = len(rif_interfaces) | ||
| with allure.step("Verify the rif loopback action default action: loopback traffic will be forwarded"): |
There was a problem hiding this comment.
Will this still work if allure is not configured?
There was a problem hiding this comment.
Yes, it works, and it is not the first time we use the allure in sonic-mgmt, such as in tests/ecmp/inner_hashing/test_inner_hashing_lag.py, we already used the allure.
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
526cb53
to
090bc7a
Compare
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
nhe-NV
requested review from
wangxin and
roy-sror
and removed request for
wangxin
Change-Id: I2f666a0ea91b3de80909362196452681e82680bc
nhe-NV
force-pushed
the
add_loopback_action_tests
branch
from
090bc7a
to
599c551
Compare
iface_loopback_action folder was new added in #5871. It uses package level fixture which will run the conftest before sanity check. In ports_configuration it did some remove vlan member or portchannel member operation, which will cause the following sanity check failure obviously. What is the motivation for this PR? Make sanity check run before conftest of iface loopback action. How did you do it? Use module level fixture instead. Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
What is the motivation for this PR? Add new test cases for loopback action How did you do it? Add 3 testcases for the loopback action feature: test_loopback_action_basic test_loopback_action_port_flap test_loopback_action_reload How did you verify/test it? Run all the new test cases, and tests pass
Description of PR
Summary: Add test cases for loopback action
Fixes # (issue)
Type of change
Back port request
Approach
What is the motivation for this PR?
Add new test cases for loopback action
How did you do it?
Add 3 testcases for the loopback action feature:
test_loopback_action_basic
test_loopback_action_port_flap
test_loopback_action_reload
How did you verify/test it?
Run all the new test cases, and tests pass
Any platform specific information?
No
Supported testbed topology if it's a new test case?
any
Documentation
https://github.com/sonic-net/SONiC/blob/master/doc/ip-interface/loopback-action/rif_loopback_action_testplan.md
https://github.com/sonic-net/SONiC/blob/master/doc/ip-interface/loopback-action/ip-interface-loopback-action-design.md