Add VS test cases for dynamic port cfg #2047
Conversation
Signed-off-by: tomeri <tomeri@nvidia.com>
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Are you following the vs tests section in this doc? https://github.com/Azure/SONiC/pull/900/files , if so, it seems we should add more test cases. |
I will update this section in the HLD. |
|
@zhenggen-xu - The comments on this PR were addressed. Can you please approve this PR ? |
| assert len(num) == num_of_ports-len(ports) | ||
|
|
||
| # add port | ||
| time.sleep(i%3) |
There was a problem hiding this comment.
what is this waiting for?
There was a problem hiding this comment.
I added different timeouts between delete/create to catch potential race conditions that can lead to system crush.
There was a problem hiding this comment.
Can you add this as the comments in the code ?
There was a problem hiding this comment.
it is already inside on line 9-10
but maybe it's better to put it next to the time.sleep
There was a problem hiding this comment.
i added those comments before the sleep
| num_of_ports) | ||
| assert len(num) == num_of_ports | ||
|
|
||
| time.sleep((i%2)+1) |
There was a problem hiding this comment.
what is the waiting here, and why it varies?
There was a problem hiding this comment.
same as above:
different timeouts between delete/create can catch potential race conditions
tests/test_port_add_remove.py
Outdated
| rc = dvs.servers[17].runcmd("ping -c 1 6.6.6.6") | ||
| assert rc == 0 | ||
|
|
||
| dvs.remove_vlan_member("6", "Ethernet68") |
There was a problem hiding this comment.
Change the PORT status (like admin etc) to whatever before the test case?
There was a problem hiding this comment.
changing the ports bach back to admin down was added
There was a problem hiding this comment.
Actually, we don't know if the port was admin down or not, I would think we should save the initial admin status, and then change to whatever it was before the test case.
There was a problem hiding this comment.
I noticed that on default it is down
I agree that the best thing to do is to save the initial admin status and change it back to the initial admin status in the end
Signed-off-by: tomeri <tomeri@nvidia.com>
|
@zhenggen-xu - Can you please review and approve ? |
|
@zhenggen-xu could you please help to review? |
| assert len(num) == num_of_ports-len(ports) | ||
|
|
||
| # add port | ||
| time.sleep(i%3) |
There was a problem hiding this comment.
Can you add this as the comments in the code ?
tests/test_port_add_remove.py
Outdated
| dvs.setup_db() | ||
| dvs.create_vlan("6") | ||
| dvs.create_vlan_member("6", PORT) | ||
| dvs.create_vlan_member("6", "Ethernet68") |
There was a problem hiding this comment.
Can we make Ethernet68 as defined constant too at beginning?
tests/test_port_add_remove.py
Outdated
| rc = dvs.servers[17].runcmd("ping -c 1 6.6.6.6") | ||
| assert rc == 0 | ||
|
|
||
| dvs.remove_vlan_member("6", "Ethernet68") |
There was a problem hiding this comment.
Actually, we don't know if the port was admin down or not, I would think we should save the initial admin status, and then change to whatever it was before the test case.
- add comments before the sleep - set a define for Ethernet68
|
@zhenggen-xu kindly reminder, please review recent changes following your feedback. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This is not addressed: |
Signed-off-by: tomeri tomeri@nvidia.com
DEPENDS ON: Dynamic port configuration - add port buffer cfg to the port ref counter #2022
What I did
added a vs test to check functionality of add/remove ports dynamically on run time
Why I did it
in order to test add/remove ports functionality
How I verified it
test passed
Details if related