From 8b3c3ea4aee5911cc65b0de01553c86cf1dc9df0 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Mon, 30 Jun 2025 12:19:23 +0500 Subject: [PATCH] Refactor test functions that were returning a Deferred directly. (#6889) --- .../test_downloader_handler_twisted_http10.py | 6 +- .../test_downloader_handler_twisted_http2.py | 15 +-- tests/test_downloader_handlers.py | 103 +++++++++--------- tests/test_downloader_handlers_http_base.py | 40 ++++--- tests/test_downloadermiddleware.py | 8 +- tests/test_utils_defer.py | 24 ++-- 6 files changed, 100 insertions(+), 96 deletions(-) diff --git a/tests/test_downloader_handler_twisted_http10.py b/tests/test_downloader_handler_twisted_http10.py index bc306aa07..4b700be6b 100644 --- a/tests/test_downloader_handler_twisted_http10.py +++ b/tests/test_downloader_handler_twisted_http10.py @@ -39,8 +39,10 @@ class TestHttps10(TestHttp10): @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") class TestHttp10Proxy(HTTP10DownloadHandlerMixin, TestHttpProxyBase): - def test_download_with_proxy_https_timeout(self): + @deferred_f_from_coro_f + async def test_download_with_proxy_https_timeout(self): pytest.skip("Not implemented") - def test_download_with_proxy_without_http_scheme(self): + @deferred_f_from_coro_f + async def test_download_with_proxy_without_http_scheme(self): pytest.skip("Not implemented") diff --git a/tests/test_downloader_handler_twisted_http2.py b/tests/test_downloader_handler_twisted_http2.py index 3e685bb28..77077e6e5 100644 --- a/tests/test_downloader_handler_twisted_http2.py +++ b/tests/test_downloader_handler_twisted_http2.py @@ -89,22 +89,13 @@ class TestHttps2(H2DownloadHandlerMixin, TestHttps11Base): with pytest.raises(SchemeNotSupported): await self.download_request(request, Spider("foo")) - def test_download_broken_content_cause_data_loss(self, url="broken"): + async def _test_download_cause_data_loss(self, url: str) -> None: pytest.skip(self.HTTP2_DATALOSS_SKIP_REASON) - def test_download_broken_chunked_content_cause_data_loss(self): + async def _test_download_allow_data_loss(self, url: str) -> None: pytest.skip(self.HTTP2_DATALOSS_SKIP_REASON) - def test_download_broken_content_allow_data_loss(self, url="broken"): - pytest.skip(self.HTTP2_DATALOSS_SKIP_REASON) - - def test_download_broken_chunked_content_allow_data_loss(self): - pytest.skip(self.HTTP2_DATALOSS_SKIP_REASON) - - def test_download_broken_content_allow_data_loss_via_setting(self, url="broken"): - pytest.skip(self.HTTP2_DATALOSS_SKIP_REASON) - - def test_download_broken_chunked_content_allow_data_loss_via_setting(self): + async def _test_download_allow_data_loss_via_setting(self, url: str) -> None: pytest.skip(self.HTTP2_DATALOSS_SKIP_REASON) @deferred_f_from_coro_f diff --git a/tests/test_downloader_handlers.py b/tests/test_downloader_handlers.py index 518dc6b24..0775ae1ee 100644 --- a/tests/test_downloader_handlers.py +++ b/tests/test_downloader_handlers.py @@ -347,58 +347,55 @@ class TestFTPBase(unittest.TestCase): yield self.port.stopListening() shutil.rmtree(self.directory) - def _add_test_callbacks(self, deferred, callback=None, errback=None): - def _clean(data): - self.download_handler.client.transport.loseConnection() - return data + async def download_request(self, request: Request) -> Response: + return await maybe_deferred_to_future( + self.download_handler.download_request(request, None) + ) - deferred.addCallback(_clean) - if callback: - deferred.addCallback(callback) - if errback: - deferred.addErrback(errback) - return deferred + def _lose_connection(self): + self.download_handler.client.transport.loseConnection() - def test_ftp_download_success(self): + @deferred_f_from_coro_f + async def test_ftp_download_success(self): request = Request( url=f"ftp://127.0.0.1:{self.portNum}/file.txt", meta=self.req_meta ) - d = self.download_handler.download_request(request, None) - - def _test(r): + try: + r = await self.download_request(request) assert r.status == 200 assert r.body == b"I have the power!" assert r.headers == {b"Local Filename": [b""], b"Size": [b"17"]} assert r.protocol is None + finally: + self._lose_connection() - return self._add_test_callbacks(d, _test) - - def test_ftp_download_path_with_spaces(self): + @deferred_f_from_coro_f + async def test_ftp_download_path_with_spaces(self): request = Request( url=f"ftp://127.0.0.1:{self.portNum}/file with spaces.txt", meta=self.req_meta, ) - d = self.download_handler.download_request(request, None) - - def _test(r): + try: + r = await self.download_request(request) assert r.status == 200 assert r.body == b"Moooooooooo power!" assert r.headers == {b"Local Filename": [b""], b"Size": [b"18"]} + finally: + self._lose_connection() - return self._add_test_callbacks(d, _test) - - def test_ftp_download_nonexistent(self): + @deferred_f_from_coro_f + async def test_ftp_download_nonexistent(self): request = Request( url=f"ftp://127.0.0.1:{self.portNum}/nonexistent.txt", meta=self.req_meta ) - d = self.download_handler.download_request(request, None) - - def _test(r): + try: + r = await self.download_request(request) assert r.status == 404 + finally: + self._lose_connection() - return self._add_test_callbacks(d, _test) - - def test_ftp_local_filename(self): + @deferred_f_from_coro_f + async def test_ftp_local_filename(self): f, local_fname = mkstemp() fname_bytes = to_bytes(local_fname) local_fname = Path(local_fname) @@ -406,41 +403,42 @@ class TestFTPBase(unittest.TestCase): meta = {"ftp_local_filename": fname_bytes} meta.update(self.req_meta) request = Request(url=f"ftp://127.0.0.1:{self.portNum}/file.txt", meta=meta) - d = self.download_handler.download_request(request, None) - - def _test(r): + try: + r = await self.download_request(request) assert r.body == fname_bytes assert r.headers == {b"Local Filename": [fname_bytes], b"Size": [b"17"]} assert local_fname.exists() assert local_fname.read_bytes() == b"I have the power!" local_fname.unlink() + finally: + self._lose_connection() - return self._add_test_callbacks(d, _test) - - def _test_response_class(self, filename, response_class): + async def _test_response_class(self, filename: str, response_class: type[Response]): f, local_fname = mkstemp() - local_fname = Path(local_fname) + local_fname_path = Path(local_fname) os.close(f) meta = {} meta.update(self.req_meta) request = Request(url=f"ftp://127.0.0.1:{self.portNum}/{filename}", meta=meta) - d = self.download_handler.download_request(request, None) - - def _test(r): + try: + r = await self.download_request(request) assert type(r) is response_class # pylint: disable=unidiomatic-typecheck - local_fname.unlink() + local_fname_path.unlink() + finally: + self._lose_connection() - return self._add_test_callbacks(d, _test) + @deferred_f_from_coro_f + async def test_response_class_from_url(self): + await self._test_response_class("file.txt", TextResponse) - def test_response_class_from_url(self): - return self._test_response_class("file.txt", TextResponse) - - def test_response_class_from_body(self): - return self._test_response_class("html-file-without-extension", HtmlResponse) + @deferred_f_from_coro_f + async def test_response_class_from_body(self): + await self._test_response_class("html-file-without-extension", HtmlResponse) class TestFTP(TestFTPBase): - def test_invalid_credentials(self): + @deferred_f_from_coro_f + async def test_invalid_credentials(self): if self.reactor_pytest != "default" and sys.platform == "win32": pytest.skip( "This test produces DirtyReactorAggregateError on Windows with asyncio" @@ -450,12 +448,11 @@ class TestFTP(TestFTPBase): meta = dict(self.req_meta) meta.update({"ftp_password": "invalid"}) request = Request(url=f"ftp://127.0.0.1:{self.portNum}/file.txt", meta=meta) - d = self.download_handler.download_request(request, None) - - def _test(r): - assert r.type == ConnectionLost - - return self._add_test_callbacks(d, errback=_test) + try: + with pytest.raises(ConnectionLost): + await self.download_request(request) + finally: + self._lose_connection() class TestAnonymousFTP(TestFTPBase): diff --git a/tests/test_downloader_handlers_http_base.py b/tests/test_downloader_handlers_http_base.py index 9b2c49fd4..1cdbbc831 100644 --- a/tests/test_downloader_handlers_http_base.py +++ b/tests/test_downloader_handlers_http_base.py @@ -419,34 +419,35 @@ class TestHttp11Base(TestHttpBase): response = await self.download_request(request, Spider("foo")) assert response.body == b"chunked content\n" - @deferred_f_from_coro_f - async def test_download_broken_content_cause_data_loss( - self, url: str = "broken" - ) -> None: + async def _test_download_cause_data_loss(self, url: str) -> None: # TODO: this one checks for Twisted-specific exceptions request = Request(self.getURL(url)) with pytest.raises(ResponseFailed) as exc_info: await self.download_request(request, Spider("foo")) assert any(r.check(_DataLoss) for r in exc_info.value.reasons) - def test_download_broken_chunked_content_cause_data_loss(self): - return self.test_download_broken_content_cause_data_loss("broken-chunked") + @deferred_f_from_coro_f + async def test_download_broken_content_cause_data_loss(self) -> None: + await self._test_download_cause_data_loss("broken") @deferred_f_from_coro_f - async def test_download_broken_content_allow_data_loss( - self, url: str = "broken" - ) -> None: + async def test_download_broken_chunked_content_cause_data_loss(self): + await self._test_download_cause_data_loss("broken-chunked") + + async def _test_download_allow_data_loss(self, url: str) -> None: request = Request(self.getURL(url), meta={"download_fail_on_dataloss": False}) response = await self.download_request(request, Spider("foo")) assert response.flags == ["dataloss"] - def test_download_broken_chunked_content_allow_data_loss(self): - return self.test_download_broken_content_allow_data_loss("broken-chunked") + @deferred_f_from_coro_f + async def test_download_broken_content_allow_data_loss(self) -> None: + await self._test_download_allow_data_loss("broken") @deferred_f_from_coro_f - async def test_download_broken_content_allow_data_loss_via_setting( - self, url: str = "broken" - ) -> None: + async def test_download_broken_chunked_content_allow_data_loss(self): + await self._test_download_allow_data_loss("broken-chunked") + + async def _test_download_allow_data_loss_via_setting(self, url: str) -> None: crawler = get_crawler(settings_dict={"DOWNLOAD_FAIL_ON_DATALOSS": False}) download_handler = build_from_crawler(self.download_handler_cls, crawler) request = Request(self.getURL(url)) @@ -455,10 +456,13 @@ class TestHttp11Base(TestHttpBase): ) assert response.flags == ["dataloss"] - def test_download_broken_chunked_content_allow_data_loss_via_setting(self): - return self.test_download_broken_content_allow_data_loss_via_setting( - "broken-chunked" - ) + @deferred_f_from_coro_f + async def test_download_broken_content_allow_data_loss_via_setting(self): + await self._test_download_allow_data_loss_via_setting("broken-chunked") + + @deferred_f_from_coro_f + async def test_download_broken_chunked_content_allow_data_loss_via_setting(self): + await self._test_download_allow_data_loss_via_setting("broken-chunked") @deferred_f_from_coro_f async def test_protocol(self): diff --git a/tests/test_downloadermiddleware.py b/tests/test_downloadermiddleware.py index 61a5a7df5..d12baf1ad 100644 --- a/tests/test_downloadermiddleware.py +++ b/tests/test_downloadermiddleware.py @@ -5,7 +5,7 @@ from gzip import BadGzipFile from unittest import mock import pytest -from twisted.internet.defer import Deferred, succeed +from twisted.internet.defer import Deferred, inlineCallbacks, succeed from twisted.trial.unittest import TestCase from scrapy.core.downloader.middleware import DownloaderMiddlewareManager @@ -20,15 +20,17 @@ from scrapy.utils.test import get_crawler, get_from_asyncio_queue class TestManagerBase(TestCase): settings_dict = None + @inlineCallbacks def setUp(self): self.crawler = get_crawler(Spider, self.settings_dict) self.spider = self.crawler._create_spider("foo") self.mwman = DownloaderMiddlewareManager.from_crawler(self.crawler) self.crawler.engine = self.crawler._create_engine() - return self.crawler.engine.open_spider(self.spider) + yield self.crawler.engine.open_spider(self.spider) + @inlineCallbacks def tearDown(self): - return self.crawler.engine.close_spider(self.spider) + yield self.crawler.engine.close_spider(self.spider) async def _download( self, request: Request, response: Response | None = None diff --git a/tests/test_utils_defer.py b/tests/test_utils_defer.py index 1cfaf70fa..36533ad5d 100644 --- a/tests/test_utils_defer.py +++ b/tests/test_utils_defer.py @@ -29,19 +29,24 @@ if TYPE_CHECKING: class TestMustbeDeferred(unittest.TestCase): - def test_success_function(self) -> Deferred[list[int]]: + @inlineCallbacks + def test_success_function(self) -> Generator[Deferred[Any], Any, None]: steps: list[int] = [] def _append(v: int) -> list[int]: steps.append(v) return steps - dfd = mustbe_deferred(_append, 1) - dfd.addCallback(self.assertEqual, [1, 2]) # it is [1] with maybeDeferred - steps.append(2) # add another value, that should be caught by assertEqual - return dfd + def _assert(v: list[int]) -> None: + assert v == [1, 2] # it is [1] with maybeDeferred - def test_unfired_deferred(self) -> Deferred[list[int]]: + dfd = mustbe_deferred(_append, 1) + dfd.addCallback(_assert) + steps.append(2) # add another value, that should be caught by assertEqual + yield dfd + + @inlineCallbacks + def test_unfired_deferred(self) -> Generator[Deferred[Any], Any, None]: steps: list[int] = [] def _append(v: int) -> Deferred[list[int]]: @@ -52,10 +57,13 @@ class TestMustbeDeferred(unittest.TestCase): reactor.callLater(0, dfd.callback, steps) return dfd + def _assert(v: list[int]) -> None: + assert v == [1, 2] + dfd = mustbe_deferred(_append, 1) - dfd.addCallback(self.assertEqual, [1, 2]) # it is [1] with maybeDeferred + dfd.addCallback(_assert) steps.append(2) # add another value, that should be caught by assertEqual - return dfd + yield dfd def cb1(value, arg1, arg2):