Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamic Port Cfg - VS test #12
Dynamic Port Cfg - VS test #12
Changes from 2 commits
2282fea
aca4357
a93e80c
87f1edd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse part of the DVSPort remove_port ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to remove the buffer cfg dependencies without removing the port, the removal of the port will be inside the loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a function in DVSPort -
remove_port_dependencies
that will be used here and inremove_port
function?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that in case new dependencies added we need to modify test cases that do that instaed of modifying one function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are both using the same remove function:
self.dvs_port.remove_port(key) - for removing port dependencies and the port itself
between remove and add port I decided to add different sleep timeouts so a function that will remove and add ports needs to use the I variable inside and it will make the function a bit complex
So I prefer to leave it as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is 10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 times for adding and removing ports.
I wanted the test to remove and add ports in a loop, since i dont wat the test execution runtime will take too much, 10 times is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to create a named constant value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add a constant value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the logic behind (i+1)%3 ? Same comment for L104
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted the sleep time between removing and adding ports will be different in each iteration so I used this calculation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid sleep ? Same comment for L104
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need sleep between removing and adding ports - it takes few seconds until all ports are removed/created
since we are using (i+1)%3 we have different timeout for each iteration.
different sleep timeout between add and remove ports can detect bugs, for example we found a bug on LLDP that was reproduced only on specific timeout values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test case designed to cover some real bug? VS does not have LLDP. How are you detecting those bugs? The only assert I see after sleep is the expectation on the number of SAI_OBJECT_TYPE_PORT objects. It will capture a situation when creation of the port fails in case there is specific sleep time interval after port deletion. Is it even a possible scenario? Can't we just assume that if we successfully deleted the port and the re-creation of the port succeded with 0 sec sleep in between it will work with any sleep interval? What is happening in the system during that sleep that can affect the test case result? The asic.db.wait_for_n_keys will sleep till the entry will appear - will it affect somehow your sleep interval logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test case is not covering a real bug, the lldp isue that I found was on real switch and it is just an example for race conditions that related to timeouts between delete and create.
I wanted to add several different timeouts between delete and create in order to cover such cases of race conditions that may occur between orchagent and syncd.
I guess that VS switch is not like the real switch and such cases of race conditions will probably be more relevant to real switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell from your comment is that these sleep's are redundant here as they are relevant for real switch and not for VS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't say that, maybe I wasn't clear, I think that race condition can also be on VS switch (maybe on real switch it's more common), that's why the different sleep time in each iteration is relevant also on VS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how are you catching this race condition, where the test fails when you hit it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case duplicated the code in
test_add_remove_a_port
- can we have a single test case that can be parametrized with the number of ports and then parametrize it with 1 port and N ports where N is the total number of ports in the system?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you right, it's a kind of duplicate code, we can change it but I prefer to leave it as is, in different test cases, one of them is testing only one port and the second one is testing all the ports, I think it will be more readable.