Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix dev.deprecated and refine its unit tests #647

Merged
merged 15 commits into from
Apr 11, 2024
Merged

Fix dev.deprecated and refine its unit tests #647

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
385b5b9
replace `raise` for warning with `warn`
DanielYang59 Mar 30, 2024
49d51de
adjust warn position
DanielYang59 Mar 31, 2024
12a646d
pre-commit auto-fixes
pre-commit-ci[bot] Mar 31, 2024
1998adb
fix datetime.now patch
DanielYang59 Mar 31, 2024
8819902
Merge branch 'fix-deprecation' of https://github.com/DanielYang59/mon…
DanielYang59 Mar 31, 2024
07d52f3
revert to `raise` for `DeprecationWarning`
DanielYang59 Mar 31, 2024
890ed14
temp fix for "should raise" case
DanielYang59 Mar 31, 2024
4ea8d1b
add `ipython` to optional requirements
DanielYang59 Mar 31, 2024
5da8026
remove accidentially commited files
DanielYang59 Mar 31, 2024
d69055a
add test file back
DanielYang59 Mar 31, 2024
3fa8f13
Merge branch 'master' into fix-deprecation
DanielYang59 Apr 1, 2024
bcc5001
mock GITHUB_REPOSITORY
DanielYang59 Apr 1, 2024
fadaf14
refine unit test for deadline
DanielYang59 Apr 1, 2024
5e58909
remove `unittest.main()`
DanielYang59 Apr 1, 2024
1483615
Merge branch 'master' into fix-deprecation
shyuep Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions monty/dev.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,13 @@ def _is_in_owner_repo() -> bool:
# Only raise warning in code owner's repo CI
if (
_deadline is not None
and os.getenv("CI")
and os.getenv("CI") is not None
and datetime.now() > _deadline
and _is_in_owner_repo()
):
raise DeprecationWarning(
"This function should have been removed on {deadline:%Y-%m-%d}."
warnings.warn(
f"This function should have been removed on {_deadline:%Y-%m-%d}.",
DeprecationWarning,
)

def craft_message(
Expand All @@ -89,7 +90,7 @@ def craft_message(
msg = f"{old.__name__} is deprecated"

if deadline is not None:
msg += f", and will be removed on {deadline:%Y-%m-%d}\n"
msg += f", and will be removed on {_deadline:%Y-%m-%d}\n"
DanielYang59 marked this conversation as resolved.
Show resolved Hide resolved

if replacement is not None:
if isinstance(replacement, property):
Expand All @@ -115,7 +116,7 @@ def wrapped(*args, **kwargs):
# Convert deadline to datetime type
_deadline = datetime(*deadline) if deadline is not None else None

# Raise a CI warning after removal deadline
# Raise CI warning after removal deadline
raise_deadline_warning()

return deprecated_decorator
Expand Down
49 changes: 25 additions & 24 deletions tests/test_dev.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import unittest
import warnings
import datetime

import pytest
from monty.dev import deprecated, install_excepthook, requires
Expand Down Expand Up @@ -89,39 +88,38 @@ def classmethod_b(cls):
assert TestClass_deprecationwarning().classmethod_b() == "b"

def test_deprecated_deadline(self):
@deprecated(deadline=(2000, 1, 1))
def func_old():
pass

with warnings.catch_warnings(record=True) as warn_msgs:
# Trigger a warning.
func_old()
# Verify message
assert "will be removed on 2000-01-01" in str(warn_msgs[0].message)

@deprecated(deadline=(2000, 1, 1))
def func_old():
pass

assert "This function should have been removed" in str(warn_msgs[0].message)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that should be easy, no? monkeypatch.setenv("GITHUB_REPOSITORY", "materialsvirtuallab/monty")

Copy link
Contributor Author

@DanielYang59 DanielYang59 Mar 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_repo to always return True (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 CI variable as well, such that unit test wouldn't fail locally.

Copy link
Contributor Author

@DanielYang59 DanielYang59 Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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
# # No warn case 1: date before deadline
# DEBUG
# with warnings.catch_warnings(record=True) as warn_msgs:
# monkeypatch.setattr(datetime.datetime, "now", datetime.datetime(1999, 1, 1))

# No warn case 1: date before deadline
with warnings.catch_warnings(record=True) as warn_msgs:
monkeypatch.setattr(
datetime, "datetime", lambda: datetime.datetime(1999, 1, 1)
)
func_old()
# @deprecated(deadline=(2000, 1, 1))
# def func_old():
# pass

for warning in warn_msgs:
assert "This function should have been removed on" not in str(
warning.message
)
# for warning in warn_msgs:
# assert "This function should have been removed on" not in str(
# warning.message
# )

# No warn case 2: not in CI env
with warnings.catch_warnings(record=True) as warn_msgs:
monkeypatch.delenv("CI", raising=False)
func_old()

@deprecated(deadline=(2000, 1, 1))
def func_old():
pass

for warning in warn_msgs:
assert "This function should have been removed on" not in str(
Expand All @@ -131,7 +129,10 @@ def func_old():
# No warn case 3: not in code owner repo
with warnings.catch_warnings(record=True) as warn_msgs:
monkeypatch.setenv("GITHUB_REPOSITORY", "NONE/NONE")
func_old()

@deprecated(deadline=(2000, 1, 1))
def func_old():
pass

for warning in warn_msgs:
assert "This function should have been removed on" not in str(
Expand Down
8 changes: 2 additions & 6 deletions tests/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ def test_reverse_readline(self):
for idx, line in enumerate(reverse_readline(f)):
assert (
int(line) == self.NUMLINES - idx
), "read_backwards read {} whereas it should "(
"have read {" "}"
).format(int(line), self.NUMLINES - idx)
), f"read_backwards read {int(line)} whereas it should have read {self.NUMLINES - idx}"

def test_reverse_readline_fake_big(self):
"""
Expand All @@ -44,9 +42,7 @@ def test_reverse_readline_fake_big(self):
for idx, line in enumerate(reverse_readline(f, max_mem=0)):
assert (
int(line) == self.NUMLINES - idx
), "read_backwards read {} whereas it should "(
"have read {" "}"
).format(int(line), self.NUMLINES - idx)
), f"read_backwards read {int(line)} whereas it should have read {self.NUMLINES - idx}"

def test_reverse_readline_bz2(self):
"""
Expand Down
Loading