fix TestClass name, make module level variable all capital and other code cleanup
#702
Conversation
WalkthroughThe recent changes involve modifications to the Changes
Poem
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 (
|
DanielYang59
changed the title
ruff bebavior in CI and pre-commitpre-commit behaviour locally and from bot
DanielYang59
changed the title
pre-commit behaviour locally and from botpre-commit behaviour locally and in CI
| @@ -8,8 +8,8 @@ def test_command(): | |||
| sleep05 = Command("sleep 0.5") | |||
|
|
|||
| sleep05.run(timeout=1) | |||
| print(sleep05) | |||
| assert sleep05.retcode == 0 | |||
There was a problem hiding this comment.
This unit test seems to fail intermittently (seemingly in Windows only), I guess that's probably the reason the print exists.
First time failure: https://github.com/materialsvirtuallab/monty/actions/runs/10104741275/job/27944181978
Second time: https://github.com/materialsvirtuallab/monty/actions/runs/10104606892/job/27943837091
But still need to recreate this issue in the first place
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #702 +/- ##
==========================================
+ Coverage 82.57% 82.80% +0.22%
==========================================
Files 27 27
Lines 1584 1576 -8
Branches 284 284
==========================================
- Hits 1308 1305 -3
+ Misses 215 210 -5
Partials 61 61 ☔ View full report in Codecov by Sentry. |
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- .github/workflows/lint.yml (2 hunks)
- .pre-commit-config.yaml (3 hunks)
- pyproject.toml (1 hunks)
- src/monty/io.py (1 hunks)
- src/monty/json.py (1 hunks)
- src/monty/serialization.py (1 hunks)
- src/monty/subprocess.py (2 hunks)
- src/monty/termcolor.py (2 hunks)
- tests/test_design_patterns.py (2 hunks)
- tests/test_dev.py (3 hunks)
- tests/test_json.py (5 hunks)
- tests/test_pprint.py (1 hunks)
- tests/test_serialization.py (1 hunks)
- tests/test_subprocess.py (1 hunks)
Files skipped from review due to trivial changes (8)
- .pre-commit-config.yaml
- src/monty/io.py
- src/monty/json.py
- src/monty/serialization.py
- src/monty/subprocess.py
- tests/test_design_patterns.py
- tests/test_dev.py
- tests/test_serialization.py
Additional comments not posted (10)
tests/test_subprocess.py (1)
12-13: Enhance debugging with detailed assertion messages.The updated assertion provides more context in case of failure, which is especially useful for intermittent issues.
.github/workflows/lint.yml (1)
12-12: Upgradesetup-pythonaction to v5.The upgrade to
setup-python@v5ensures the latest features and improvements are utilized.tests/test_pprint.py (1)
37-37: Improve test robustness with assertions.Replacing the print statement with an assertion ensures that the output of
draw_tree(root)is validated, enhancing the test's robustness.pyproject.toml (2)
42-42: Addition ofipythondependency is approved.The inclusion of
ipythonenhances the interactive computing capabilities, which can be beneficial for debugging and testing.
43-43: Addition ofpydanticdependency is approved.The inclusion of
pydanticenhances data validation and settings management, which is beneficial for testing and development.src/monty/termcolor.py (2)
160-160: Verify the usage of theflushargument.The removal of the error handling for
TypeErrorsimplifies the function but may lead to unhandled exceptions if theflushargument is passed in unsupported environments.Verification successful
Verification successful. No issues found.
The
flushargument is not used in the codebase, so the removal of the error handling forTypeErrorin thecprint_mapfunction will not cause any unhandled exceptions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `flush` argument in the codebase. # Test: Search for the usage of the `cprint_map` function with the `flush` argument. rg --type python -A 5 $'cprint_map'Length of output: 65
Script:
#!/bin/bash # Description: Verify the usage of the `flush` argument in the codebase. # Test: Search for the usage of the `cprint_map` function. rg -A 5 'cprint_map' # Test: Search for the usage of the `flush` argument. rg -A 5 'flush'Length of output: 1272
131-131: Verify the usage of theflushargument.The removal of the error handling for
TypeErrorsimplifies the function but may lead to unhandled exceptions if theflushargument is passed in unsupported environments.Verification successful
Verify the usage of the
flushargument.The removal of the error handling for
TypeErrorsimplifies the function but may lead to unhandled exceptions if theflushargument is passed in unsupported environments. Currently, theflushargument is not used in the codebase, but it is recommended to verify its usage if introduced in the future.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `flush` argument in the codebase. # Test: Search for the usage of the `cprint` function with the `flush` argument. rg --type python -A 5 $'cprint'Length of output: 61
Script:
#!/bin/bash # Description: Verify the usage of the `flush` argument in the codebase. # Test: Search for the usage of the `cprint` function with the `flush` argument. rg -t py -A 5 $'cprint'Length of output: 4682
tests/test_json.py (3)
11-19: Import reorganization is approved.The reorganization of import statements improves readability and maintainability.
21-21: Variable renaming is approved.The renaming of
tests_versiontoTESTS_VERSIONenhances consistency in naming conventions across the test suite.
323-323: Adjustment in assertion is approved.The modification to check that the output is of type
dictenhances the robustness of the test.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
tests/test_files/3000_lines_gz.txt.gzis excluded by!**/*.gz
Files selected for processing (2)
- tests/test_io.py (1 hunks)
- tests/test_tempfile.py (2 hunks)
Files skipped from review due to trivial changes (2)
- tests/test_io.py
- tests/test_tempfile.py
This reverts commit b78e6da.
|
@shyuep Please review this, thanks! Meanwhile I cannot reliably recreate (I even wrote a script to run it 10000 times in my local windows machine, but no failure) the random issue for |
|
@shyuep Can you please review this? Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
pyproject.toml (1)
42-43: Re-evaluate the inclusion ofipythonin CI dependenciesThe usage of
pydanticin both test and source files justifies its inclusion in the CI dependencies. However,ipythondoes not appear to be actively used in the codebase, suggesting it may not be necessary. Consider whetheripythonis required for interactive testing or other purposes not evident in the current codebase.
pydantic: Used intests/test_json.pyandsrc/monty/json.pyipython: Only mentioned in a comment insrc/monty/collections.pyAnalysis chain
Addition of
ipythonandpydanticto CI dependencies is approved.The inclusion of
ipythonandpydanticin thecidependencies seems justified based on the PR summary, which mentions their necessity for specific tests. Ensure these dependencies are indeed required and do not introduce conflicts with existing packages.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the necessity of `ipython` and `pydantic` in the CI environment. # Test: Search for the usage of `ipython` and `pydantic` in the codebase. Expect: Usage in test files or CI scripts. rg --type py 'ipython|pydantic'Length of output: 1713
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- .pre-commit-config.yaml (3 hunks)
- tasks.py (1 hunks)
- tests/test_collections.py (1 hunks)
- tests/test_design_patterns.py (3 hunks)
- tests/test_dev.py (4 hunks)
- tests/test_fractions.py (1 hunks)
- tests/test_functools.py (1 hunks)
- tests/test_io.py (1 hunks)
- tests/test_json.py (5 hunks)
- tests/test_os.py (1 hunks)
- tests/test_serialization.py (1 hunks)
- tests/test_shutil.py (1 hunks)
- tests/test_tempfile.py (1 hunks)
Files skipped from review due to trivial changes (11)
- tasks.py
- tests/test_collections.py
- tests/test_design_patterns.py
- tests/test_dev.py
- tests/test_fractions.py
- tests/test_functools.py
- tests/test_io.py
- tests/test_os.py
- tests/test_serialization.py
- tests/test_shutil.py
- tests/test_tempfile.py
Additional comments not posted (7)
.pre-commit-config.yaml (3)
11-11: Version update approved forruff-pre-commit.The version update to
v0.6.1is approved. Ensure compatibility with your project's requirements.
27-27: Version update approved formirrors-mypy.The version update to
v1.11.1is approved. Ensure compatibility with your project's requirements.
1-1: Verify the new exclusion pattern.The exclusion pattern has been updated. Ensure that the directory structure for test files has been modified accordingly.
Run the following script to verify the existence of the
tests/test_filesdirectory:Verification successful
Verification successful: Updated exclusion pattern is valid.
The
tests/test_filesdirectory exists, confirming that the exclusion pattern update in.pre-commit-config.yamlaligns with the current directory structure. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the `tests/test_files` directory. # Test: Check if the directory exists. Expect: Directory should exist. fd --type d --exact-depth 1 --glob 'tests/test_files'Length of output: 423
Script:
#!/bin/bash # Description: Verify the existence of the `tests/test_files` directory using the correct `fd` command. # Test: Check if the directory exists. Expect: Directory should exist. fd --type d --full-path 'tests/test_files'Length of output: 60
tests/test_json.py (4)
11-12: Import reorganization approved.The
pytestimport has been moved to a more logical position, improving code organization.
22-22: Variable renaming approved.Renaming
tests_versiontoTESTS_VERSIONenhances readability and maintains consistency.
686-686: Assertion modification approved.Replacing a print statement with an assertion enhances test validation.
877-877: Skip condition modification approved.The change to "numpy or bson not present" ensures tests are skipped if either dependency is missing.
|
@shyuep Can you review this please? |
|
mark this as draft, dependencies tweaking should migrate to #714 |
DanielYang59
changed the title
pre-commit behaviour locally and in CI
DanielYang59
changed the title
TestClass name, make module level variable all capital and other code cleanup
DanielYang59
force-pushed
the
consistent-ruff
branch
from
a4358d9
to
bfe9431
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/test_shutil.py (1)
Line range hint
23-219: Consider organizing tests by functionality.While the current organization is functional, consider grouping related tests more tightly:
- File operations (copy, remove)
- Compression operations (gzip, compress)
- Symlink operations
This would improve maintainability and make it easier to add new tests in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/test_shutil.py(8 hunks)tests/test_subprocess.py(1 hunks)
🔇 Additional comments (9)
tests/test_subprocess.py (1)
11-11: LGTM: Good practice to document known issues.
The comment clearly documents the intermittent Windows CI issue and references PR702 for tracking.
tests/test_shutil.py (8)
23-23: LGTM! Good naming convention update.
The change from test_dir to TEST_DIR follows Python's PEP 8 naming convention for module-level constants.
28-37: LGTM! Consistent path handling.
The path construction using os.path.join with TEST_DIR is correct and platform-independent. The symlink creation is properly guarded for non-Windows systems.
41-52: LGTM! Well-structured test flow.
The test method has a clear progression of operations with appropriate assertions at each step. Path handling is consistent and correct.
57-57: LGTM! Good Path object testing.
Testing with pathlib.Path objects alongside string paths ensures compatibility with both interfaces.
69-69: LGTM! Proper test setup and cleanup.
Path construction is consistent, and the test methods maintain proper test isolation.
Also applies to: 73-73, 96-97
125-126: LGTM! Comprehensive test coverage.
The test cases properly cover various scenarios including coexisting files, subdirectories, and metadata preservation.
Also applies to: 129-129, 132-133, 145-145, 167-167
188-188: LGTM! Proper platform-specific handling.
The test methods are correctly skipped on Windows and include proper cleanup of temporary files.
Also applies to: 196-196
202-202: LGTM! Thorough symlink testing.
The symlink tests properly cover both following and non-following behaviors. Consider verifying symlink behavior across different file systems.
Also applies to: 205-206, 215-215, 218-219
Summary
TestClasstoClassForTestto avoidPytestCollectionWarningTag added for future PR:
test_commandunit test seem to fail randomly in windows, https://github.com/materialsvirtuallab/monty/pull/702/files#r1692442685, fixTestClassname, make module level variable all capital and other code cleanup #702 (comment)Summary by CodeRabbit
test_pint_quantityandtest_jsanitizeto cover additional scenarios.test_dirtoTEST_DIRin file operations for consistency.test_commandwith detailed debug messages for failed assertions.