From a2cceb2885c6c44fa16fce5ce8f8bd394803c96c Mon Sep 17 00:00:00 2001 From: Yutong Zhang Date: Tue, 3 Sep 2024 15:02:23 +0800 Subject: [PATCH 1/6] Get all matches --- .../plugins/conditional_mark/__init__.py | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/tests/common/plugins/conditional_mark/__init__.py b/tests/common/plugins/conditional_mark/__init__.py index cb7d6b5769..30f541cc48 100644 --- a/tests/common/plugins/conditional_mark/__init__.py +++ b/tests/common/plugins/conditional_mark/__init__.py @@ -402,31 +402,22 @@ 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 = [] - max_length = -1 + all_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) + return all_matches def update_issue_status(condition_str, session): @@ -582,12 +573,28 @@ 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) + + max_length = -1 + conditional_marks = {} + + for match in all_matches: + test_case = list(match.keys())[0] + length = len(test_case) + marks = match[test_case].keys() + for mark in marks: + if conditional_marks.get(mark): + if length > max_length: + conditional_marks[mark] = match + max_length = length + else: + conditional_marks[mark] = match + all_matches = list(conditional_marks.values()) - 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() From b8b1aa45ebacf1dcfcd3f0e89d346456384df087 Mon Sep 17 00:00:00 2001 From: Yutong Zhang Date: Tue, 3 Sep 2024 16:42:12 +0800 Subject: [PATCH 2/6] Remove duplicate matches --- .../plugins/conditional_mark/__init__.py | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/tests/common/plugins/conditional_mark/__init__.py b/tests/common/plugins/conditional_mark/__init__.py index 30f541cc48..e4652f6a58 100644 --- a/tests/common/plugins/conditional_mark/__init__.py +++ b/tests/common/plugins/conditional_mark/__init__.py @@ -413,11 +413,32 @@ def find_all_matches(nodeid, conditions): list: All match test case name or None if not found """ all_matches = [] + max_length = -1 + conditional_marks = {} + ret = [] + 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]): all_matches.append(condition) - return all_matches + + for match in all_matches: + test_case = list(match.keys())[0] + length = len(test_case) + marks = match[test_case].keys() + for mark in marks: + if conditional_marks.get(mark): + if length > max_length: + conditional_marks.update({mark: match}) + max_length = length + else: + conditional_marks.update({mark: match}) + + for match in list(conditional_marks.values()): + if match not in ret: + ret.append(match) + + return ret def update_issue_status(condition_str, session): @@ -575,22 +596,6 @@ def pytest_collection_modifyitems(session, config, items): for item in items: all_matches = find_all_matches(item.nodeid, conditions) - max_length = -1 - conditional_marks = {} - - for match in all_matches: - test_case = list(match.keys())[0] - length = len(test_case) - marks = match[test_case].keys() - for mark in marks: - if conditional_marks.get(mark): - if length > max_length: - conditional_marks[mark] = match - max_length = length - else: - conditional_marks[mark] = match - all_matches = list(conditional_marks.values()) - if all_matches: logger.debug('Found match "{}" for test case "{}"'.format(all_matches, item.nodeid)) From 2f2d664182b1e4093c4b068315c7ef39da1fa39a Mon Sep 17 00:00:00 2001 From: Yutong Zhang Date: Wed, 4 Sep 2024 10:07:49 +0800 Subject: [PATCH 3/6] Optimise --- tests/common/plugins/conditional_mark/__init__.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/common/plugins/conditional_mark/__init__.py b/tests/common/plugins/conditional_mark/__init__.py index e4652f6a58..8be43c466e 100644 --- a/tests/common/plugins/conditional_mark/__init__.py +++ b/tests/common/plugins/conditional_mark/__init__.py @@ -415,7 +415,7 @@ def find_all_matches(nodeid, conditions): all_matches = [] max_length = -1 conditional_marks = {} - ret = [] + matches = [] for condition in conditions: # condition is a dict which has only one item, so we use condition.keys()[0] to get its key. @@ -434,11 +434,13 @@ def find_all_matches(nodeid, conditions): 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 ret: - ret.append(match) + if match not in matches: + matches.append(match) - return ret + return matches def update_issue_status(condition_str, session): From 90b2490343605a141ba282f19a0d310dafea513e Mon Sep 17 00:00:00 2001 From: Yutong Zhang Date: Wed, 4 Sep 2024 13:48:34 +0800 Subject: [PATCH 4/6] Optimise README --- .../common/plugins/conditional_mark/README.md | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/tests/common/plugins/conditional_mark/README.md b/tests/common/plugins/conditional_mark/README.md index 9f2519e648..5154e6476f 100644 --- a/tests/common/plugins/conditional_mark/README.md +++ b/tests/common/plugins/conditional_mark/README.md @@ -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`. @@ -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. @@ -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 ``` @@ -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. From 575f70b3a6367f24bf1a230dacbdef63ead3931c Mon Sep 17 00:00:00 2001 From: Yutong Zhang Date: Wed, 4 Sep 2024 15:41:17 +0800 Subject: [PATCH 5/6] Fix as comment --- .../plugins/conditional_mark/__init__.py | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/tests/common/plugins/conditional_mark/__init__.py b/tests/common/plugins/conditional_mark/__init__.py index 8be43c466e..f92dc5bfd4 100644 --- a/tests/common/plugins/conditional_mark/__init__.py +++ b/tests/common/plugins/conditional_mark/__init__.py @@ -423,22 +423,30 @@ def find_all_matches(nodeid, conditions): all_matches.append(condition) for match in all_matches: - test_case = list(match.keys())[0] - length = len(test_case) - marks = match[test_case].keys() + case_starting_substring = list(match.keys())[0] + length = len(case_starting_substring) + marks = match[case_starting_substring].keys() for mark in marks: - if conditional_marks.get(mark): + if mark in conditional_marks: if length > max_length: - conditional_marks.update({mark: match}) + conditional_marks.update({ + mark: { + case_starting_substring: { + mark: match[case_starting_substring][mark]} + }}) max_length = length else: - conditional_marks.update({mark: match}) + conditional_marks.update({ + mark: { + case_starting_substring: { + mark: match[case_starting_substring][mark]} + }}) # 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) + for condition in list(conditional_marks.values()): + if condition not in matches: + matches.append(condition) return matches From 42e602cf3e596f0cb377b0868096f23fc375f392 Mon Sep 17 00:00:00 2001 From: Yutong Zhang Date: Thu, 5 Sep 2024 10:25:48 +0800 Subject: [PATCH 6/6] Optimize readme --- tests/common/plugins/conditional_mark/README.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/common/plugins/conditional_mark/README.md b/tests/common/plugins/conditional_mark/README.md index 5154e6476f..8582bed78e 100644 --- a/tests/common/plugins/conditional_mark/README.md +++ b/tests/common/plugins/conditional_mark/README.md @@ -12,9 +12,12 @@ 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 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). +In `pytest_collection_modifyitems`, each collected test item (test case) is examined. +For each item, all potential matching conditions found based on the test case name are identified. +If a match is found and its mark is unique across all matches, the corresponding mark will be added to the test case. +If there are multiple matches, the mark from the longest match is used. +Different marks across multiple files are allowed. + ## 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`. @@ -77,8 +80,10 @@ folder3: ## Match rule -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. +This plugin process each expanded (for parametrized test cases) test cases one by one. +For each test case, it will get all potential matches that match the pattern of test case name. +And then, for each match, if the mark in it is unique across 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.