Optimize the matching rule of conditional marks #14395

yutongzhang-microsoft · 2024-09-03T07:07:15Z

Description of PR

Previously, the matching rule of the conditional marks was based on the longest match. This meant that when a test case had multiple potential matches, we only applied the mark with the longest matching entry. In this PR, we've optimized this rule: we now consider all potential matches. If a mark is unique across these matches, it will be applied to the test case. Otherwise, we will still use the longest matching entry.

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Back port request

Approach

What is the motivation for this PR?

Previously, the matching rule of the conditional marks was based on the longest match. This meant that when a test case had multiple potential matches, we only applied the mark with the longest matching entry. In this PR, we've optimized this rule: we now consider all potential matches. If a mark is unique across these matches, it will be applied to the test case. Otherwise, we will still use the longest matching entry.

How did you do it?

In this PR, we've optimized this rule of conditional mark: we now consider all potential matches. If a mark is unique across these matches, it will be applied to the test case. Otherwise, we will still use the longest matching entry.

How did you verify/test it?

We have conditional marks below

mark:
  skip:
    reason: "Skip mark"
    conditions:
      - "topo_type in ['vs']"

mark/test_mark.py:
  xfail:
    reason: "Xfail mark/test_mark.py"
    conditions:
      - "asic_type in ['vs']"
  skip:
    reason: "Skip mark/test_mark.py"
    conditions:
      - "asic_type in ['vs']"

mark/test_mark.py::test_conditional_mark:
  skip:
    reason: "Skip mark/test_mark.py::test_conditional_mark"
    conditions:
      - "asic_type in ['vs']"

And we run test case mark/test_mark.py::test_conditional_mark on kvm testbed.
We get the expected match of this test case and add them to the case

08:25:40 __init__.pytest_collection_modifyitems   L0611 INFO   | Found match "[{'mark/test_mark.py::test_conditional_mark': {'skip': {'reason': 'Skip mark/test_mark.py::test_conditional_mark', 'conditions': ["asic_type in ['vs']"]}}}, {'mark/test_mark.py': {'xfail': {'reason': 'Xfail mark/test_mark.py', 'conditions': ["asic_type in ['vs']"]}}}]" for test case "mark/test_mark.py::test_conditional_mark"
08:25:40 __init__.pytest_collection_modifyitems   L0649 INFO   | Adding mark MarkDecorator(mark=Mark(name='skip', args=(), kwargs={'reason': 'Skip mark/test_mark.py::test_conditional_mark'})) to mark/test_mark.py::test_conditional_mark
08:25:40 __init__.pytest_collection_modifyitems   L0649 INFO   | Adding mark MarkDecorator(mark=Mark(name='xfail', args=(), kwargs={'reason': 'Xfail mark/test_mark.py', 'strict': False})) to mark/test_mark.py::test_conditional_mark

Any platform specific information?

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

Documentation

tests/common/plugins/conditional_mark/__init__.py

#14434) What is the motivation for this PR? Previously, we use temporary workaround #13602 to skip traffic test in VS platform because conditional mark could only match longest prefix, but now, it's fixed in #14395, so it would be better to use a common way -- conditional mark to skip traffic test but not hardcode How did you do it? Remove skip_traffic_test fixture in duthost_utils and use fixture in ptfhost_utils Add condition with tests need to skip traffic test in tests_mark_conditions_skip_traffic_test.yaml

…rk. (#14515) Description of PR We changed the matching rule of conditional mark in PR #14395. There is a bug when getting the maximum matching length. In this PR, we fix this bug. Approach What is the motivation for this PR? We changed the matching rule of conditional mark in PR #14395. There is a bug when getting the maximum matching length. In this PR, we fix this bug. co-authorized by: jianquanye@microsoft.com

What is the motivation for this PR? Previously, the matching rule of the conditional marks was based on the longest match. This meant that when a test case had multiple potential matches, we only applied the mark with the longest matching entry. In this PR, we've optimized this rule: we now consider all potential matches. If a mark is unique across these matches, it will be applied to the test case. Otherwise, we will still use the longest matching entry. How did you do it? In this PR, we've optimized this rule of conditional mark: we now consider all potential matches. If a mark is unique across these matches, it will be applied to the test case. Otherwise, we will still use the longest matching entry. How did you verify/test it? We have conditional marks below mark: skip: reason: "Skip mark" conditions: - "topo_type in ['vs']" mark/test_mark.py: xfail: reason: "Xfail mark/test_mark.py" conditions: - "asic_type in ['vs']" skip: reason: "Skip mark/test_mark.py" conditions: - "asic_type in ['vs']" mark/test_mark.py::test_conditional_mark: skip: reason: "Skip mark/test_mark.py::test_conditional_mark" conditions: - "asic_type in ['vs']" And we run test case mark/test_mark.py::test_conditional_mark on kvm testbed. We get the expected match of this test case and add them to the case 08:25:40 __init__.pytest_collection_modifyitems L0611 INFO | Found match "[{'mark/test_mark.py::test_conditional_mark': {'skip': {'reason': 'Skip mark/test_mark.py::test_conditional_mark', 'conditions': ["asic_type in ['vs']"]}}}, {'mark/test_mark.py': {'xfail': {'reason': 'Xfail mark/test_mark.py', 'conditions': ["asic_type in ['vs']"]}}}]" for test case "mark/test_mark.py::test_conditional_mark" 08:25:40 __init__.pytest_collection_modifyitems L0649 INFO | Adding mark MarkDecorator(mark=Mark(name='skip', args=(), kwargs={'reason': 'Skip mark/test_mark.py::test_conditional_mark'})) to mark/test_mark.py::test_conditional_mark 08:25:40 __init__.pytest_collection_modifyitems L0649 INFO | Adding mark MarkDecorator(mark=Mark(name='xfail', args=(), kwargs={'reason': 'Xfail mark/test_mark.py', 'strict': False})) to mark/test_mark.py::test_conditional_mark Any platform specific information?

sonic-net#14434) What is the motivation for this PR? Previously, we use temporary workaround sonic-net#13602 to skip traffic test in VS platform because conditional mark could only match longest prefix, but now, it's fixed in sonic-net#14395, so it would be better to use a common way -- conditional mark to skip traffic test but not hardcode How did you do it? Remove skip_traffic_test fixture in duthost_utils and use fixture in ptfhost_utils Add condition with tests need to skip traffic test in tests_mark_conditions_skip_traffic_test.yaml

…rk. (sonic-net#14515) Description of PR We changed the matching rule of conditional mark in PR sonic-net#14395. There is a bug when getting the maximum matching length. In this PR, we fix this bug. Approach What is the motivation for this PR? We changed the matching rule of conditional mark in PR sonic-net#14395. There is a bug when getting the maximum matching length. In this PR, we fix this bug. co-authorized by: jianquanye@microsoft.com

What is the motivation for this PR? Previously, the matching rule of the conditional marks was based on the longest match. This meant that when a test case had multiple potential matches, we only applied the mark with the longest matching entry. In this PR, we've optimized this rule: we now consider all potential matches. If a mark is unique across these matches, it will be applied to the test case. Otherwise, we will still use the longest matching entry. How did you do it? In this PR, we've optimized this rule of conditional mark: we now consider all potential matches. If a mark is unique across these matches, it will be applied to the test case. Otherwise, we will still use the longest matching entry. How did you verify/test it? We have conditional marks below mark: skip: reason: "Skip mark" conditions: - "topo_type in ['vs']" mark/test_mark.py: xfail: reason: "Xfail mark/test_mark.py" conditions: - "asic_type in ['vs']" skip: reason: "Skip mark/test_mark.py" conditions: - "asic_type in ['vs']" mark/test_mark.py::test_conditional_mark: skip: reason: "Skip mark/test_mark.py::test_conditional_mark" conditions: - "asic_type in ['vs']" And we run test case mark/test_mark.py::test_conditional_mark on kvm testbed. We get the expected match of this test case and add them to the case 08:25:40 __init__.pytest_collection_modifyitems L0611 INFO | Found match "[{'mark/test_mark.py::test_conditional_mark': {'skip': {'reason': 'Skip mark/test_mark.py::test_conditional_mark', 'conditions': ["asic_type in ['vs']"]}}}, {'mark/test_mark.py': {'xfail': {'reason': 'Xfail mark/test_mark.py', 'conditions': ["asic_type in ['vs']"]}}}]" for test case "mark/test_mark.py::test_conditional_mark" 08:25:40 __init__.pytest_collection_modifyitems L0649 INFO | Adding mark MarkDecorator(mark=Mark(name='skip', args=(), kwargs={'reason': 'Skip mark/test_mark.py::test_conditional_mark'})) to mark/test_mark.py::test_conditional_mark 08:25:40 __init__.pytest_collection_modifyitems L0649 INFO | Adding mark MarkDecorator(mark=Mark(name='xfail', args=(), kwargs={'reason': 'Xfail mark/test_mark.py', 'strict': False})) to mark/test_mark.py::test_conditional_mark Any platform specific information?

sonic-net#14434) What is the motivation for this PR? Previously, we use temporary workaround sonic-net#13602 to skip traffic test in VS platform because conditional mark could only match longest prefix, but now, it's fixed in sonic-net#14395, so it would be better to use a common way -- conditional mark to skip traffic test but not hardcode How did you do it? Remove skip_traffic_test fixture in duthost_utils and use fixture in ptfhost_utils Add condition with tests need to skip traffic test in tests_mark_conditions_skip_traffic_test.yaml

…rk. (sonic-net#14515) Description of PR We changed the matching rule of conditional mark in PR sonic-net#14395. There is a bug when getting the maximum matching length. In this PR, we fix this bug. Approach What is the motivation for this PR? We changed the matching rule of conditional mark in PR sonic-net#14395. There is a bug when getting the maximum matching length. In this PR, we fix this bug. co-authorized by: jianquanye@microsoft.com

…arks (#14912) What is the motivation for this PR? As part of #11968, a bugfix was introduced in the find_longest_matches function to identify only the longest matching entry in tests_mark_conditions.yaml. The previous bug led to incorrect behavior in finding the matching entries, deviating from our intended logic before: Our expected behavior was to retrieve only the longest matching entry and evaluate the conditions under this single, unique entry. Instead, the bug caused the function to retrieve all entries matching the test case prefix, evaluating conditions from all matching entries. This resulted in an unintended situation where conditions from shorter matching entries which are absent in the longest one would still be considered during evaluation. In PR #14395, we optimized the logic for finding marks in conditional marking. Now, although we will retrieve all entries matching the test case prefix but only the longest matching entry is evaluated if the mark is identical across all matching entries, which aligns with our original expectations. Additionally, conditions that exist only in shorter matching entries with the same mark are now ignored. To maintain consistency with previous behavior(although unexpected), we have added these conditions to the longer matching entry. How did you do it? To maintain consistency with previous behavior(although unexpected), we have added these conditions to the longer matching entry.

arista-nwolfe · 2024-10-10T23:11:16Z

tests/common/plugins/conditional_mark/__init__.py

+        marks = match[case_starting_substring].keys()
+        for mark in marks:
+            if mark in conditional_marks:
+                if length > max_length:


Shouldn't the max_length be a per-mark attribute?
Otherwise if you had the following:

package: skip: condition: -<conditionA> xfail: condition: -<conditionB> package/test.py::testName: skip: condition: -<conditionC> package/test.py: xfail: condition: -<conditionD>

When the match package/test.py::testName is processed against the skip mark it will cause the max_length to be set to len('package/test.py::testName') but then later when package/test.py is processed for xfail it won't be added because it's incorrectly comparing against the max_length of the skip mark.

Hi, @arista-nwolfe, in our yaml file of conditional mark, we set all conditions in alphabet order. So we will check the case_starting_substring from short to long.

Hi, @arista-nwolfe, in our yaml file of conditional mark, we set all conditions in alphabet order. So we will check the case_starting_substring from short to long.

Can we rely on the yaml file being populated that way? If we are then you technically don't even need a if length >= max_length: check since the latter processed match will always be the same length or longer than the previous matches.

Hi, @arista-nwolfe, in our yaml file of conditional mark, we set all conditions in alphabet order. So we will check the case_starting_substring from short to long.

Can we rely on the yaml file being populated that way? If we are then you technically don't even need a if length >= max_length: check since the latter processed match will always be the same length or longer than the previous matches.

Yes, we have a pre-commit checker to check.

mssonicbld · 2024-10-14T01:51:01Z

@yutongzhang-microsoft PR conflicts with 202405 branch

yutongzhang-microsoft added 4 commits September 3, 2024 15:02

Get all matches

a2cceb2

Remove duplicate matches

b8b1aa4

Optimise

2f2d664

Optimise README

90b2490

yutongzhang-microsoft marked this pull request as ready for review September 4, 2024 06:12

yutongzhang-microsoft changed the title ~~Get all matches in conditional marks~~ Get all potential matches in conditional marks Sep 4, 2024

yutongzhang-microsoft changed the title ~~Get all potential matches in conditional marks~~ Optimize the matching rule of conditional marks Sep 4, 2024

yutongzhang-microsoft requested a review from wangxin September 4, 2024 06:22

wangxin reviewed Sep 4, 2024

View reviewed changes

tests/common/plugins/conditional_mark/__init__.py Outdated Show resolved Hide resolved

tests/common/plugins/conditional_mark/__init__.py Outdated Show resolved Hide resolved

tests/common/plugins/conditional_mark/__init__.py Outdated Show resolved Hide resolved

yutongzhang-microsoft added 2 commits September 4, 2024 15:41

Fix as comment

575f70b

Optimize readme

42e602c

wangxin approved these changes Sep 5, 2024

View reviewed changes

wangxin merged commit d03f0fb into sonic-net:master Sep 5, 2024
16 checks passed

yutongzhang-microsoft deleted the yutongzhang/get_all_matches branch September 5, 2024 06:52

xwjiang-ms mentioned this pull request Sep 5, 2024

Use conditional mark to skip traffic test and remove temporary fixture #14434

Merged

8 tasks

yutongzhang-microsoft mentioned this pull request Sep 11, 2024

[Bugfix] Bugfix for getting maximum matching length of conditional mark. #14515

Merged

8 tasks

yutongzhang-microsoft added Request for 202405 branch Request for 202012 branch Request for 202205 branch Request for 202305 branch Request for 202311 branch labels Oct 9, 2024

yutongzhang-microsoft mentioned this pull request Oct 10, 2024

[Bugfix]Adding conditions to longer matching entries in conditional marks #14912

Merged

8 tasks

yutongzhang-microsoft mentioned this pull request Oct 10, 2024

[Bug]: conditional_mark only considers the longest match in the yaml file even when skipping. #14698

Open

arista-nwolfe reviewed Oct 10, 2024

View reviewed changes

wangxin added the Approved for 202405 branch label Oct 14, 2024

mssonicbld added the Cherry Pick Conflict_202405 label Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize the matching rule of conditional marks #14395

Optimize the matching rule of conditional marks #14395

yutongzhang-microsoft commented Sep 3, 2024 •

edited

Loading

arista-nwolfe Oct 10, 2024

yutongzhang-microsoft Oct 11, 2024

arista-nwolfe Oct 11, 2024 •

edited

Loading

yutongzhang-microsoft Oct 14, 2024

mssonicbld commented Oct 14, 2024

Optimize the matching rule of conditional marks #14395

Optimize the matching rule of conditional marks #14395

Conversation

yutongzhang-microsoft commented Sep 3, 2024 • edited Loading

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

arista-nwolfe Oct 10, 2024

Choose a reason for hiding this comment

yutongzhang-microsoft Oct 11, 2024

Choose a reason for hiding this comment

arista-nwolfe Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

yutongzhang-microsoft Oct 14, 2024

Choose a reason for hiding this comment

mssonicbld commented Oct 14, 2024

yutongzhang-microsoft commented Sep 3, 2024 •

edited

Loading

arista-nwolfe Oct 11, 2024 •

edited

Loading