Repository: theupdateframework/python-tuf (language: python)

Issue:
PyPy3.10 test failures due to datetime mocking: `TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types, got MagicMock`
**Description of issue or feature request**:

The test suite currently fails 3 tests on PyPy3.10 7.3.17. This is due to a known limitation of pure Python `datetime` module from CPython. However, CPython normally covers it with a C extension, hiding the problem, but PyPy suffers from it.


**Current behavior**:

```pytb
$ tox -e pypy310
pypy310: commands[0]> python3 --version
Python 3.10.14 (39dc8d3c85a7, Aug 29 2024, 13:06:01)
[PyPy 7.3.17 with GCC 13.3.1 20240614]
pypy310: commands[1]> python3 -m coverage run aggregate_tests.py
.................................................................................................................................................EEE.....................................
======================================================================
ERROR: test_expired_metadata (test_updater_top_level_update.TestRefresh)
Verifies that expired local timestamp/snapshot can be used for
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1379, in patched
    return func(*newargs, **newkeywargs)
  File "/tmp/python-tuf/tests/test_updater_top_level_update.py", line 752, in test_expired_metadata
    now = datetime.datetime.now(timezone.utc)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1114, in __call__
    return self._mock_call(*args, **kwargs)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1118, in _mock_call
    return self._execute_mock_call(*args, **kwargs)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1188, in _execute_mock_call
    return self._mock_wraps(*args, **kwargs)
  File "/usr/lib/pypy3.10/datetime.py", line 1720, in now
    return cls.fromtimestamp(t, tz)
  File "/usr/lib/pypy3.10/datetime.py", line 1709, in fromtimestamp
    return cls._fromtimestamp(t, tz is not None, tz)
  File "/usr/lib/pypy3.10/datetime.py", line 1698, in _fromtimestamp
    result = tz.fromutc(result)
  File "/usr/lib/pypy3.10/datetime.py", line 2362, in fromutc
    if isinstance(dt, datetime):
TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types, got MagicMock

======================================================================
ERROR: test_expired_timestamp_snapshot_rollback (test_updater_top_level_update.TestRefresh)
Verifies that rollback protection is done even if local timestamp has expired.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1379, in patched
    return func(*newargs, **newkeywargs)
  File "/tmp/python-tuf/tests/test_updater_top_level_update.py", line 358, in test_expired_timestamp_snapshot_rollback
    now = datetime.datetime.now(timezone.utc)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1114, in __call__
    return self._mock_call(*args, **kwargs)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1118, in _mock_call
    return self._execute_mock_call(*args, **kwargs)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1188, in _execute_mock_call
    return self._mock_wraps(*args, **kwargs)
  File "/usr/lib/pypy3.10/datetime.py", line 1720, in now
    return cls.fromtimestamp(t, tz)
  File "/usr/lib/pypy3.10/datetime.py", line 1709, in fromtimestamp
    return cls._fromtimestamp(t, tz is not None, tz)
  File "/usr/lib/pypy3.10/datetime.py", line 1698, in _fromtimestamp
    result = tz.fromutc(result)
  File "/usr/lib/pypy3.10/datetime.py", line 2362, in fromutc
    if isinstance(dt, datetime):
TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types, got MagicMock

======================================================================
ERROR: test_expired_timestamp_version_rollback (test_updater_top_level_update.TestRefresh)
Verifies that local timestamp is used in rollback checks even if it is expired.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1379, in patched
    return func(*newargs, **newkeywargs)
  File "/tmp/python-tuf/tests/test_updater_top_level_update.py", line 322, in test_expired_timestamp_version_rollback
    now = datetime.datetime.now(timezone.utc)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1114, in __call__
    return self._mock_call(*args, **kwargs)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1118, in _mock_call
    return self._execute_mock_call(*args, **kwargs)
  File "/usr/lib/pypy3.10/unittest/mock.py", line 1188, in _execute_mock_call
    return self._mock_wraps(*args, **kwargs)
  File "/usr/lib/pypy3.10/datetime.py", line 1720, in now
    return cls.fromtimestamp(t, tz)
  File "/usr/lib/pypy3.10/datetime.py", line 1709, in fromtimestamp
    return cls._fromtimestamp(t, tz is not None, tz)
  File "/usr/lib/pypy3.10/datetime.py", line 1698, in _fromtimestamp
    result = tz.fromutc(result)
  File "/usr/lib/pypy3.10/datetime.py", line 2362, in fromutc
    if isinstance(dt, datetime):
TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types, got MagicMock

----------------------------------------------------------------------
Ran 185 tests in 6.709s

FAILED (errors=3)
pypy310: exit 1 (7.69 seconds) /tmp/python-tuf/tests> python3 -m coverage run aggregate_tests.py pid=490151
  pypy310: FAIL code 1 (7.78=setup[0.07]+cmd[0.02,7.69] seconds)
  evaluation failed :( (7.84 seconds)
```

**Expected behavior**:

Tests passing :-).

One possible solution would be to use a more complete time mocking solution, e.g. [freezegun](https://pypi.org/project/freezegun/). I've been able to quickly confirm that it solves the problem, using a quick patch:

```diff
 from tests.repository_simulator import RepositorySimulator
 from tuf.api.exceptions import (
@@ -306,8 +308,7 @@ def test_new_timestamp_unsigned(self) -> None:
 
         self._assert_files_exist([Root.type])
 
-    @patch.object(datetime, "datetime", wraps=datetime.datetime)
-    def test_expired_timestamp_version_rollback(self, mock_time: Mock) -> None:
+    def test_expired_timestamp_version_rollback(self) -> None:
         """Verifies that local timestamp is used in rollback checks even if it is expired.
 
         The timestamp updates and rollback checks are performed
@@ -331,14 +332,12 @@ def test_expired_timestamp_version_rollback(self, mock_time: Mock) -> None:
 
         self.sim.timestamp.version = 1
 
-        mock_time.now.return_value = datetime.datetime.now(
-            timezone.utc
-        ) + datetime.timedelta(days=18)
-        patcher = patch("datetime.datetime", mock_time)
-        # Check that a rollback protection is performed even if
-        # local timestamp has expired
-        with patcher, self.assertRaises(BadVersionNumberError):
-            self._run_refresh()
+        with freezegun.freeze_time(datetime.datetime.now( timezone.utc) +
+                                   datetime.timedelta(days=18)):
+            # Check that a rollback protection is performed even if
+            # local timestamp has expired
+            with self.assertRaises(BadVersionNumberError):
+                self._run_refresh()
 
         self._assert_version_equals(Timestamp.type, 2)
 
```

If you agree with this approach, I can make a proper pull request.


Discussion / hints:
Well that does seem annoying... I would really like to avoid new dependencies if possible. This looks like it would be manageable as just a test dependency, but still: `unittest` has an advantage of being part of the standard library.

Trying to come up with some alternatives:
* just skip these tests when running on pypy -- this feels acceptable since the expiry tests would still run on other Pythons. They are also included in tuf-conformance tests that run on CI
* do the test without mocking, like maybe run a separate exectuable under libfaketime? -- this sounds like a lot of complexity that is not worth it


Skipping seems preferable to me when compared to a new dependency.
What do you think of `@unittest.skipIf(platform.python_implementation() == "PyPy", 'skipping time mocking tests on pypy')` or something like it?
Skipping them would mean that there would be an uncovered code path on PyPy, wouldn't it? Then that path would be susceptible to actually hitting another problem on PyPy. And libfaketime sounds like a much heavier dependency than freezegun.

Perhaps the mocking could be modified not to cause the problem. After all, the problem stems from using `MagicMock`. I'm going to experiment with a bit and get back to you.
Sigh, what an annoying thing. PyPy works fine if we override `datetime.datetime.now` directly but CPython doesn't tolerate modifying it.
I'm afraid I don't have a good way of doing this, short of having conditional mocking code that detects whether we're using pure Python or C implementation, and uses a different code path appropriately.
Thanks for taking another look. I'll play with it this week a bit as well, we can decide then.
Yeah, I can't think of a better way to solve this than your original suggestion. freezegun also looks like a sensible module.

I'd be happy with a patch like you described, sorry it took a while to get here. 

I imagine adding freezegun to` requirements/test.txt` will suffice.


Fix patch:
diff --git a/requirements/lint.txt b/requirements/lint.txt
index 55640d1f78..b69387daa9 100644
--- a/requirements/lint.txt
+++ b/requirements/lint.txt
@@ -8,3 +8,6 @@
 # are pinned to prevent unexpected linting failures when tools update)
 ruff==0.6.7
 mypy==1.11.2
+
+# Required for type stubs
+freezegun==1.5.1


Test patch:
diff --git a/requirements/test.txt b/requirements/test.txt
index d5f61d17a5..f77d2abba1 100644
--- a/requirements/test.txt
+++ b/requirements/test.txt
@@ -5,3 +5,4 @@
 
 # coverage measurement
 coverage==7.6.1
+freezegun==1.5.1
diff --git a/tests/test_updater_top_level_update.py b/tests/test_updater_top_level_update.py
index 78c8d7764a..cd82b5ba90 100644
--- a/tests/test_updater_top_level_update.py
+++ b/tests/test_updater_top_level_update.py
@@ -11,7 +11,9 @@
 import unittest
 from datetime import timezone
 from typing import Iterable, Optional
-from unittest.mock import MagicMock, Mock, call, patch
+from unittest.mock import MagicMock, call, patch
+
+import freezegun
 
 from tests import utils
 from tests.repository_simulator import RepositorySimulator
@@ -306,8 +308,7 @@ def test_new_timestamp_unsigned(self) -> None:
 
         self._assert_files_exist([Root.type])
 
-    @patch.object(datetime, "datetime", wraps=datetime.datetime)
-    def test_expired_timestamp_version_rollback(self, mock_time: Mock) -> None:
+    def test_expired_timestamp_version_rollback(self) -> None:
         """Verifies that local timestamp is used in rollback checks even if it is expired.
 
         The timestamp updates and rollback checks are performed
@@ -331,10 +332,9 @@ def test_expired_timestamp_version_rollback(self, mock_time: Mock) -> None:
 
         self.sim.timestamp.version = 1
 
-        mock_time.now.return_value = datetime.datetime.now(
-            timezone.utc
-        ) + datetime.timedelta(days=18)
-        patcher = patch("datetime.datetime", mock_time)
+        patcher = freezegun.freeze_time(
+            datetime.datetime.now(timezone.utc) + datetime.timedelta(days=18)
+        )
         # Check that a rollback protection is performed even if
         # local timestamp has expired
         with patcher, self.assertRaises(BadVersionNumberError):
@@ -342,8 +342,7 @@ def test_expired_timestamp_version_rollback(self, mock_time: Mock) -> None:
 
         self._assert_version_equals(Timestamp.type, 2)
 
-    @patch.object(datetime, "datetime", wraps=datetime.datetime)
-    def test_expired_timestamp_snapshot_rollback(self, mock_time: Mock) -> None:
+    def test_expired_timestamp_snapshot_rollback(self) -> None:
         """Verifies that rollback protection is done even if local timestamp has expired.
 
         The snapshot updates and rollback protection checks are performed
@@ -370,10 +369,9 @@ def test_expired_timestamp_snapshot_rollback(self, mock_time: Mock) -> None:
         self.sim.update_snapshot()
         self.sim.timestamp.expires = now + datetime.timedelta(days=21)
 
-        mock_time.now.return_value = datetime.datetime.now(
-            timezone.utc
-        ) + datetime.timedelta(days=18)
-        patcher = patch("datetime.datetime", mock_time)
+        patcher = freezegun.freeze_time(
+            datetime.datetime.now(timezone.utc) + datetime.timedelta(days=18)
+        )
         # Assert that rollback protection is done even if
         # local timestamp has expired
         with patcher, self.assertRaises(BadVersionNumberError):
@@ -736,8 +734,7 @@ def test_load_metadata_from_cache(self, wrapped_open: MagicMock) -> None:
         expected_calls = [("root", 2), ("timestamp", None)]
         self.assertListEqual(self.sim.fetch_tracker.metadata, expected_calls)
 
-    @patch.object(datetime, "datetime", wraps=datetime.datetime)
-    def test_expired_metadata(self, mock_time: Mock) -> None:
+    def test_expired_metadata(self) -> None:
         """Verifies that expired local timestamp/snapshot can be used for
         updating from remote.
 
@@ -761,10 +758,9 @@ def test_expired_metadata(self, mock_time: Mock) -> None:
 
         # Mocking time so that local timestam has expired
         # but the new timestamp has not
-        mock_time.now.return_value = datetime.datetime.now(
-            timezone.utc
-        ) + datetime.timedelta(days=18)
-        with patch("datetime.datetime", mock_time):
+        with freezegun.freeze_time(
+            datetime.datetime.now(timezone.utc) + datetime.timedelta(days=18)
+        ):
             self._run_refresh()
 
         # Assert that the final version of timestamp/snapshot is version 2