-
Notifications
You must be signed in to change notification settings - Fork 72
Fix test names for pytest #2358
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR prevents pytest from discovering helper functions (test_case_1
, test_case_2
, etc.) as standalone test cases by switching from direct imports to module imports and updating all references accordingly.
- Replaced direct imports of test-case constructors with module-level imports for
_rotary_embedding_models
and_smollm_1
- Updated all usages to qualify test constructors with their module to hide them from pytest
Comments suppressed due to low confidence (2)
onnxscript/rewriter/ort_fusions/cos_sin_cache_test.py:54
- [nitpick] Rename the variable
test
to something more descriptive (e.g.,test_case
orcase
) to avoid shadowing built-ins and clarify its purpose.
test = _rotary_embedding_models.partial_rotary_test_case()
onnxscript/rewriter/ort_fusions/cos_sin_cache_test.py:12
- [nitpick] Consider aliasing these module imports to shorter names (e.g.,
_rotary_embedding_models as rem_models
,_smollm_1 as smollm_models
) to reduce verbosity and improve readability in the test code.
from onnxscript.rewriter.ort_fusions.models import _rotary_embedding_models, _smollm_1
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a bad naming. Maybe we should consider rename it.
Agreed |
With the latest version of pytest we get `onnxscript/rewriter/ort_fusions/cos_sin_cache_test.py::test_case_1 - Failed: Expected None, but test returned <onnxscript.rewriter.ort_fusions.models._rotary_embedding_models._TestCase1 object at 0x117c920d0>. Did you mean to use `assert` instead of `return`?` This is because the imported functions `test_case_1` and `test_case_2` are not really test cases but were treated as such by pytest. This PR hides them from the test module so they are not triggered.
With the latest version of pytest we get
This is because the imported functions
test_case_1
andtest_case_2
are not really test cases but were treated as such by pytest. This PR hides them from the test module so they are not triggered.