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.

Sign up for GitHub

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

Jump to bottom

Optimize the matching rule of conditional marks #14395

Merged
merged 6 commits into from
Sep 5, 2024
Merged

Optimize the matching rule of conditional marks #14395

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a2cceb2
Get all matches
yutongzhang-microsoft Sep 3, 2024
b8b1aa4
Remove duplicate matches
yutongzhang-microsoft Sep 3, 2024
2f2d664
Optimise
yutongzhang-microsoft Sep 4, 2024
90b2490
Optimise README
yutongzhang-microsoft Sep 4, 2024
575f70b
Fix as comment
yutongzhang-microsoft Sep 4, 2024
42e602c
Optimize readme
yutongzhang-microsoft Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions tests/common/plugins/conditional_mark/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ This plugin works at the collection stage of pytest. It mainly uses two pytest h

In `pytest_collection` hook function, it reads the specified conditions file and collect some basic facts that can be used in condition evaluation. The loaded information is stored in pytest object `session.config.cache`.

In `pytest_collection_modifyitems`, it checks each collected test item (test case). For each item, it searches for the longest matches test case name defined in the conditions content. If a match is found, then it will add the marks specified for this case based on conditions for each of the marks.
Different marks in multiple files are allowed. If different marks are found, all of them will be added to the test item(test case). However, when having the duplicate mark, it will choose the first one.
In `pytest_collection_modifyitems`, it checks each collected test item (test case). For each item, it searches for all matches which are included in test case name in the conditions content.
If a match is found, and the mark is the only one in all matches, then it will add the mark to test case based on conditions. Otherwise, we will use the mark in longest match.
Different marks in multiple files are allowed. If different marks are found, all of them will be added to the test item(test case).

## How to use `--mark-conditions-files`
`--mark-conditions-files` supports exactly file name such as `tests/common/plugins/conditional_mark/test_mark_conditions.yaml` or the pattern of the file name such as `tests/common/plugins/conditional_mark/test_mark_conditions*.yaml` which will collect all files under the path `tests/common/plugins/conditional_mark` named as `test_mark_conditions*.yaml`.
Expand Down Expand Up @@ -74,9 +75,10 @@ folder3:
reason: "Skip all the test scripts under subfolder 'folder3'"
```

## Longest match rule
## Match rule

This plugin process each expanded (for parametrized test cases) test cases one by one. For each test case, the marks specified in the longest match entry in the conditions file will take precedence.
This plugin process each expanded (for parametrized test cases) test cases one by one. For each test case, it will get all matches that match the pattern of test case name.
And then, for each match, if the mark in it is unique in all matches, we will add this mark to test case based on conditions. Otherwise, we will use the mark which belongs to the longest match.

Then we can easily apply a set of marks for specific test case in a script file and another set of marks for rest of the test cases in the same script file.

Expand All @@ -88,8 +90,12 @@ feature_a/test_file_1.py:
conditions:
- "release in ['201911']"
feature_a/test_file_1.py::testcase_3:
skip:
reason: "testcase_3 should be skipped for 202311 image"
conditions:
- "release in ['202311']"
xfail:
reason: "testcase_i are suppose to fail because an issue"
reason: "testcase_3 are suppose to fail because an issue"
conditions:
- https://github.com/sonic-net/sonic-mgmt/issues/1234
```
Expand All @@ -104,10 +110,12 @@ def testcase_2

def testcase_3
```
In this example, `testcase_1` and `testcase_2` will have nodeid like `feature_a/test_file_1.py::testcase_1` and `feature_a/test_file_1.py::testcase_2`. They will match entry `feature_a/test_file_1.py`. So, the `skip` mark will be added to `testcase_1` and `testcase_2` when `release in ['201911']`.
For `testcase_3`, its nodeid will be `feature_a/test_file_1.py::testcase_3`. Then it will only match `feature_a/test_file_1.py::testcase_3`. The `xfail` mark will be added to `testcase_3` when the Github issue is still open. Entry `feature_a/test_file_1.py` also matches its nodeid. But, because it is not the longest match, it will simply be ignored.
In this example, `testcase_1` and `testcase_2` will have nodeid like `feature_a/test_file_1.py::testcase_1` and `feature_a/test_file_1.py::testcase_2`.
They will match entry `feature_a/test_file_1.py`. So, the `skip` mark will be added to `testcase_1` and `testcase_2` when `release in ['201911']`.

In a summary, under such scenario, the `skip` mark will be conditionally added to `testcase_1` and `testcase_2`. The `xfail` mark will be conditionally added to `testcase_3`.
For `testcase_3`, its nodeid will be `feature_a/test_file_1.py::testcase_3`. It will match both `feature_a/test_file_1.py` and `feature_a/test_file_1.py::testcase_3`.
For mark `xfail`, it is the only mark in all matches, so it will be added to `testcase_3` when the Github issue is still open.
And for mark `skip`, it exists in multiple matches. We will use the longest match of this match, which is under the entry `feature_a/test_file_1.py::testcase_3`.

If a test case is parameterized, we can even specify different mark for different parameter value combinations for the same test case.

Expand Down
50 changes: 32 additions & 18 deletions tests/common/plugins/conditional_mark/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,31 +402,45 @@ def load_basic_facts(session):
return results


def find_longest_matches(nodeid, conditions):
"""Find the longest matches of the given test case name in the conditions list.

This is similar to longest prefix match in routing table. The longest match takes precedence.
def find_all_matches(nodeid, conditions):
"""Find all matches of the given test case name in the conditions list.

Args:
nodeid (str): Full test case name
conditions (list): List of conditions

Returns:
str: Longest match test case name or None if not found
list: All match test case name or None if not found
"""
longest_matches = []
all_matches = []
max_length = -1
conditional_marks = {}
matches = []

for condition in conditions:
# condition is a dict which has only one item, so we use condition.keys()[0] to get its key.
if nodeid.startswith(list(condition.keys())[0]):
length = len(list(condition.keys())[0])
if length > max_length:
max_length = length
longest_matches = []
longest_matches.append(condition)
elif length == max_length:
longest_matches.append(condition)
return longest_matches
all_matches.append(condition)

for match in all_matches:
test_case = list(match.keys())[0]
yutongzhang-microsoft marked this conversation as resolved.
Show resolved Hide resolved
length = len(test_case)
marks = match[test_case].keys()
for mark in marks:
if conditional_marks.get(mark):
yutongzhang-microsoft marked this conversation as resolved.
Show resolved Hide resolved
if length > max_length:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@arista-nwolfe arista-nwolfe Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

conditional_marks.update({mark: match})
yutongzhang-microsoft marked this conversation as resolved.
Show resolved Hide resolved
max_length = length
else:
conditional_marks.update({mark: match})

# We may have the same matches of different marks
# Need to remove duplicate here
for match in list(conditional_marks.values()):
if match not in matches:
matches.append(match)

return matches


def update_issue_status(condition_str, session):
Expand Down Expand Up @@ -582,12 +596,12 @@ def pytest_collection_modifyitems(session, config, items):
json.dumps(basic_facts, indent=2)))
dynamic_update_skip_reason = session.config.option.dynamic_update_skip_reason
for item in items:
longest_matches = find_longest_matches(item.nodeid, conditions)
all_matches = find_all_matches(item.nodeid, conditions)

if longest_matches:
logger.debug('Found match "{}" for test case "{}"'.format(longest_matches, item.nodeid))
if all_matches:
logger.debug('Found match "{}" for test case "{}"'.format(all_matches, item.nodeid))

for match in longest_matches:
for match in all_matches:
# match is a dict which has only one item, so we use match.values()[0] to get its value.
for mark_name, mark_details in list(list(match.values())[0].items()):
conditions_logical_operator = mark_details.get('conditions_logical_operator', 'AND').upper()
Expand Down
Loading