Minor test cleanup: skipif tests that require optional dependencies, migrate unittest skipif to pytest, minor import/typing clean up
#742
Conversation
WalkthroughThe pull request introduces several modifications across multiple files in the monty library, primarily focusing on import statements, type checking, and test framework updates. The changes include conditional imports using Changes
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #742 +/- ##
=======================================
Coverage 84.75% 84.75%
=======================================
Files 27 27
Lines 1653 1653
Branches 315 315
=======================================
Hits 1401 1401
Misses 192 192
Partials 60 60 ☔ View full report in Codecov by Sentry. |
DanielYang59
force-pushed
the
fix-some-tests
branch
2 times, most recently
from
445dfcc to
4d6165b
Compare
DanielYang59
changed the title
unittest skipif to pytest, minor import/typing clean up
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_dev.py (1)
11-14: Fix unused exception variable.The exception variable
excis assigned but never used.-except ImportError as exc: +except ImportError:🧰 Tools
🪛 Ruff (0.8.2)
13-13: Local variable
excis assigned to but never usedRemove assignment to unused variable
exc(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/test_files/test_settings.yaml.gzis excluded by!**/*.gz
📒 Files selected for processing (13)
src/monty/multiprocessing.py(1 hunks)src/monty/string.py(1 hunks)tests/test_design_patterns.py(1 hunks)tests/test_dev.py(2 hunks)tests/test_functools.py(1 hunks)tests/test_itertools.py(0 hunks)tests/test_json.py(3 hunks)tests/test_multiprocessing.py(1 hunks)tests/test_os.py(1 hunks)tests/test_serialization.py(1 hunks)tests/test_shutil.py(2 hunks)tests/test_string.py(1 hunks)tests/test_termcolor.py(0 hunks)
💤 Files with no reviewable changes (2)
- tests/test_termcolor.py
- tests/test_itertools.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_string.py
- tests/test_design_patterns.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_dev.py
13-13: Local variable exc is assigned to but never used
Remove assignment to unused variable exc
(F841)
🔇 Additional comments (11)
tests/test_multiprocessing.py (2)
5-5: LGTM! Clean implementation of optional dependency handling.The try-except block for tqdm import and the conditional test skip implementation align well with the PR's objective of handling optional dependencies.
Also applies to: 7-12
15-15: LGTM! Proper migration to pytest skip marker.The migration from unittest to pytest skip marker is correctly implemented, maintaining the same functionality while standardizing the testing approach.
src/monty/multiprocessing.py (1)
8-11: LGTM! Proper implementation of conditional type imports.Moving type hints under TYPE_CHECKING condition follows best practices and improves runtime performance by avoiding unnecessary imports during execution.
tests/test_serialization.py (1)
68-68: LGTM! Consistent migration to pytest skip marker.The migration from unittest.skipIf to pytest.mark.skipif maintains the same functionality while aligning with the project's standardization on pytest.
tests/test_os.py (1)
4-4: LGTM! Proper implementation of conditional type imports.Moving the Path import under TYPE_CHECKING condition follows best practices and is consistent with the type hint optimization pattern applied across the codebase.
Also applies to: 11-12
src/monty/string.py (1)
Line range hint
7-14: LGTM! Good optimization of import statements.Moving
Iterableto theTYPE_CHECKINGblock is a good practice as it reduces runtime overhead while maintaining type checking capabilities during development.tests/test_dev.py (1)
207-209: LGTM! Good handling of optional dependency.The test is properly skipped when IPython is not installed, using the correct pytest marker.
tests/test_shutil.py (1)
191-191: LGTM! Good migration to pytest skip markers.The migration from
unittest.skipIftopytest.mark.skipifis consistent across all test methods, maintaining the same skip conditions and messages.Also applies to: 199-199, 205-205, 218-218
tests/test_functools.py (1)
622-622: LGTM! Good migration to pytest skip marker.The migration from
unittest.skipIftopytest.mark.skipifmaintains the same skip condition and message, consistent with changes in other test files.tests/test_json.py (2)
51-55: LGTM! Clean implementation of optional dependency import.The import follows the established pattern in the file for handling optional dependencies.
Line range hint
1264-1285: LGTM! Well-structured test for extended JSON serialization.The test comprehensively covers datetime, NaN, and infinity serialization, with proper type checking and value comparison. Good use of
pytest.approxfor floating-point comparison of datetime timestamps.
DanielYang59
force-pushed
the
fix-some-tests
branch
from
4d6165b to
07c92dd
Compare
DanielYang59
changed the title
unittest skipif to pytest, minor import/typing clean upunittest skipif to pytest, minor import/typing clean up
tests/test_files/test_settings.yamltosettings_for_test.yamlto avoid undesiredpytestpickup, to fix [Bug]: Tests fail to run: TypeError: Test file has to be YAML list, got <class 'dict'> #731tests/test_itertools.py(as other test scripts doesn't seem to carry this header)unittestflavored skip mark topytest