Fix ECN config update test skipping WRED profile validation (#23597)#23598
Closed
yxieca wants to merge 1 commit into
Closed
Fix ECN config update test skipping WRED profile validation (#23597)#23598yxieca wants to merge 1 commit into
yxieca wants to merge 1 commit into
Conversation
When WRED profiles with zero deltas are skipped during patch generation, their expected values were also excluded from ASIC DB validation. This caused the validation to fail because the ASIC DB contains entries for all profiles but the expected values dict only contained modified ones. Fix by including skipped profiles' current values in new_values so the ASIC DB comparison accounts for all WRED profiles. Fixes sonic-net#23597 Signed-off-by: Ying Xie <ying.xie@microsoft.com>
Collaborator
|
/azp run |
github-actions
Bot
requested review from
isabelmsft,
xincunli-sonic and
yutongzhang-microsoft
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
Author
|
🤖 This PR was generated with the assistance of an AI agent (OpenClaw/sonic-mgmt). Issue: #23597 — ECN config update test failure after PR #23319 merged Root cause: When WRED profiles have zero deltas (e.g., Q4/Q5 with identical min/max thresholds), they are correctly skipped during JSON patch generation but were also excluded from the expected values dictionary. The ASIC DB validation then fails because it compares all WRED objects against only the modified subset. Fix: Include skipped profiles' current values in the expected values dictionary so the validation comparison is complete. |
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description of PR
Summary:
Fix
test_ecn_config_updates[None-replace-green_max_threshold]failure introduced by PR #23319. When WRED profiles have zero deltas, their expected values were excluded from ASIC DB validation, causing sorted comparison to fail due to list length mismatch.Fixes #23597
Type of change
Back port request
Approach
What is the motivation for this PR?
PR #23319 introduced ECN config update tests that generate JSON patches for WRED profile modifications. When a WRED profile has zero deltas (current value is already at the boundary, so no change is possible), it is correctly skipped during patch generation. However, the expected values dict (
new_values) also excluded these profiles. Since ASIC DB contains entries for all WRED profiles, the sorted comparison between ASIC DB values and expected values fails — the lists have different lengths.How did you do it?
Include skipped profiles' current (unchanged) values in
new_valuesso the ASIC DB comparison accounts for all WRED profiles, not just the modified ones.How did you verify/test it?
Tested on local KVM T0 testbed —
test_ecn_config_updates[None-replace-green_max_threshold]passes with the fix.Any platform specific information?
N/A — generic test framework fix.
Supported testbed topology if it's a new test case?
N/A — bug fix.
Documentation
N/A
This PR was generated with the assistance of an AI agent on behalf of @yxieca.