Skip to content

[bgp] Refactor test_bgp_allow_list#7137

Merged
yaqiangz merged 1 commit intosonic-net:masterfrom
yaqiangz:azure-master_refactor_bgp_allow_list
Dec 30, 2022
Merged

[bgp] Refactor test_bgp_allow_list#7137
yaqiangz merged 1 commit intosonic-net:masterfrom
yaqiangz:azure-master_refactor_bgp_allow_list

Conversation

@yaqiangz
Copy link
Contributor

@yaqiangz yaqiangz commented Dec 30, 2022

Description of PR

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(improvement)

Back port request

  • 201911
  • 202012
  • 202205

Approach

What is the motivation for this PR?

There is something confusing in current test_bgp_allow_list test. This PR is to refactor this case. It help us to add bgp_allow_list test scenes more easily in the future

How did you do it?

  1. Modify variable name
  2. Modify code structure

How did you verify/test it?

Run test_test_bgp_allow_list in t1 testbed.

bgp/test_bgp_allow_list.py::test_default_allow_list_preconfig[1] PASSED [ 25%] 
bgp/test_bgp_allow_list.py::test_allow_list[1-permit] PASSED [ 50%]
bgp/test_bgp_allow_list.py::test_allow_list[1-deny] PASSED [ 75%]
bgp/test_bgp_allow_list.py::test_default_allow_list_postconfig[1] PASSED [100%]

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@yaqiangz yaqiangz requested a review from Blueve December 30, 2022 04:39
@Blueve Blueve requested a review from StormLiangMS December 30, 2022 05:11
@Blueve
Copy link
Collaborator

Blueve commented Dec 30, 2022

@StormLiangMS could you help review this change as well?
This is a refactor, we can easily add more test cases and reuse the framework in future.

Copy link
Collaborator

@StormLiangMS StormLiangMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved with 2 questions.

ALLOW_LIST = {
'BGP_ALLOWED_PREFIXES': {
'DEPLOYMENT_ID|{}|{}'.format(DEPLOYMENT_ID, TEST_COMMUNITY): {
'DEPLOYMENT_ID|0|{}'.format(TEST_COMMUNITY): {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change to 0 from DEPLOYMENT_ID?

"t0": "t1",
"t1": "t2",
"m0": "m1",
"t2": "t3"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no t3, upstream for t2 is wan router.

@yaqiangz yaqiangz merged commit 73c620f into sonic-net:master Dec 30, 2022
yaqiangz added a commit to yaqiangz/sonic-mgmt that referenced this pull request Jan 3, 2023
What is the motivation for this PR?
There is something confusing in current test_bgp_allow_list test. This PR is to refactor this case.

How did you do it?
Modify variable name
Modify code structure
How did you verify/test it?
Run test_test_bgp_allow_list in t1 testbed.

Signed-off-by: Yaqiang Zhu <[email protected]>
yaqiangz added a commit that referenced this pull request Jan 3, 2023
What is the motivation for this PR?
Cherry-pick and resolve conflicts of this PR: #7137
There is something confusing in current test_bgp_allow_list test. This PR is to refactor this case.

How did you do it?
Modify variable name
Modify code structure
How did you verify/test it?
Run test_test_bgp_allow_list in t1 testbed.

Signed-off-by: Yaqiang Zhu <[email protected]>
maksymhedeon pushed a commit to githedgehog/sonic-mgmt that referenced this pull request Jan 5, 2023
What is the motivation for this PR?
Cherry-pick and resolve conflicts of this PR: sonic-net#7137
There is something confusing in current test_bgp_allow_list test. This PR is to refactor this case.

How did you do it?
Modify variable name
Modify code structure
How did you verify/test it?
Run test_test_bgp_allow_list in t1 testbed.

Signed-off-by: Yaqiang Zhu <[email protected]>
maksymhedeon pushed a commit to githedgehog/sonic-mgmt that referenced this pull request Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants