Consolidate conftest.py into testing directory #847
Conversation
|
cc @400Ping |
There was a problem hiding this comment.
Pull request overview
This PR consolidates the root-level conftest.py into testing/conftest.py to maintain a cleaner project root structure. The functionality from both files is merged while maintaining the same test behavior.
Changes:
- Moved pytest configuration from root
conftest.pytotesting/conftest.py - Merged QDP availability checking, custom markers, and test fixtures into a single configuration file
- Changed QDP detection from
importlib.util.find_specto try/except import approach
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| conftest.py | Deleted root-level conftest.py to consolidate pytest configuration |
| testing/conftest.py | Merged root conftest.py functionality, added comprehensive docstrings and QDP-related test infrastructure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| import _qdp # noqa: F401 | ||
| import torch # noqa: F401 | ||
| except ImportError: | ||
| import _qdp # noqa: F401, PLC0415 | ||
|
|
||
| _QDP_AVAILABLE = True | ||
| _QDP_IMPORT_ERROR = None | ||
| except ImportError as e: | ||
| _QDP_IMPORT_ERROR = str(e) |
There was a problem hiding this comment.
The QDP availability check has changed from using importlib.util.find_spec to a try/except import. While both approaches work, the try/except approach will catch all import errors (including those from broken installations or dependency issues), whereas find_spec only checks if the module can be located. This behavioral change could make debugging more difficult if _qdp is found but fails to import due to dependency issues, as the error message will be more generic. Consider whether this change in behavior is intentional.
|
|
||
| # Check if QDP extension is available at module load time | ||
| _QDP_AVAILABLE = False | ||
| _QDP_IMPORT_ERROR: str | None = "No module named '_qdp'" |
There was a problem hiding this comment.
The initial assignment of _QDP_IMPORT_ERROR to "No module named '_qdp'" on line 33 is redundant, as this value will always be overwritten either to None (line 38) if import succeeds or to str(e) (line 40) if import fails. Consider removing this initial assignment for clarity.
| _QDP_IMPORT_ERROR: str | None = "No module named '_qdp'" | |
| _QDP_IMPORT_ERROR: str | None |
| import _qdp # noqa: F401 | ||
| import torch # noqa: F401 | ||
| except ImportError: | ||
| import _qdp # noqa: F401, PLC0415 |
There was a problem hiding this comment.
Import of '_qdp' is not used.
| import _qdp # noqa: F401, PLC0415 | |
| import _qdp # noqa: F401, PLC0415 | |
| del _qdp |
|
LG, thanks @guan404ming for the fix. |
|
Oh, I discovered why we introduce root level test conf. That is because we want to use command |
2 participants
Purpose of PR
Merge root conftest.py into testing/conftest.py to keep the root clean
Related Issues or PRs
Changes Made
Breaking Changes
Checklist