Improve test coverage for scrapy.extensions (#7809)

scrapy/extensions/closespider.py

@@ -119,7 +119,7 @@ class CloseSpider:
             self.task = None
 
         if self.task_no_item:
-            if self.task_no_item.running:
+            if self.task_no_item.running:  # pragma: no branch
                 self.task_no_item.stop()
             self.task_no_item = None
 

scrapy/extensions/feedexport.py

@@ -465,7 +465,7 @@ class FeedSlot:
         )
 
     def finish_exporting(self) -> None:
-        if self._exporting:
+        if self._exporting:  # pragma: no branch
             assert self.exporter
             self.exporter.finish_exporting()
             self._exporting = False
@@ -553,7 +553,7 @@ class FeedExporter:
         for slot in self.slots:
             self._schedule_slot_close(slot, spider)
 
-        if self._pending_close_tasks:
+        if self._pending_close_tasks:  # pragma: no branch
             if is_asyncio_available():
                 await asyncio.wait(
                     cast("list[asyncio.Task[None]]", list(self._pending_close_tasks))

scrapy/extensions/memusage.py

@@ -94,7 +94,7 @@ class MemoryUsage:
 
     def engine_stopped(self) -> None:
         for tsk in self.tasks:
-            if tsk.running:
+            if tsk.running:  # pragma: no branch
                 tsk.stop()
 
     def update(self) -> None:

scrapy/extensions/periodic_log.py

@@ -38,7 +38,6 @@ class PeriodicLog:
     ):
         self.stats: StatsCollector = stats
         self.interval: float = interval
-        self.multiplier: float = 60.0 / self.interval
         self.task: AsyncioLoopingCall | LoopingCall | None = None
         self.encoder: JSONEncoder = ScrapyJSONEncoder(sort_keys=True, indent=4)
         self.ext_stats_enabled: bool = bool(ext_stats)
@@ -165,5 +164,5 @@ class PeriodicLog:
 
     def spider_closed(self, spider: Spider, reason: str) -> None:
         self.log()
-        if self.task and self.task.running:
+        if self.task and self.task.running:  # pragma: no branch
             self.task.stop()

tests/test_downloadermiddleware_httpcache.py

@@ -14,6 +14,7 @@ import pytest
 
 from scrapy.downloadermiddlewares.httpcache import HttpCacheMiddleware
 from scrapy.exceptions import IgnoreRequest
+from scrapy.extensions.httpcache import DummyPolicy
 from scrapy.http import HtmlResponse, Request, Response
 from scrapy.spiders import Spider
 from scrapy.utils.test import get_crawler
@@ -24,6 +25,14 @@ if TYPE_CHECKING:
     from scrapy.crawler import Crawler
 
 
+class AlwaysStalePolicy(DummyPolicy):
+    """:class:`~scrapy.extensions.httpcache.DummyPolicy` that always
+    revalidates cached responses."""
+
+    def is_cached_response_fresh(self, cachedresponse, request):
+        return False
+
+
 class TestBase:
     """Base class with common setup and helper methods."""
 
@@ -282,6 +291,21 @@ class DummyPolicyTestMixin(PolicyTestMixin):
             self.assertEqualResponse(self.response, response)
             assert "cached" in response.flags
 
+    def test_revalidation_keeps_cached_response(self):
+        # The dummy policy considers every cached response valid, so a policy
+        # that subclasses it to force revalidation always gets the cached
+        # response back, whatever the new response is.
+        with self._middleware(HTTPCACHE_POLICY=AlwaysStalePolicy) as mw:
+            assert mw.process_request(self.request) is None
+            mw.process_response(self.request, self.response)
+
+            assert mw.process_request(self.request) is None
+            fresh_response = self.response.replace(body=b"new body")
+            response = mw.process_response(self.request, fresh_response)
+            self.assertEqualResponse(self.response, response)
+            assert "cached" in response.flags
+            assert mw.stats.get_value("httpcache/revalidate") == 1
+
 
 class RFC2616PolicyTestMixin(PolicyTestMixin):
     """Mixin containing RFC2616 policy specific test methods."""
@@ -553,6 +577,53 @@ class RFC2616PolicyTestMixin(PolicyTestMixin):
                 else:
                     assert "cached" in res5.flags
 
+    def test_middleware_ignore_schemes(self):
+        # file responses are not cached by default
+        req = Request("file:///tmp/t.txt")
+        res = Response(req.url, headers={"Expires": self.tomorrow})
+        with self._middleware() as mw:
+            assert mw.process_request(req) is None
+            mw.process_response(req, res)
+
+            assert mw.storage.retrieve_response(mw.crawler.spider, req) is None
+            assert mw.process_request(req) is None
+
+    def test_max_stale_with_value(self):
+        # A response that expired one day ago.
+        headers = {"Date": self.yesterday, "Expires": self.yesterday}
+        with self._middleware() as mw:
+            req0 = Request("http://example.com")
+            res0 = Response(req0.url, headers=headers)
+            self._process_requestresponse(mw, req0, res0)
+
+            # max-stale greater than the staleness of the cached response
+            req1 = req0.replace(headers={"Cache-Control": "max-stale=172800"})
+            res1 = mw.process_request(req1)
+            assert isinstance(res1, Response)
+            assert "cached" in res1.flags
+
+            # max-stale lower than the staleness of the cached response
+            req2 = req0.replace(headers={"Cache-Control": "max-stale=60"})
+            assert mw.process_request(req2) is None
+
+            # a non-integer max-stale value is ignored
+            req3 = req0.replace(headers={"Cache-Control": "max-stale=soon"})
+            assert mw.process_request(req3) is None
+
+    def test_response_dated_in_the_future(self):
+        # A Date header ahead of the local clock must not make the cached
+        # response look aged.
+        headers = {"Date": self.tomorrow, "Cache-Control": "max-age=10"}
+        with self._middleware() as mw:
+            req0 = Request("http://example.com")
+            res0 = Response(req0.url, headers=headers)
+            res1 = self._process_requestresponse(mw, req0, res0)
+            assert "cached" not in res1.flags
+
+            res2 = self._process_requestresponse(mw, req0, None)
+            self.assertEqualResponse(res1, res2)
+            assert "cached" in res2.flags
+
     def test_process_exception(self):
         with self._middleware() as mw:
             res0 = Response(self.request.url, headers={"Expires": self.yesterday})

tests/test_extension_memusage.py

@@ -2,6 +2,7 @@ from __future__ import annotations
 
 import logging
 import sys
+from typing import TYPE_CHECKING
 
 import pytest
 
@@ -13,8 +14,12 @@ from scrapy.extensions.memusage import MemoryUsage
 from scrapy.spiders import Spider
 from scrapy.utils.test import get_crawler
 from tests.utils import OneShotLoop
+from tests.utils.cmdline import proc
 from tests.utils.decorators import coroutine_test
 
+if TYPE_CHECKING:
+    from tests.mockserver.http import MockServer
+
 # MemoryUsage relies on the stdlib 'resource' module (not available on Windows)
 pytestmark = pytest.mark.skipif(
     sys.platform.startswith("win"),
@@ -25,6 +30,14 @@ pytestmark = pytest.mark.skipif(
 MB = 1024 * 1024
 
 
+class TwoShotLoop(OneShotLoop):
+    """Like :class:`OneShotLoop`, but runs the check twice."""
+
+    def start(self, interval: float, now: bool = True) -> None:
+        super().start(interval, now=now)
+        self.func()
+
+
 class _LoopSpider(Spider):
     name = "loop-data-spider"
 
@@ -50,6 +63,49 @@ def test_memusage_disabled() -> None:
         MemoryUsage.from_crawler(get_crawler(settings_dict=settings))
 
 
+def test_memusage_limit_stops_crawler_without_spider(mockserver: MockServer) -> None:
+    # The Scrapy shell starts the engine without opening a spider, so the
+    # whole crawler is stopped instead of a spider being closed.
+    _, out, err = proc(
+        "shell",
+        mockserver.url("/text"),
+        "-c",
+        "response.status",
+        "--set",
+        "MEMUSAGE_LIMIT_MB=1",
+    )
+    assert "Memory usage exceeded 1MiB" in err
+    assert "200" in out
+
+
+@coroutine_test
+async def test_memusage_below_thresholds_logs_peak(
+    caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    settings = {
+        "MEMUSAGE_LIMIT_MB": 100,
+        "MEMUSAGE_WARNING_MB": 50,
+        "MEMUSAGE_CHECK_INTERVAL_SECONDS": 0.01,
+        "TELNETCONSOLE_ENABLED": False,
+        "LOG_LEVEL": "INFO",
+    }
+
+    monkeypatch.setattr(memusage_mod, "create_looping_call", OneShotLoop)
+    monkeypatch.setattr(MemoryUsage, "get_virtual_size", lambda _: 25 * MB)
+
+    crawler = get_crawler(spidercls=_LoopSpider, settings_dict=settings)
+
+    with caplog.at_level(logging.INFO, logger="scrapy.extensions.memusage"):
+        await crawler.crawl_async(url="data:,", loops=1)
+
+    assert crawler.stats
+    assert crawler.stats.get_value("memusage/limit_reached") is None
+    assert crawler.stats.get_value("memusage/warning_reached") is None
+    assert crawler.stats.get_value("memusage/max") == 25 * MB
+    assert crawler.stats.get_value("finish_reason") == "finished"
+    assert any("Peak memory usage is 25MiB" in r.getMessage() for r in caplog.records)
+
+
 @coroutine_test
 async def test_memusage_limit_closes_spider_with_reason_and_error_log(
     caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch
@@ -92,8 +148,9 @@ async def test_memusage_warning_logs_but_allows_normal_finish(
         "LOG_LEVEL": "INFO",
     }
 
-    # Avoid background LoopingCall that can log after the test finishes.
-    monkeypatch.setattr(memusage_mod, "create_looping_call", OneShotLoop)
+    # Avoid background LoopingCall that can log after the test finishes; check
+    # twice, since the warning is only meant to be reported once.
+    monkeypatch.setattr(memusage_mod, "create_looping_call", TwoShotLoop)
     monkeypatch.setattr(MemoryUsage, "get_virtual_size", lambda self: 75 * MB)
 
     crawler = get_crawler(spidercls=_LoopSpider, settings_dict=settings)
@@ -112,4 +169,7 @@ async def test_memusage_warning_logs_but_allows_normal_finish(
     assert crawler.stats
     assert crawler.stats.get_value("memusage/warning_reached") == 1
     assert crawler.stats.get_value("finish_reason") == "finished"
-    assert any("memory usage reached" in r.getMessage().lower() for r in caplog.records)
+    warnings_logged = [
+        r for r in caplog.records if "memory usage reached" in r.getMessage().lower()
+    ]
+    assert len(warnings_logged) == 1

tests/test_extension_periodic_log.py

@@ -1,8 +1,13 @@
 from __future__ import annotations
 
 import datetime
+import json
+import logging
 from typing import TYPE_CHECKING, Any
 
+import pytest
+
+from scrapy.exceptions import NotConfigured
 from scrapy.extensions.periodic_log import PeriodicLog
 from scrapy.utils.test import get_crawler
 
@@ -86,6 +91,14 @@ class TestPeriodicLog:
         assert extension({"PERIODIC_LOG_DELTA": True, "LOGSTATS_INTERVAL": 60})
         assert extension({"PERIODIC_LOG_DELTA": "True", "LOGSTATS_INTERVAL": 60})
 
+    def test_no_interval(self):
+        with pytest.raises(NotConfigured):
+            extension({"PERIODIC_LOG_STATS": True, "LOGSTATS_INTERVAL": 0})
+
+    def test_nothing_enabled(self):
+        with pytest.raises(NotConfigured):
+            extension({"LOGSTATS_INTERVAL": 60})
+
     @coroutine_test
     async def test_log_delta(self):
         def emulate(
@@ -212,3 +225,26 @@ class TestPeriodicLog:
             {"PERIODIC_LOG_STATS": {"include": ["downloader/"], "exclude": ["bytes"]}},
             lambda k, v: "downloader/" in k and "bytes" not in k,
         )
+
+    @coroutine_test
+    async def test_log_timing(self, caplog: pytest.LogCaptureFixture) -> None:
+        settings = {
+            "EXTENSIONS": {"scrapy.extensions.periodic_log.PeriodicLog": 0},
+            "PERIODIC_LOG_TIMING_ENABLED": True,
+            "LOGSTATS_INTERVAL": 30,
+        }
+        crawler = get_crawler(MetaSpider, settings)
+        with caplog.at_level(logging.INFO, logger="scrapy.extensions.periodic_log"):
+            await crawler.crawl_async()
+
+        records = [
+            r for r in caplog.records if r.name == "scrapy.extensions.periodic_log"
+        ]
+        assert records, "PeriodicLog logged nothing"
+        # Only the timing section is enabled, and it is logged on spider close.
+        data = json.loads(records[-1].getMessage())
+        assert list(data) == ["time"]
+        assert data["time"]["log_interval"] == 30
+        assert data["time"]["log_interval_real"] >= 0
+        assert data["time"]["elapsed"] >= 0
+        assert data["time"]["start_time"] <= data["time"]["utcnow"]

tests/test_extension_telnet.py

@@ -7,7 +7,8 @@ import pytest
 from twisted.conch.telnet import ITelnetProtocol
 from twisted.cred import credentials
 
-from scrapy.extensions.telnet import TelnetConsole
+from scrapy import Spider
+from scrapy.extensions.telnet import TelnetConsole, update_telnet_vars
 from scrapy.utils.defer import maybe_deferred_to_future
 from scrapy.utils.test import get_crawler
 from tests.utils.decorators import coroutine_test
@@ -16,16 +17,20 @@ if TYPE_CHECKING:
     from collections.abc import Generator
 
     from scrapy.crawler import Crawler
+    from scrapy.http import Response
 
 pytestmark = pytest.mark.requires_reactor  # TelnetConsole requires a reactor
 
 
-def _get_crawler(settings_dict: dict[str, Any] | None = None) -> Crawler:
+def _get_crawler(
+    spidercls: type[Spider] | None = None,
+    settings_dict: dict[str, Any] | None = None,
+) -> Crawler:
     settings = {
         "TELNETCONSOLE_ENABLED": True,
         **(settings_dict or {}),
     }
-    return get_crawler(settings_dict=settings)
+    return get_crawler(spidercls, settings_dict=settings)
 
 
 @contextmanager
@@ -84,3 +89,47 @@ def test_invalid_reversed_portrange() -> None:
     console = TelnetConsole(_get_crawler(settings_dict=settings))
     with pytest.raises(ValueError, match=r"invalid portrange: \[2, 1\]"):
         console.start_listening()
+
+
+@coroutine_test
+async def test_telnet_vars() -> None:
+    """Log into the console of a running crawl, which is when the telnet
+    variables are built."""
+    received: list[dict[str, Any]] = []
+
+    def on_update_telnet_vars(telnet_vars: dict[str, Any]) -> None:
+        received.append(telnet_vars)
+
+    class TelnetSpider(Spider):
+        name = "telnet"
+        start_urls = ["data:,"]
+
+        async def parse(self, response: Response) -> None:
+            assert self.crawler.extensions
+            console = next(
+                ext
+                for ext in self.crawler.extensions.middlewares
+                if isinstance(ext, TelnetConsole)
+            )
+            creds = credentials.UsernamePassword(
+                console.username.encode("utf8"), console.password.encode("utf8")
+            )
+            portal = console.protocol().protocolArgs[0]
+            await maybe_deferred_to_future(portal.login(creds, None, ITelnetProtocol))
+
+    crawler = _get_crawler(TelnetSpider)
+    crawler.signals.connect(on_update_telnet_vars, signal=update_telnet_vars)
+    await crawler.crawl_async()
+
+    assert len(received) == 1
+    telnet_vars = received[0]
+    assert telnet_vars["crawler"] is crawler
+    assert telnet_vars["engine"] is crawler.engine
+    assert telnet_vars["spider"] is crawler.spider
+    assert telnet_vars["extensions"] is crawler.extensions
+    assert telnet_vars["stats"] is crawler.stats
+    assert telnet_vars["settings"] is crawler.settings
+    assert callable(telnet_vars["est"])
+    assert callable(telnet_vars["p"])
+    assert callable(telnet_vars["prefs"])
+    assert "telnetconsole.html" in telnet_vars["help"]

tests/test_feedexport.py

@@ -24,6 +24,7 @@ from scrapy.extensions.feedexport import (
     FeedExporter,
     FeedSlot,
     FileFeedStorage,
+    ItemFilter,
     apply_uri_params,
 )
 from scrapy.utils.python import to_unicode
@@ -1289,6 +1290,13 @@ class TestFeedExporterSignals:
         assert self.feed_exporter_closed_received
 
 
+class TestItemFilter:
+    def test_no_feed_options(self):
+        item_filter = ItemFilter(None)
+        assert item_filter.item_classes == ()
+        assert item_filter.accepts(MyItem({"foo": "bar"}))
+
+
 class TestFeedExportInit:
     def test_unsupported_storage(self):
         settings = {
@@ -1300,6 +1308,24 @@ class TestFeedExportInit:
         with pytest.raises(NotConfigured):
             FeedExporter.from_crawler(crawler)
 
+    def test_disabled_storage(self, caplog: pytest.LogCaptureFixture):
+        class DisabledFeedStorage:
+            def __init__(self, uri, *, feed_options=None):
+                raise NotConfigured("not today")
+
+        settings = {
+            "FEED_STORAGES": {"disabled": DisabledFeedStorage},
+            "FEEDS": {
+                "disabled://uri": {},
+            },
+        }
+        crawler = get_crawler(settings_dict=settings)
+        with caplog.at_level(logging.ERROR), pytest.raises(NotConfigured):
+            FeedExporter.from_crawler(crawler)
+        assert (
+            "Disabled feed storage scheme: disabled. Reason: not today" in caplog.text
+        )
+
     def test_unsupported_format(self):
         settings = {
             "FEEDS": {

tests/test_feedexport_postprocess.py

@@ -12,6 +12,7 @@ from typing import TYPE_CHECKING, Any
 
 import pytest
 
+from scrapy.extensions.postprocessing import PostProcessingManager
 from scrapy.utils.test import get_crawler
 from tests.utils.bases.feedexport import TestFeedExportBase
 from tests.utils.decorators import coroutine_test
@@ -87,6 +88,15 @@ class TestFeedPostProcessedExports(TestFeedExportBase):
         data_stream.seek(0)
         return data_stream.read()
 
+    def test_tell_reports_target_file_position(self):
+        """Exporters that wrap the file they get, e.g. through
+        :class:`io.TextIOWrapper`, need it to report a position."""
+        file = BytesIO()
+        manager = PostProcessingManager([self.MyPlugin1], file, {})
+        assert manager.tell() == 0
+        manager.write(b"foo")
+        assert manager.tell() == file.tell() == 3
+
     @coroutine_test
     async def test_gzip_plugin(self):
         filename = self._named_tempfile("gzip_file")

tests/test_feedexport_storages.py

@@ -3,6 +3,7 @@ from __future__ import annotations
 import logging
 import os
 import string
+import sys
 import tempfile
 from io import BytesIO
 from pathlib import Path
@@ -14,6 +15,7 @@ import pytest
 from w3lib.url import path_to_file_uri
 
 import scrapy
+from scrapy.exceptions import NotConfigured
 from scrapy.extensions.feedexport import (
     BlockingFeedStorage,
     FileFeedStorage,
@@ -166,6 +168,12 @@ class TestFTPFeedStorage:
         st = FTPFeedStorage(f"ftp://foo:{pw_quoted}@example.com/some_path", {})
         assert st.password == string.punctuation
 
+    def test_uri_without_hostname(self):
+        with pytest.raises(
+            ValueError, match="Got a storage URI without a hostname: ftp:///some_path"
+        ):
+            FTPFeedStorage("ftp:///some_path")
+
 
 class MyBlockingFeedStorage(BlockingFeedStorage):
     def _store_in_thread(self, file: IO[bytes]) -> None:
@@ -205,6 +213,13 @@ class TestBlockingFeedStorage:
             b.open(spider=spider)
 
 
+def test_s3_without_boto3(monkeypatch: pytest.MonkeyPatch) -> None:
+    monkeypatch.setitem(sys.modules, "boto3", None)
+    monkeypatch.setitem(sys.modules, "boto3.session", None)
+    with pytest.raises(NotConfigured, match="missing boto3 library"):
+        S3FeedStorage("s3://mybucket/export.csv", "access_key", "secret_key")
+
+
 @pytest.mark.requires_boto3
 class TestS3FeedStorage:
     def test_parse_credentials(self):