[testQosSaiDscpEcn] [test_ecn_config.py] Skip QoS ECN tests on non-j2c platforms#23318
[testQosSaiDscpEcn] [test_ecn_config.py] Skip QoS ECN tests on non-j2c platforms#23318gopi-arista wants to merge 1 commit intosonic-net:masterfrom
Conversation
PR sonic-net#20544 introduced the testQosSaiDscpEcn test and updated configurations exclusively for Jericho2 (j2c) platforms. Because equivalent configurations were not added for other platform yaml files (like td3, th2), these tests fail on non-j2c platforms. This updates tests_mark_conditions.yaml to explicitly skip testQosSaiDscpEcn and test_ecn_config.py for unsupported platforms by enforcing the broadcom-dnx asic_subtype condition. Signed-off-by: thumati.gopi <[email protected]>
|
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
StormLiangMS
left a comment
There was a problem hiding this comment.
@gopi-arista —
Template: ✅ OK
DCO: ✅ signed
CI: ✅ all green
Code Review:
[Critical] or vs and logic inversion in skip condition
Both changes replace and with or, which fundamentally changes the semantics:
Line 3928 (test_ecn_config.py):
# Before: skip when BOTH asic_type is not cisco/broadcom AND asic_subtype is not broadcom-dnx
- "asic_type not in ['cisco-8000', 'broadcom']"
# After: skip when EITHER asic_type is not cisco/broadcom OR subtype is not broadcom-dnx
- "asic_type not in ['cisco-8000', 'broadcom'] or asic_subtype not in ['broadcom-dnx']"With or, a Broadcom device with asic_subtype='memory-based' (not broadcom-dnx) would be skipped — which is likely wrong since non-DNX Broadcom should run ECN tests.
Line 4080 (testQosSaiDscpEcn): Same and → or change. This would skip the test on any Broadcom device whose subtype is NOT broadcom-dnx, which is overly broad.
Suggestion: The intent seems to be "run only on cisco-8000, broadcom, or broadcom-dnx". The correct condition with or would be:
- "asic_type not in ['cisco-8000', 'broadcom'] and asic_subtype not in ['broadcom-dnx']"(i.e., skip only when it's NEITHER broadcom/cisco NOR broadcom-dnx). This is the original condition — so why is it being changed?
testQosSaiDscpEcn block. Please coordinate.
StormLiangMS
left a comment
There was a problem hiding this comment.
@gopi-arista — The and → or change in the skip conditions is a logic bug. With or, any Broadcom device whose subtype is NOT broadcom-dnx (e.g., memory-based) would be skipped from ECN tests, which is incorrect.
Please fix the boolean logic, and coordinate with #23317 which modifies the same block.
|
The or change is intentional. As I have already explained in my initial comment, the newly introduced testQosSaiDscpEcn was parametrized with an additional ecn_5 variable. The PR #20544 added ecn_5 and updated needed configurations exclusively for Jericho2. This test was added to both the qos/test_ecn_config.py and qos/test_ecn_config.py. Because of the lack of equivalent configurations on other platform yaml files (like td3, th2), this test fails on non-j2c platforms. |
PR #20544 introduced the testQosSaiDscpEcn test and updated configurations exclusively for Jericho2 (j2c) platforms. Because equivalent configurations were not added for other platform yaml files (like td3, th2), these tests fail on non-j2c platforms.
Description of PR
Summary:
The PR Qos test case added for verifying ECN configuration based on WRED profile #20544 added a new test testQosSaiDscpEcn parametrized with ["ecn_1", "ecn_2", "ecn_3", "ecn_4", "ecn_5"]
Changed ecn_1 - ecn_4 configuration and added new ecn_5 configuration to sonic-mgmt/tests/qos/files/qos_params.j2c.yaml (Jericho2)
Did NOT make configuration changes to other platform yaml files such as td3, th2, etc.
Type of change
Back port request
Approach
What is the motivation for this PR?
PR #20544 introduced the testQosSaiDscpEcn test and updated QoS ECN configurations exclusively for Jericho2 (j2c) platforms. Because equivalent configurations were not added for other platform yaml files (like td3, th2), these tests fail on non-j2c platforms. This PR prevents those false-positive failures.
How did you do it?
Updated the skip conditions in tests/common/plugins/conditional_mark/tests_mark_conditions.yaml so that the tests only run on the supported Jericho2 platform.
Changes made:
How did you verify/test it?
Manually tested on testbed running dualtor-AA56 topology — verified that tests are skipped appropriately on non-j2c hardware without failing the test run.
Any platform specific information?
These tests will now be explicitly skipped on non-j2c ASICs (such as Tomahawk, Trident3, etc.) and will only execute on broadcom-dnx platforms.
Supported testbed topology if it's a new test case?
N/A — Skipped for non-supported platforms, , not a new test case.
Documentation
N/A — test skip condition update.