Assorted test improvements (#7232)

* Await some missed deferreds.

* Simplify test_engine_stop_download_*.

* Fix the header value.

* Extend TestHttpWithCrawlerBase.

* Make test_tls_logging more universal.

* Add more tests for header handling.

* Clarify certificate and ip_address integration tests.

* Make test_download_with_maxsize_very_large_file() more universal.

* Fix test_coroutine_asyncio().

* Typo.

tests/test_crawl.py

@@ -633,43 +633,52 @@ class TestCrawlSpider:
         yield crawler.crawl(seed=url, mockserver=self.mockserver)
         assert crawler.spider.meta["responses"][0].certificate is None
 
-    @inlineCallbacks
+    @pytest.mark.parametrize(
-    def test_response_ssl_certificate(self):
+        "url",
-        crawler = get_crawler(SingleRequestSpider)
+        [
-        url = self.mockserver.url("/echo?body=test", is_secure=True)
+            "/echo?body=test",
-        yield crawler.crawl(seed=url, mockserver=self.mockserver)
+            pytest.param(
-        cert = crawler.spider.meta["responses"][0].certificate
+                "/status?n=200",
-        assert isinstance(cert, Certificate)
+                marks=pytest.mark.xfail(
-        assert cert.getSubject().commonName == b"localhost"
+                    reason="With HTTP11DownloadHandler, responses with no body are returned early and contain no certificate",
-        assert cert.getIssuer().commonName == b"localhost"
+                    strict=True,
-
+                ),
-    @pytest.mark.xfail(
+            ),
-        reason="Responses with no body return early and contain no certificate"
+        ],
     )
-    @inlineCallbacks
+    @deferred_f_from_coro_f
-    def test_response_ssl_certificate_empty_response(self):
+    async def test_response_ssl_certificate(
+        self, mockserver: MockServer, url: str
+    ) -> None:
         crawler = get_crawler(SingleRequestSpider)
-        url = self.mockserver.url("/status?n=200", is_secure=True)
+        url = mockserver.url(url, is_secure=True)
-        yield crawler.crawl(seed=url, mockserver=self.mockserver)
+        await crawler.crawl_async(seed=url, mockserver=mockserver)
+        assert isinstance(crawler.spider, SingleRequestSpider)
         cert = crawler.spider.meta["responses"][0].certificate
         assert isinstance(cert, Certificate)
         assert cert.getSubject().commonName == b"localhost"
         assert cert.getIssuer().commonName == b"localhost"
 
-    @inlineCallbacks
+    @pytest.mark.parametrize(
-    def test_dns_server_ip_address_none(self):
+        "url",
+        [
+            "/echo?body=test",
+            pytest.param(
+                "/status?n=200",
+                marks=pytest.mark.xfail(
+                    reason="With HTTP11DownloadHandler, responses with no body are returned early and contain no ip_address",
+                    strict=True,
+                ),
+            ),
+        ],
+    )
+    @deferred_f_from_coro_f
+    async def test_response_ip_address(self, mockserver: MockServer, url: str) -> None:
         crawler = get_crawler(SingleRequestSpider)
-        url = self.mockserver.url("/status?n=200")
+        url = mockserver.url(url)
-        yield crawler.crawl(seed=url, mockserver=self.mockserver)
-        ip_address = crawler.spider.meta["responses"][0].ip_address
-        assert ip_address is None
-
-    @inlineCallbacks
-    def test_dns_server_ip_address(self):
-        crawler = get_crawler(SingleRequestSpider)
-        url = self.mockserver.url("/echo?body=test")
         expected_netloc, _ = urlparse(url).netloc.split(":")
-        yield crawler.crawl(seed=url, mockserver=self.mockserver)
+        await crawler.crawl_async(seed=url, mockserver=mockserver)
+        assert isinstance(crawler.spider, SingleRequestSpider)
         ip_address = crawler.spider.meta["responses"][0].ip_address
         assert isinstance(ip_address, IPv4Address)
         assert str(ip_address) == gethostbyname(expected_netloc)

tests/test_downloader_handler_twisted_http11.py

@@ -60,7 +60,16 @@ class TestHttps11CustomCiphers(HTTP11DownloadHandlerMixin, TestHttpsCustomCipher
 class TestHttp11WithCrawler(TestHttpWithCrawlerBase):
     @property
     def settings_dict(self) -> dict[str, Any] | None:
-        return None  # default handler settings
+        return {
+            "DOWNLOAD_HANDLERS": {
+                "http": "scrapy.core.downloader.handlers.http11.HTTP11DownloadHandler",
+                "https": "scrapy.core.downloader.handlers.http11.HTTP11DownloadHandler",
+            }
+        }
+
+
+class TestHttps11WithCrawler(TestHttp11WithCrawler):
+    is_secure = True
 
 
 class TestHttp11Proxy(HTTP11DownloadHandlerMixin, TestHttpProxyBase):

tests/test_downloader_handler_twisted_http2.py

@@ -4,14 +4,12 @@ from __future__ import annotations
 
 import json
 from typing import TYPE_CHECKING, Any
-from unittest import mock
 
 import pytest
 from testfixtures import LogCapture
-from twisted.internet import defer
 from twisted.web.http import H2_ENABLED
 
-from scrapy.exceptions import DownloadCancelledError, UnsupportedURLSchemeError
+from scrapy.exceptions import UnsupportedURLSchemeError
 from scrapy.http import Request
 from scrapy.utils.defer import deferred_f_from_coro_f, maybe_deferred_to_future
 from tests.test_downloader_handlers_http_base import (
@@ -57,32 +55,6 @@ class TestHttps2(H2DownloadHandlerMixin, TestHttps11Base):
             response = await download_handler.download_request(request)
         assert response.protocol == "h2"
 
-    @deferred_f_from_coro_f
-    async def test_download_with_maxsize_very_large_file(
-        self, mockserver: MockServer
-    ) -> None:
-        from twisted.internet import reactor
-
-        with mock.patch("scrapy.core.http2.stream.logger") as logger:
-            request = Request(
-                mockserver.url("/largechunkedfile", is_secure=self.is_secure)
-            )
-
-            def check(logger: mock.Mock) -> None:
-                logger.error.assert_called_once_with(mock.ANY)
-
-            async with self.get_dh({"DOWNLOAD_MAXSIZE": 1_500}) as download_handler:
-                with pytest.raises(DownloadCancelledError):
-                    await download_handler.download_request(request)
-
-            # As the error message is logged in the dataReceived callback, we
-            # have to give a bit of time to the reactor to process the queue
-            # after closing the connection.
-            d: defer.Deferred[mock.Mock] = defer.Deferred()
-            d.addCallback(check)
-            reactor.callLater(0.1, d.callback, logger)
-            await maybe_deferred_to_future(d)
-
     def test_download_cause_data_loss(self) -> None:  # type: ignore[override]
         pytest.skip(self.HTTP2_DATALOSS_SKIP_REASON)
 
@@ -206,7 +178,8 @@ class TestHttp2WithCrawler(TestHttpWithCrawlerBase):
     def settings_dict(self) -> dict[str, Any] | None:
         return {
             "DOWNLOAD_HANDLERS": {
-                "https": "scrapy.core.downloader.handlers.http2.H2DownloadHandler"
+                "http": None,
+                "https": "scrapy.core.downloader.handlers.http2.H2DownloadHandler",
             }
         }
 

tests/test_downloader_handlers_http_base.py

@@ -8,12 +8,13 @@ import sys
 from abc import ABC, abstractmethod
 from contextlib import asynccontextmanager
 from http import HTTPStatus
+from ipaddress import IPv4Address
+from socket import gethostbyname
 from typing import TYPE_CHECKING, Any
-from unittest import mock
+from urllib.parse import urlparse
 
 import pytest
-from testfixtures import LogCapture
+from twisted.internet.ssl import Certificate
-from twisted.internet import defer
 
 from scrapy.exceptions import (
     CannotResolveHostError,
@@ -25,7 +26,6 @@ from scrapy.exceptions import (
     UnsupportedURLSchemeError,
 )
 from scrapy.http import Headers, HtmlResponse, Request, Response, TextResponse
-from scrapy.utils.asyncio import call_later
 from scrapy.utils.defer import (
     deferred_f_from_coro_f,
     deferred_from_coro,
@@ -135,6 +135,48 @@ class TestHttpBase(ABC):
             assert header_name in body["headers"]
             assert body["headers"][header_name] == [header_value]
 
+    @deferred_f_from_coro_f
+    async def test_request_header_none(self, mockserver: MockServer) -> None:
+        """Adding a header with None as the value should not send that header."""
+        request_headers = {
+            "Cookie": None,
+            "X-Custom-Header": None,
+        }
+        request = Request(
+            mockserver.url("/echo", is_secure=self.is_secure),
+            headers=request_headers,
+        )
+        async with self.get_dh() as download_handler:
+            response = await download_handler.download_request(request)
+        assert response.status == HTTPStatus.OK
+        body = json.loads(response.body.decode("utf-8"))
+        assert "headers" in body
+        for header_name in request_headers:
+            assert header_name not in body["headers"]
+
+    @pytest.mark.parametrize(
+        "request_headers",
+        [
+            {"X-Custom-Header": ["foo", "bar"]},
+            [("X-Custom-Header", "foo"), ("X-Custom-Header", "bar")],
+        ],
+    )
+    @deferred_f_from_coro_f
+    async def test_request_header_duplicate(
+        self, mockserver: MockServer, request_headers: Any
+    ) -> None:
+        """All values for a header should be sent."""
+        request = Request(
+            mockserver.url("/echo", is_secure=self.is_secure),
+            headers=request_headers,
+        )
+        async with self.get_dh() as download_handler:
+            response = await download_handler.download_request(request)
+        assert response.status == HTTPStatus.OK
+        body = json.loads(response.body.decode("utf-8"))
+        assert "headers" in body
+        assert body["headers"]["X-Custom-Header"] == ["foo", "bar"]
+
     @deferred_f_from_coro_f
     async def test_server_receives_correct_request_body(
         self, mockserver: MockServer
@@ -166,7 +208,7 @@ class TestHttpBase(ABC):
             "Content-Encoding": "gzip",
             "Content-MD5": "Q2hlY2sgSW50ZWdyaXR5IQ==",
             "Content-Type": "text/html; charset=utf-8",
-            "Date": "Date: Tue, 15 Nov 1994 08:12:31 GMT",
+            "Date": "Tue, 15 Nov 1994 08:12:31 GMT",
             "Pragma": "no-cache",
             "Retry-After": "120",
             "Set-Cookie": "CookieName=CookieValue; Max-Age=3600; Version=1",
@@ -450,28 +492,14 @@ class TestHttp11Base(TestHttpBase):
 
     @deferred_f_from_coro_f
     async def test_download_with_maxsize_very_large_file(
-        self, mockserver: MockServer
+        self, mockserver: MockServer, caplog: pytest.LogCaptureFixture
     ) -> None:
-        # TODO: the logger check is specific to scrapy.core.downloader.handlers.http11
+        request = Request(mockserver.url("/largechunkedfile", is_secure=self.is_secure))
-        with mock.patch("scrapy.core.downloader.handlers.http11.logger") as logger:
+        async with self.get_dh({"DOWNLOAD_MAXSIZE": 1_500}) as download_handler:
-            request = Request(
+            with pytest.raises(DownloadCancelledError):
-                mockserver.url("/largechunkedfile", is_secure=self.is_secure)
+                await download_handler.download_request(request)
-            )
 
-            def check(logger: mock.Mock) -> None:
+        assert "larger than download max size" in caplog.text
-                logger.warning.assert_called_once_with(mock.ANY, mock.ANY)
-
-            async with self.get_dh({"DOWNLOAD_MAXSIZE": 1_500}) as download_handler:
-                with pytest.raises(DownloadCancelledError):
-                    await download_handler.download_request(request)
-
-            # As the error message is logged in the dataReceived callback, we
-            # have to give a bit of time to the reactor to process the queue
-            # after closing the connection.
-            d: defer.Deferred[mock.Mock] = defer.Deferred()
-            d.addCallback(check)
-            call_later(0.1, d.callback, logger)
-            await maybe_deferred_to_future(d)
 
     @deferred_f_from_coro_f
     async def test_download_with_maxsize_per_req(self, mockserver: MockServer) -> None:
@@ -618,17 +646,17 @@ class TestHttps11Base(TestHttp11Base):
         pytest.skip("Unable to test on HTTPS")
 
     @deferred_f_from_coro_f
-    async def test_tls_logging(self, mockserver: MockServer) -> None:
+    async def test_tls_logging(
+        self, mockserver: MockServer, caplog: pytest.LogCaptureFixture
+    ) -> None:
         request = Request(mockserver.url("/text", is_secure=self.is_secure))
         async with self.get_dh(
             {"DOWNLOADER_CLIENT_TLS_VERBOSE_LOGGING": True}
         ) as download_handler:
-            with LogCapture() as log_capture:
+            with caplog.at_level("DEBUG"):
                 response = await download_handler.download_request(request)
         assert response.body == b"Works"
-        log_capture.check_present(
+        assert self.tls_log_message in caplog.text
-            ("scrapy.core.downloader.tls", "DEBUG", self.tls_log_message)
-        )
 
 
 class TestSimpleHttpsBase(ABC):
@@ -744,6 +772,33 @@ class TestHttpWithCrawlerBase(ABC):
         reason = crawler.spider.meta["close_reason"]  # type: ignore[attr-defined]
         assert reason == "finished"
 
+    @deferred_f_from_coro_f
+    async def test_response_ssl_certificate(self, mockserver: MockServer) -> None:
+        if not self.is_secure:
+            pytest.skip("Only applies to HTTPS")
+        # copy of TestCrawl.test_response_ssl_certificate()
+        # the current test implementation can only work for Twisted-based download handlers
+        crawler = get_crawler(SingleRequestSpider, self.settings_dict)
+        url = mockserver.url("/echo?body=test", is_secure=self.is_secure)
+        await crawler.crawl_async(seed=url, mockserver=mockserver)
+        assert isinstance(crawler.spider, SingleRequestSpider)
+        cert = crawler.spider.meta["responses"][0].certificate
+        assert isinstance(cert, Certificate)
+        assert cert.getSubject().commonName == b"localhost"
+        assert cert.getIssuer().commonName == b"localhost"
+
+    @deferred_f_from_coro_f
+    async def test_response_ip_address(self, mockserver: MockServer) -> None:
+        # copy of TestCrawl.test_response_ip_address()
+        crawler = get_crawler(SingleRequestSpider, self.settings_dict)
+        url = mockserver.url("/echo?body=test", is_secure=self.is_secure)
+        expected_netloc, _ = urlparse(url).netloc.split(":")
+        await crawler.crawl_async(seed=url, mockserver=mockserver)
+        assert isinstance(crawler.spider, SingleRequestSpider)
+        ip_address = crawler.spider.meta["responses"][0].ip_address
+        assert isinstance(ip_address, IPv4Address)
+        assert str(ip_address) == gethostbyname(expected_netloc)
+
 
 class TestHttpProxyBase(ABC):
     is_secure = False

tests/test_engine_stop_download_bytes.py

@@ -2,8 +2,6 @@ from __future__ import annotations
 
 from typing import TYPE_CHECKING
 
-from testfixtures import LogCapture
-
 from scrapy.exceptions import StopDownload
 from scrapy.utils.defer import deferred_f_from_coro_f
 from tests.test_engine import (
@@ -16,6 +14,8 @@ from tests.test_engine import (
 )
 
 if TYPE_CHECKING:
+    import pytest
+
     from tests.mockserver.http import MockServer
 
 
@@ -27,7 +27,9 @@ class BytesReceivedCrawlerRun(CrawlerRun):
 
 class TestBytesReceivedEngine(TestEngineBase):
     @deferred_f_from_coro_f
-    async def test_crawler(self, mockserver: MockServer) -> None:
+    async def test_crawler(
+        self, mockserver: MockServer, caplog: pytest.LogCaptureFixture
+    ) -> None:
         for spider in (
             MySpider,
             DictItemsSpider,
@@ -35,32 +37,13 @@ class TestBytesReceivedEngine(TestEngineBase):
             DataClassItemsSpider,
         ):
             run = BytesReceivedCrawlerRun(spider)
-            with LogCapture() as log:
+            with caplog.at_level("DEBUG"):
                 await run.run(mockserver)
-                log.check_present(
+            for url in ("/redirected", "/static/", "/numbers"):
-                    (
+                assert (
-                        "scrapy.core.downloader.handlers.http11",
+                    f"Download stopped for <GET {mockserver.url(url)}> "
-                        "DEBUG",
+                    "from signal handler BytesReceivedCrawlerRun.bytes_received"
-                        f"Download stopped for <GET {mockserver.url('/redirected')}> "
+                ) in caplog.text
-                        "from signal handler BytesReceivedCrawlerRun.bytes_received",
-                    )
-                )
-                log.check_present(
-                    (
-                        "scrapy.core.downloader.handlers.http11",
-                        "DEBUG",
-                        f"Download stopped for <GET {mockserver.url('/static/')}> "
-                        "from signal handler BytesReceivedCrawlerRun.bytes_received",
-                    )
-                )
-                log.check_present(
-                    (
-                        "scrapy.core.downloader.handlers.http11",
-                        "DEBUG",
-                        f"Download stopped for <GET {mockserver.url('/numbers')}> "
-                        "from signal handler BytesReceivedCrawlerRun.bytes_received",
-                    )
-                )
             self._assert_visited_urls(run)
             self._assert_scheduled_requests(run, count=9)
             self._assert_downloaded_responses(run, count=9)

tests/test_engine_stop_download_headers.py

@@ -2,8 +2,6 @@ from __future__ import annotations
 
 from typing import TYPE_CHECKING
 
-from testfixtures import LogCapture
-
 from scrapy.exceptions import StopDownload
 from scrapy.utils.defer import deferred_f_from_coro_f
 from tests.test_engine import (
@@ -16,6 +14,8 @@ from tests.test_engine import (
 )
 
 if TYPE_CHECKING:
+    import pytest
+
     from tests.mockserver.http import MockServer
 
 
@@ -27,7 +27,9 @@ class HeadersReceivedCrawlerRun(CrawlerRun):
 
 class TestHeadersReceivedEngine(TestEngineBase):
     @deferred_f_from_coro_f
-    async def test_crawler(self, mockserver: MockServer) -> None:
+    async def test_crawler(
+        self, mockserver: MockServer, caplog: pytest.LogCaptureFixture
+    ) -> None:
         for spider in (
             MySpider,
             DictItemsSpider,
@@ -35,32 +37,13 @@ class TestHeadersReceivedEngine(TestEngineBase):
             DataClassItemsSpider,
         ):
             run = HeadersReceivedCrawlerRun(spider)
-            with LogCapture() as log:
+            with caplog.at_level("DEBUG"):
                 await run.run(mockserver)
-                log.check_present(
+            for url in ("/redirected", "/static/", "/numbers"):
-                    (
+                assert (
-                        "scrapy.core.downloader.handlers.http11",
+                    f"Download stopped for <GET {mockserver.url(url)}> "
-                        "DEBUG",
+                    "from signal handler HeadersReceivedCrawlerRun.headers_received"
-                        f"Download stopped for <GET {mockserver.url('/redirected')}> from"
+                ) in caplog.text
-                        " signal handler HeadersReceivedCrawlerRun.headers_received",
-                    )
-                )
-                log.check_present(
-                    (
-                        "scrapy.core.downloader.handlers.http11",
-                        "DEBUG",
-                        f"Download stopped for <GET {mockserver.url('/static/')}> from signal"
-                        " handler HeadersReceivedCrawlerRun.headers_received",
-                    )
-                )
-                log.check_present(
-                    (
-                        "scrapy.core.downloader.handlers.http11",
-                        "DEBUG",
-                        f"Download stopped for <GET {mockserver.url('/numbers')}> from"
-                        " signal handler HeadersReceivedCrawlerRun.headers_received",
-                    )
-                )
             self._assert_visited_urls(run)
             self._assert_downloaded_responses(run, count=6)
             self._assert_signals_caught(run)

tests/test_pipelines.py

@@ -476,7 +476,8 @@ class TestMiddlewareManagerSpider:
         ):
             await mwman.close_spider_async()
 
-    def test_deprecated_spider_arg_with_crawler(self, crawler: Crawler) -> None:
+    @deferred_f_from_coro_f
+    async def test_deprecated_spider_arg_with_crawler(self, crawler: Crawler) -> None:
         """Crawler is provided and has a spider, works. The instance passed to a deprecated method
         is ignored, even if mismatched."""
         mwman = ItemPipelineManager(crawler=crawler)
@@ -485,14 +486,15 @@ class TestMiddlewareManagerSpider:
             ScrapyDeprecationWarning,
             match=r"ItemPipelineManager.open_spider\(\) is deprecated, use open_spider_async\(\) instead",
         ):
-            mwman.open_spider(DefaultSpider())
+            await maybe_deferred_to_future(mwman.open_spider(DefaultSpider()))
         with pytest.warns(
             ScrapyDeprecationWarning,
             match=r"ItemPipelineManager.close_spider\(\) is deprecated, use close_spider_async\(\) instead",
         ):
-            mwman.close_spider(DefaultSpider())
+            await maybe_deferred_to_future(mwman.close_spider(DefaultSpider()))
 
-    def test_deprecated_spider_arg_without_crawler(self) -> None:
+    @deferred_f_from_coro_f
+    async def test_deprecated_spider_arg_without_crawler(self) -> None:
         """The first instance passed to a deprecated method is used. Mismatched ones raise an error."""
         with pytest.warns(
             ScrapyDeprecationWarning,
@@ -504,7 +506,7 @@ class TestMiddlewareManagerSpider:
             ScrapyDeprecationWarning,
             match=r"ItemPipelineManager.open_spider\(\) is deprecated, use open_spider_async\(\) instead",
         ):
-            mwman.open_spider(spider)
+            await maybe_deferred_to_future(mwman.open_spider(spider))
         with (
             pytest.warns(
                 ScrapyDeprecationWarning,
@@ -514,12 +516,12 @@ class TestMiddlewareManagerSpider:
                 RuntimeError, match="Different instances of Spider were passed"
             ),
         ):
-            mwman.close_spider(DefaultSpider())
+            await maybe_deferred_to_future(mwman.close_spider(DefaultSpider()))
         with pytest.warns(
             ScrapyDeprecationWarning,
             match=r"ItemPipelineManager.close_spider\(\) is deprecated, use close_spider_async\(\) instead",
         ):
-            mwman.close_spider(spider)
+            await maybe_deferred_to_future(mwman.close_spider(spider))
 
     @deferred_f_from_coro_f
     async def test_no_spider_arg_without_crawler(self) -> None:

tests/test_utils_defer.py

@@ -321,9 +321,11 @@ class TestDeferredFFromCoroF:
 
         yield self._assert_result(c_f)
 
+    @pytest.mark.only_asyncio
     @inlineCallbacks
     def test_coroutine_asyncio(self):
         async def c_f() -> int:
+            await asyncio.sleep(0.01)
             return 42
 
         yield self._assert_result(c_f)