Skip to content
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

Enhance conditional mark logic with use_longest for special cases #17317

Conversation

yutongzhang-microsoft
Copy link
Contributor

Description of PR

In PR #16930, we refined the logic of conditional marking to ensure that the longest matching entry with a True condition is prioritized. If the condition for the longest match is False, we then consider the next longest match.

However, there is a special case where the longest match has a False condition, but we still want to adopt this False condition instead of falling back to shorter matches. For example, in a skip scenario:

  • Shorter matching entries have True conditions, meaning the test should be skipped.
  • The longest matching entry has a False condition, meaning the test should not be skipped.
  • In this case, we actually want to respect the longest match and not skip the test.

To address this, this PR introduces a new key, use_longest. When use_longest is set to True, only the conditions of the longest matching entry will be used, ensuring the intended behavior in such edge cases.

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • New Test case
    • Skipped for non-supported platforms
  • Test case improvement

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405
  • 202411

Approach

What is the motivation for this PR?

In PR #16930, we refined the logic of conditional marking to ensure that the longest matching entry with a True condition is prioritized. If the condition for the longest match is False, we then consider the next longest match.

However, there is a special case where the longest match has a False condition, but we still want to adopt this False condition instead of falling back to shorter matches. For example, in a skip scenario:

  • Shorter matching entries have True conditions, meaning the test should be skipped.
  • The longest matching entry has a False condition, meaning the test should not be skipped.
  • In this case, we actually want to respect the longest match and not skip the test.

To address this, this PR introduces a new key, use_longest. When use_longest is set to True, only the conditions of the longest matching entry will be used, ensuring the intended behavior in such edge cases.

How did you do it?

To address this, this PR introduces a new key, use_longest. When use_longest is set to True, only the conditions of the longest matching entry will be used, ensuring the intended behavior in such edge cases.

How did you verify/test it?

Add unit test test cases

tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_all_false_conditions_in_matching_path_1 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_all_false_conditions_in_matching_path_2 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_all_false_conditions_in_matching_path_3 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_contradicting_conditions PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_defalut_logic_operation PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_duplicated_conditions PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_empty_conditions PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_logic_operation_and PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_logic_operation_or PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_no_matches PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_1 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_2 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_3 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_4 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_5 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_6 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_partly_false_conditions_in_longest_entry PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_true_conditions_in_longest_entry PASSED

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@congh-nvidia congh-nvidia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wangxin wangxin merged commit a085b79 into sonic-net:master Mar 5, 2025
18 checks passed
@yutongzhang-microsoft yutongzhang-microsoft deleted the yutongzhang/use_longest_match branch March 5, 2025 07:26
nnelluri-cisco pushed a commit to nnelluri-cisco/sonic-mgmt that referenced this pull request Mar 15, 2025
…nic-net#17317)

What is the motivation for this PR?
In PR sonic-net#16930, we refined the logic of conditional marking to ensure that the longest matching entry with a True condition is prioritized. If the condition for the longest match is False, we then consider the next longest match.

However, there is a special case where the longest match has a False condition, but we still want to adopt this False condition instead of falling back to shorter matches. For example, in a skip scenario:

Shorter matching entries have True conditions, meaning the test should be skipped.
The longest matching entry has a False condition, meaning the test should not be skipped.
In this case, we actually want to respect the longest match and not skip the test.
To address this, this PR introduces a new key, use_longest. When use_longest is set to True, only the conditions of the longest matching entry will be used, ensuring the intended behavior in such edge cases.

How did you do it?
To address this, this PR introduces a new key, use_longest. When use_longest is set to True, only the conditions of the longest matching entry will be used, ensuring the intended behavior in such edge cases.

How did you verify/test it?
Add unit test test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants