testing: Replace _TestMethodWrapper with _callTestMethod

Overriding _callTestMethod (which was introduced in python 3.8) is a
less hacky way to detect tests that fail to use ``@gen_test`` where
needed. It's not documented, but since Python 3.11 has introduced a
similar check to the standard library we'll be able to remove it in the
near future.

The major impetus for this change is an incompatibility with
Pytest 8.2, which has made a change that tries to instantiate test
classes at discovery time without an existing method name.

Fixes #3375
Closes #3374

diff --git a/tornado/test/testing_test.py b/tornado/test/testing_test.py
index 0429feee8308fc4d02c6bb209650a10c94d56c70..4432bb17584488ba240fddfbcdf91132124aab4b 100644 (file)
--- a/tornado/test/testing_test.py
+++ b/tornado/test/testing_test.py
@@ -5,7 +5,6 @@ from tornado.testing import AsyncHTTPTestCase, AsyncTestCase, bind_unused_port,
 from tornado.web import Application
 import asyncio
 import contextlib
-import inspect
 import gc
 import os
 import platform
@@ -118,7 +117,11 @@ class AsyncHTTPTestCaseTest(AsyncHTTPTestCase):
         super().tearDown()
 
 
-class AsyncTestCaseWrapperTest(unittest.TestCase):
+class AsyncTestCaseReturnAssertionsTest(unittest.TestCase):
+    # These tests verify that tests that return non-None values (without being decorated with
+    # @gen_test) raise errors instead of incorrectly succeeding. These tests should be removed or
+    # updated when the _callTestMethod method is removed from AsyncTestCase (the same checks will
+    # still happen, but they'll be performed in the stdlib as DeprecationWarnings)
     def test_undecorated_generator(self):
         class Test(AsyncTestCase):
             def test_gen(self):
@@ -135,7 +138,10 @@ class AsyncTestCaseWrapperTest(unittest.TestCase):
         "pypy destructor warnings cannot be silenced",
     )
     @unittest.skipIf(
-        sys.version_info >= (3, 12), "py312 has its own check for test case returns"
+        # This check actually exists in 3.11 but it changed in 3.12 in a way that breaks
+        # this test.
+        sys.version_info >= (3, 12),
+        "py312 has its own check for test case returns",
     )
     def test_undecorated_coroutine(self):
         class Test(AsyncTestCase):
@@ -176,17 +182,6 @@ class AsyncTestCaseWrapperTest(unittest.TestCase):
         self.assertEqual(len(result.errors), 1)
         self.assertIn("Return value from test method ignored", result.errors[0][1])
 
-    def test_unwrap(self):
-        class Test(AsyncTestCase):
-            def test_foo(self):
-                pass
-
-        test = Test("test_foo")
-        self.assertIs(
-            inspect.unwrap(test.test_foo),
-            test.test_foo.orig_method,  # type: ignore[attr-defined]
-        )
-
 
 class SetUpTearDownTest(unittest.TestCase):
     def test_set_up_tear_down(self):

diff --git a/tornado/testing.py b/tornado/testing.py
index bdbff87bc367593cee1c5756eef435eca36e5bbb..4c33b3e22f092798ff2b70eb0940c9e9168c1dc4 100644 (file)
--- a/tornado/testing.py
+++ b/tornado/testing.py
@@ -84,39 +84,6 @@ def get_async_test_timeout() -> float:
     return 5
 
 
-class _TestMethodWrapper(object):
-    """Wraps a test method to raise an error if it returns a value.
-
-    This is mainly used to detect undecorated generators (if a test
-    method yields it must use a decorator to consume the generator),
-    but will also detect other kinds of return values (these are not
-    necessarily errors, but we alert anyway since there is no good
-    reason to return a value from a test).
-    """
-
-    def __init__(self, orig_method: Callable) -> None:
-        self.orig_method = orig_method
-        self.__wrapped__ = orig_method
-
-    def __call__(self, *args: Any, **kwargs: Any) -> None:
-        result = self.orig_method(*args, **kwargs)
-        if isinstance(result, Generator) or inspect.iscoroutine(result):
-            raise TypeError(
-                "Generator and coroutine test methods should be"
-                " decorated with tornado.testing.gen_test"
-            )
-        elif result is not None:
-            raise ValueError("Return value from test method ignored: %r" % result)
-
-    def __getattr__(self, name: str) -> Any:
-        """Proxy all unknown attributes to the original method.
-
-        This is important for some of the decorators in the `unittest`
-        module, such as `unittest.skipIf`.
-        """
-        return getattr(self.orig_method, name)
-
-
 class AsyncTestCase(unittest.TestCase):
     """`~unittest.TestCase` subclass for testing `.IOLoop`-based
     asynchronous code.
@@ -173,12 +140,6 @@ class AsyncTestCase(unittest.TestCase):
         self.__stop_args = None  # type: Any
         self.__timeout = None  # type: Optional[object]
 
-        # It's easy to forget the @gen_test decorator, but if you do
-        # the test will silently be ignored because nothing will consume
-        # the generator.  Replace the test method with a wrapper that will
-        # make sure it's not an undecorated generator.
-        setattr(self, methodName, _TestMethodWrapper(getattr(self, methodName)))
-
         # Not used in this class itself, but used by @gen_test
         self._test_generator = None  # type: Optional[Union[Generator, Coroutine]]
 
@@ -289,6 +250,30 @@ class AsyncTestCase(unittest.TestCase):
         self.__rethrow()
         return ret
 
+    def _callTestMethod(self, method: Callable) -> None:
+        """Run the given test method, raising an error if it returns non-None.
+
+        Failure to decorate asynchronous test methods with ``@gen_test`` can lead to tests
+        incorrectly passing.
+
+        Remove this override when Python 3.10 support is dropped. This check (in the form of a
+        DeprecationWarning) became a part of the standard library in 3.11.
+
+        Note that ``_callTestMethod`` is not documented as a public interface. However, it is
+        present in all supported versions of Python (3.8+), and if it goes away in the future that's
+        OK because we can just remove this override as noted above.
+        """
+        # Calling super()._callTestMethod would hide the return value, even in python 3.8-3.10
+        # where the check isn't being done for us.
+        result = method()
+        if isinstance(result, Generator) or inspect.iscoroutine(result):
+            raise TypeError(
+                "Generator and coroutine test methods should be"
+                " decorated with tornado.testing.gen_test"
+            )
+        elif result is not None:
+            raise ValueError("Return value from test method ignored: %r" % result)
+
     def stop(self, _arg: Any = None, **kwargs: Any) -> None:
         """Stops the `.IOLoop`, causing one pending (or future) call to `wait()`
         to return.