Use xcvr_skip_list to skip sfp tests#4157
Use xcvr_skip_list to skip sfp tests#4157prgeor merged 1 commit intosonic-net:masterfrom prgeor:unify-sfp-skip/master
Conversation
Signed-off-by: Prince George <[email protected]>
|
This pull request introduces 1 alert when merging 8c15c5a into 1b9bc5b - view on LGTM.com new alerts:
|
|
i am ok with this approach. --skip-absent-sfp is introduce by mellanox, I suggest to notify them before we actually remove this option, give a graceful period. So, I suggest to keep them for now. I also do not think skip-absent-sfp is a good option, we should remove them, but let's give people time to adapt that. |
|
the pr description is not clear. what is "port:-1"? the reason to remove 2 is because 1? i do not see connection between 2 and 1. |
@lguohan I updated the comment. Both 1 and 2 are used to skip test on a port. Instead of having both approaches, this PR is going with approach '1' i.e use 'xcvr_skip_list' |
@lguohan checking with Mellanox team if its OK to remove --skip-absent-sfp option |
| # Get interfaces of this asic | ||
| interface_list = get_port_map(duthost, asic_index) | ||
| interfaces_per_asic = {k:v for k, v in interface_list.items() if k in phy_intfs} | ||
| interfaces_per_asic = {k:v for k, v in interface_list.items() if k in logical_intfs} |
There was a problem hiding this comment.
I had a chat with Guohan about this. I think the main issue is that we are running SFP test against disabled (admin down) ports. If we could skip the admin down ports, that would be a much easier and generic solution.
can you update PR with that approach?
There was a problem hiding this comment.
yes, we are passing all interfaces which are in conn_graph_facts that are based on the connection graph and will not include 'admin' down' port. get_physical_port_indices() is a helper function which will give indices for the input list of logical interfaces. The list of interfaces is formed in setup() fixture in test_sfp.py
|
@prgeor This PR is breaking test_sfp you modified in this PR: https://github.com/Azure/sonic-mgmt/pull/4099/files |
Signed-off-by: Prince George <[email protected]>
4 participants
Signed-off-by: Prince George [email protected]
Description of PR
This PR unifies two approaches to skip testing SFP on a port
2 is now removed in favor of 1 because 2 is not reliable since "get_presence()" itself is an API under test.
Ports under test will be those ports which are part of device connection graph.
Removed the test cases test_get_status(), test_get_position_in_parent() as the corresponding API is not used by SFP or not applicable for SFP.
Fixed compare_value_with_platform_facts() when list of SFPs can have holes due to some ports skipped from tests.
Fixes: sonic-net/sonic-buildimage#8436
Type of change
Back port request
Approach
What is the motivation for this PR?
Unify different approaches to skip testing a SFP port under one - xcvr_skip_list
How did you verify/test it?
Run tests in test_sfp.py with two scenarios:-