Revise the Windows skip logic of UT #2383
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR revises the Windows skip logic for unit tests by improving how skip lists are handled and fixing file patterns. The changes ensure empty lists are properly handled as None values and correct the file skip pattern format.
Key Changes:
- Updated skip list validation to check for both truthiness and non-empty lists
- Fixed the skip pattern from ".py::" to ".py" for entire file skipping
- Added handling for None and tuple values in skip dictionary merging
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/xpu/xpu_test_utils.py | Added length checks to skip_list and exe_list validation to ensure empty lists are treated properly |
| test/xpu/windows_skip_cases.py | Corrected dictionary key from "test_decomp" to "test_decomp.py" and updated skip pattern format |
| test/xpu/run_test_with_skip.py | Fixed import statement, updated file skip pattern detection, added handling for None/tuple values in skip dict, and added empty list normalization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if skip_dict[key] is None: | ||
| merged_skip_dict[key] = [] | ||
| elif isinstance(skip_dict[key], tuple): | ||
| merged_skip_dict[key] = list(skip_dict[key]) | ||
| else: | ||
| merged_skip_dict[key] = skip_dict[key].copy() if skip_dict[key] else [] |
There was a problem hiding this comment.
[nitpick] The nested if-elif-else logic can be simplified. When skip_dict[key] is None or falsy, all branches result in an empty list. Consider consolidating: merged_skip_dict[key] = list(skip_dict[key]) if isinstance(skip_dict[key], tuple) else (skip_dict[key].copy() if skip_dict[key] else [])
| if skip_dict[key] is None: | |
| merged_skip_dict[key] = [] | |
| elif isinstance(skip_dict[key], tuple): | |
| merged_skip_dict[key] = list(skip_dict[key]) | |
| else: | |
| merged_skip_dict[key] = skip_dict[key].copy() if skip_dict[key] else [] | |
| merged_skip_dict[key] = list(skip_dict[key]) if isinstance(skip_dict[key], tuple) else (skip_dict[key].copy() if skip_dict[key] else []) |
| def launch_test(test_case, skip_list=None, exe_list=None): | ||
| os.environ["PYTORCH_TEST_WITH_SLOW"] = "1" | ||
| if skip_list is not None: | ||
| if skip_list and len(skip_list) > 0: |
There was a problem hiding this comment.
The check skip_list and len(skip_list) > 0 is redundant. If skip_list is truthy and not empty, if skip_list: is sufficient since empty lists are falsy in Python. The len(skip_list) > 0 check adds no additional value.
| ) | ||
| test_command += skip_options | ||
| elif exe_list is not None: | ||
| elif exe_list and len(exe_list) > 0: |
There was a problem hiding this comment.
The check exe_list and len(exe_list) > 0 is redundant. If exe_list is truthy and not empty, if exe_list: is sufficient since empty lists are falsy in Python. The len(exe_list) > 0 check adds no additional value.
| elif exe_list and len(exe_list) > 0: | |
| elif exe_list: |
| # For "selected" case, use the skip_list as is | ||
|
|
||
| # If skip_list is empty, set it to None | ||
| if skip_list is not None and len(skip_list) == 0: |
There was a problem hiding this comment.
[nitpick] This can be simplified to if skip_list is not None and not skip_list: since empty lists are falsy in Python, making the length check unnecessary.
| if skip_list is not None and len(skip_list) == 0: | |
| if skip_list is not None and not skip_list: |
disable_e2e
disable_distribute