Merge pull request #3382 from bdarnell/remove-testmethodwrapper

testing: Replace _TestMethodWrapper with _callTestMethod

Author: bdarnell

tornado/test/testing_test.py

@@ -5,7 +5,6 @@
 from tornado.web import Application
 import asyncio
 import contextlib
-import inspect
 import gc
 import os
 import platform
@@ -118,7 +117,11 @@ def tearDown(self):
         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 @@ def test_gen(self):
         "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 @@ def test_other_return(self):
         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):

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 @@ def __init__(self, methodName: str = "runTest") -> None:
         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 @@ def run(
         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.