From: Ben Darnell Date: Mon, 3 Jun 2024 19:49:59 +0000 (-0400) Subject: testing: Replace _TestMethodWrapper with _callTestMethod X-Git-Tag: v6.4.1~7^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F3382%2Fhead;p=thirdparty%2Ftornado.git 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 0429feee8..4432bb175 100644 --- 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 bdbff87bc..4c33b3e22 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 @@ 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.