From 180332ad5e9473ad951a6e3924ac0624ef6fa631 Mon Sep 17 00:00:00 2001 From: Ben Darnell Date: Mon, 3 Jun 2024 15:49:59 -0400 Subject: [PATCH] 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 --- tornado/test/testing_test.py | 23 ++++++------- tornado/testing.py | 63 ++++++++++++++---------------------- 2 files changed, 33 insertions(+), 53 deletions(-) diff --git a/tornado/test/testing_test.py b/tornado/test/testing_test.py index 0429feee83..4432bb1758 100644 --- a/tornado/test/testing_test.py +++ b/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): diff --git a/tornado/testing.py b/tornado/testing.py index bdbff87bc3..4c33b3e22f 100644 --- 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 @@ 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.