Skip to content

Fix test helper discovery in pytest and sanitizer runner #446

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

Open
lucifer1004 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix test helper discovery in pytest and sanitizer runner #446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4068492
fix(testing): respect test discovery opt-out markers
lucifer1004 Sep 14, 2026
5b1f8e5
test: keep discovery fix focused on pytest convention
lucifer1004 Sep 14, 2026
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
3 changes: 3 additions & 0 deletions deep_gemm/testing/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ def wrapper(*args, **kwargs):
return decorator


test_filter.__test__ = False

@ds-review-bot ds-review-bot Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔵 suggestion: Consider adding a one-line comment explaining why test_filter.__test__ = False is set (pytest / test_sanitizer.py opt-out marker for a test-prefixed helper). Future readers may otherwise be tempted to remove what looks like a stray attribute assignment.

🤖 v5



def ignore_env(name: str, condition: Callable):
def decorator(func):
@functools.wraps(func)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_sanitizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
(module_name, name)
for module_name in [os.path.splitext(f)[0] for f in files if f not in exclude_files]
for name, obj in inspect.getmembers(importlib.import_module(module_name))
if inspect.isfunction(obj) and name.startswith('test') and 'test_filter' not in name
if inspect.isfunction(obj) and name.startswith('test') and getattr(obj, '__test__', True)

@ds-review-bot ds-review-bot Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔵 suggestion: With default discovery, the new tests/test_test_discovery.py module (test_pytest_discovery, test_sanitizer_discovery) will now itself be picked up and run under compute-sanitizer for every tool. These are pure-Python CPU tests that spawn pytest subprocesses and mock subprocess.run, so running them under the sanitizer is wasted time (and test_sanitizer_discovery re-executes this runner via runpy inside the sanitized process). Setting __test__ = False on them is not appropriate since pytest should collect them, so adding 'test_test_discovery.py' to exclude_files is the right lever. Not blocking.

🤖 v5

@lucifer1004 lucifer1004 Sep 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The whole test has been removed.

]
tools = [x.strip() for x in args.tools.split(',')]

Expand Down