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:
|
roy-sror
left a comment
roy-sror
left a comment
There was a problem hiding this comment.
All in all looking good, Nana. Please review the minor comments. What is the test runtime?
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
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
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.
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:
|
There was a problem hiding this comment.
Consider add return None to make it more clear.
There was a problem hiding this comment.
Consider add return None to make it more clear.
There was a problem hiding this comment.
Consider use a define for port types.
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
There was a problem hiding this comment.
Consider use splitlines() instead of split("\n")
There was a problem hiding this comment.
I suggest to change to show_and_parse_loopback_action
There was a problem hiding this comment.
"expect action" -> "expected action"
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:
|
wangxin
left a comment
wangxin
left a comment
There was a problem hiding this comment.
Thanks for taking extra effort fixing code style issues in legacy code!
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
roy-sror and
wangxin
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
7 participants
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