Fix dev.deprecated and refine its unit tests
#647
Conversation
explicitly check for None fix string format in `test_io` add missing f for format string fix date format fix datetime in string DEBUG: comment out no warn case 3 DEBUG: comment out no warn case 2 DEBUG: comment out no warn case try to fix warning fix time patch try to fix date patch revert to monkeypatch
WalkthroughThe recent modifications focus on enhancing the codebase by refining deadline handling and deprecation warnings, updating dependencies, and improving test coverage and efficiency. These changes streamline development practices, ensuring better maintenance and compliance with modern Python standards, such as the use of f-strings and the removal of unnecessary code and imports. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #647 +/- ##
==========================================
+ Coverage 81.12% 81.26% +0.14%
==========================================
Files 27 27
Lines 1409 1409
Branches 306 306
==========================================
+ Hits 1143 1145 +2
+ Misses 213 209 -4
- Partials 53 55 +2 ☔ View full report in Codecov by Sentry. |
|
My
However even with above two issues fixed, the unit test is still failing in my forked repo, where the first "no warn" case ("date before deadline") still triggers a warning, suggesting the time patch in unit test may be unsuccessful? The same warning is not triggered here because this is not "the code owner's repo". Also the unit tests pass locally. I don't quite understand this behavior now, can you please give me a hand on this? @janosh Thanks! |
are you sure about that? we want the remove-deprecation error to trigger as soon as the code is parsed by the python interpreter, not wait until the function is called, right?
i think you may have def test_deprecated_deadline_no_warn(self, monkeypatch):
"""Test cases where no warning should be raised."""
@deprecated(deadline=(2000, 1, 1))
def func_old():
pass
monkeypatch.setattr(datetime.datetime, "now", lambda: datetime.datetime(1999, 1, 1)) # changed
# No warn case 1: date before deadline
with warnings.catch_warnings(record=True) as warn_msgs:
func_old()
for warning in warn_msgs:
assert "This function should have been removed on" not in str(warning.message) |
|
Hi @janosh, thanks a lot for your quick response and sharing your suggestions.
Yes I think this makes sense, but in both cases, existing unit test was inconsistent and need to be adjusted (done in 49d51de): def test_deprecated_deadline(self):
@deprecated(deadline=(2000, 1, 1))
def func_old():
pass
with warnings.catch_warnings(record=True) as warn_msgs:
func_old() # trigger a warning
assert "will be removed on 2000-01-01" in str(warn_msgs[0].message)Above unit test assert there would be a warning when @deprecated(deadline=(2000, 1, 1))
def func_old():
pass
I have been struggling with patching time for a while and I didn't find much information by searching (some suggestion use monkeypatch.setattr(datetime.datetime, "now", datetime.datetime(1999, 1, 1))There would be an error suggesting One last thing I realized from this issue is that if we just issue a |
tests/test_dev.py
Outdated
| def func_old(): | ||
| pass | ||
|
|
||
| assert "This function should have been removed" in str(warn_msgs[0].message) |
There was a problem hiding this comment.
I would need to patch this as well. Because currently the code is being pushed from my fork, and as such it is not "in code owner" repo, therefore no warning would be issue. And this is passing in my fork, could confirm my assumption
There was a problem hiding this comment.
that should be easy, no? monkeypatch.setenv("GITHUB_REPOSITORY", "materialsvirtuallab/monty")
There was a problem hiding this comment.
monkeypatch.setenv("GITHUB_REPOSITORY", "materialsvirtuallab/monty")
This is a good fix, and I'm thinking if we could make it better:
- Somehow patch
_is_in_owner_repoto always returnTrue(I'm still experimenting on this) such that we don't need to hard code the repo name ("materialsvirtuallab/monty"). Even when we hard code the repo name, unit test would still fail in forks. - Set the
CIvariable as well, such that unit test wouldn't fail locally.
There was a problem hiding this comment.
I experimented on patching _is_in_owner_repo and did not have much luck, then I decide to patch subprocess instead in bcc5001 (which is closer to the real-world scenario too):
with patch("subprocess.run") as mock_run:
# Mock "GITHUB_REPOSITORY"
monkeypatch.setenv("GITHUB_REPOSITORY", "TESTOWNER/TESTREPO")
mock_run.return_value.stdout.decode.return_value = (
"git@github.com:TESTOWNER/TESTREPO.git"
)And everything is working as expected so far.
yes! i was actually going to suggest defining a custom error for this: class ExpiredDeprecationError(RuntimeError):maybe some other parent class instead of
interesting, didn't know that. this SO answer looks promising, have you tried it? |
I'm thinking about just
Thanks for providing this, I would have a look and let you know. |
… into fix-deprecation
Seems adding a |
|
@coderabbitai review |
DanielYang59
changed the title
dev.deprecated and unit test for iodev.deprecated and refine its unit tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- monty/dev.py (3 hunks)
- requirements-optional.txt (1 hunks)
- tests/test_dev.py (2 hunks)
- tests/test_tempfile.py (2 hunks)
- tests/test_termcolor.py (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/test_termcolor.py
Additional Context Used
GitHub Check Runs (1)
codecov/patch failure (1)
monty/dev.py: [warning] 94-94: monty/dev.py#L94
Added line #L94 was not covered by tests
Additional comments not posted (5)
requirements-optional.txt (1)
10-10: The addition ofipython==8.22.2is aligned with the PR objectives to address a specific warning during tests. Ensure compatibility with other dependencies in the project.tests/test_dev.py (2)
4-4: The addition ofpatchfromunittest.mockis appropriate for mocking in the modified test methods.
92-157: The modifications to the test methods, including additional mocking and environment setup, enhance the unit testing fordev.deprecated. Consider adding comments to explain complex mocking scenarios for future maintainability.monty/dev.py (2)
77-77: Ensuringos.getenv("CI")is notNoneis a good practice to confirm the code is running in a CI environment before raising a deprecation warning.
82-82: Using f-strings for the deprecation warning message enhances readability and maintainability.
|
Please review this as well @shyuep. Thanks! |
|
Thanks for reviewing! |
Summary:
dev.deprecateddecorator discovered by @janosh in FixMSONable.REDIRECTwhen module name changed #646dev.deprecatedto generate more consistent resultsipythontorequirements-optional.txt, previously getting:Summary by CodeRabbit
ipython==8.22.2to optional dependencies for enhanced interactive console features.