Improve test coverage for crawler.py (#7682)

* Improve test coverage for crawler.py

* Silence mypy warnings

* Improve test coverage for crawler.py

tests/AsyncCrawlerProcess/asyncio_enabled_reactor_same_loop_default.py

@@ -0,0 +1,31 @@
+import asyncio
+import sys
+
+from twisted.internet import asyncioreactor
+
+import scrapy
+from scrapy.crawler import AsyncCrawlerProcess
+
+if sys.platform == "win32":
+    asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
+loop = asyncio.SelectorEventLoop()
+asyncio.set_event_loop(loop)
+asyncioreactor.install(loop)
+
+
+class NoRequestsSpider(scrapy.Spider):
+    name = "no_request"
+
+    async def start(self):
+        return
+        yield
+
+
+process = AsyncCrawlerProcess(
+    settings={
+        "TWISTED_REACTOR": "twisted.internet.asyncioreactor.AsyncioSelectorReactor",
+        "ASYNCIO_EVENT_LOOP": "asyncio.SelectorEventLoop",
+    }
+)
+process.crawl(NoRequestsSpider)
+process.start()

tests/AsyncCrawlerProcess/dns_resolver_deprecated.py

@@ -0,0 +1,31 @@
+import sys
+
+import scrapy
+from scrapy.crawler import AsyncCrawlerProcess
+from scrapy.settings import Settings
+
+
+class NoRequestsSpider(scrapy.Spider):
+    name = "no_request"
+
+    async def start(self):
+        return
+        yield
+
+
+settings = Settings()
+# The deprecated DNS_RESOLVER setting, set above its default priority so that
+# AsyncCrawlerProcess._setup_reactor() emits the deprecation warning.
+settings.set("DNS_RESOLVER", "scrapy.resolver.CachingThreadedResolver", priority=10)
+if len(sys.argv) > 1 and sys.argv[1] == "twisted-wins":
+    # TWISTED_DNS_RESOLVER at a higher priority takes precedence over the
+    # deprecated DNS_RESOLVER setting.
+    settings.set(
+        "TWISTED_DNS_RESOLVER",
+        "scrapy.resolver.CachingThreadedResolver",
+        priority=20,
+    )
+
+process = AsyncCrawlerProcess(settings)
+process.crawl(NoRequestsSpider)
+process.start()

tests/AsyncCrawlerProcess/reactorless_sleeping.py

@@ -17,4 +17,4 @@ class SleepingSpider(scrapy.Spider):
 process = AsyncCrawlerProcess(settings={"TWISTED_REACTOR_ENABLED": False})
 
 process.crawl(SleepingSpider)
-process.start()
+process.start(stop_after_crawl="--no-stop" not in sys.argv)

tests/AsyncCrawlerProcess/sleeping.py

@@ -17,4 +17,4 @@ class SleepingSpider(scrapy.Spider):
 process = AsyncCrawlerProcess(settings={})
 
 process.crawl(SleepingSpider)
-process.start()
+process.start(stop_after_crawl="--no-stop" not in sys.argv)

tests/CrawlerProcess/dns_resolver_deprecated.py

@@ -0,0 +1,31 @@
+import sys
+
+import scrapy
+from scrapy.crawler import CrawlerProcess
+from scrapy.settings import Settings
+
+
+class NoRequestsSpider(scrapy.Spider):
+    name = "no_request"
+
+    async def start(self):
+        return
+        yield
+
+
+settings = Settings()
+# The deprecated DNS_RESOLVER setting, set above its default priority so that
+# CrawlerProcess._setup_reactor() emits the deprecation warning.
+settings.set("DNS_RESOLVER", "scrapy.resolver.CachingThreadedResolver", priority=10)
+if len(sys.argv) > 1 and sys.argv[1] == "twisted-wins":
+    # TWISTED_DNS_RESOLVER at a higher priority takes precedence over the
+    # deprecated DNS_RESOLVER setting.
+    settings.set(
+        "TWISTED_DNS_RESOLVER",
+        "scrapy.resolver.CachingThreadedResolver",
+        priority=20,
+    )
+
+process = CrawlerProcess(settings)
+process.crawl(NoRequestsSpider)
+process.start()

tests/CrawlerProcess/sleeping.py

@@ -23,4 +23,4 @@ class SleepingSpider(scrapy.Spider):
 process = CrawlerProcess(settings={})
 
 process.crawl(SleepingSpider)
-process.start()
+process.start(stop_after_crawl="--no-stop" not in sys.argv)

tests/test_crawler.py

@@ -3,8 +3,11 @@ from __future__ import annotations
 import asyncio
 import logging
 import re
+import signal
+import threading
 from pathlib import Path
-from typing import Any, ClassVar
+from typing import TYPE_CHECKING, Any, ClassVar
+from unittest.mock import MagicMock
 
 import pytest
 from zope.interface.exceptions import MultipleInvalid
@@ -32,6 +35,9 @@ from scrapy.utils.spider import DefaultSpider
 from scrapy.utils.test import get_crawler, get_reactor_settings
 from tests.utils.decorators import coroutine_test
 
+if TYPE_CHECKING:
+    from collections.abc import Callable
+
 BASE_SETTINGS: dict[str, Any] = {}
 
 
@@ -651,6 +657,145 @@ class TestAsyncCrawlerProcess(TestBaseCrawler):
         self.assertOptionIsDefault(runner.settings, "RETRY_ENABLED")
 
 
+class TestAsyncCrawlerProcessReactorlessHelpers:
+    """Unit tests for the reactorless shutdown helpers of AsyncCrawlerProcess.
+
+    These cover defensive branches that guard against shutdown races and that
+    are not reachable through a full process run.
+    """
+
+    @staticmethod
+    def _bare_process(
+        monkeypatch: pytest.MonkeyPatch,
+    ) -> tuple[AsyncCrawlerProcess, list[Any]]:
+        # AsyncCrawlerProcess.__init__ has global side effects (it installs a
+        # reactor import hook and an asyncio event loop), so build a bare
+        # instance and set only the attributes these helpers read. The shutdown
+        # handlers installed by these helpers are recorded for assertions
+        # instead of touching the real process-wide signal handlers.
+        installed_handlers: list[Any] = []
+        monkeypatch.setattr(
+            "scrapy.crawler.install_shutdown_handlers",
+            lambda handler, *args, **kwargs: installed_handlers.append(handler),
+        )
+        return AsyncCrawlerProcess.__new__(AsyncCrawlerProcess), installed_handlers
+
+    @staticmethod
+    def _run_in_thread(target: Callable[[], None]) -> None:
+        # Run target in a dedicated thread so its event loop is not nested
+        # inside the event loop that may already be running the test session.
+        thread = threading.Thread(target=target)
+        thread.start()
+        thread.join()
+
+    def test_signal_shutdown_reactorless_without_loop(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, installed_handlers = self._bare_process(monkeypatch)
+        process._reactorless_loop = None
+        # No loop to schedule the shutdown task on, so it returns early, but it
+        # must still escalate the handler so a second signal forces a kill.
+        process._signal_shutdown_reactorless(signal.SIGINT, None)
+        assert installed_handlers == [process._signal_kill_reactorless]
+
+    def test_signal_kill_reactorless_without_loop(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, installed_handlers = self._bare_process(monkeypatch)
+        process._reactorless_loop = None
+        process._reactorless_main_task = None
+        # No loop to cancel the main task on, so it returns early, but it must
+        # still ignore any further signals.
+        process._signal_kill_reactorless(signal.SIGINT, None)
+        assert installed_handlers == [signal.SIG_IGN]
+
+    def test_signal_kill_reactorless_without_main_task(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, installed_handlers = self._bare_process(monkeypatch)
+        loop = MagicMock()
+        process._reactorless_loop = loop
+        process._reactorless_main_task = None
+        # No main task to cancel, so nothing is scheduled on the loop.
+        process._signal_kill_reactorless(signal.SIGINT, None)
+        assert installed_handlers == [signal.SIG_IGN]
+        loop.call_soon_threadsafe.assert_not_called()
+
+    def test_shutdown_graceful_reactorless_main_task_already_done(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, _ = self._bare_process(monkeypatch)
+        process._stop_after_crawl = False
+
+        async def noop() -> None:
+            return None
+
+        monkeypatch.setattr(process, "stop", noop)
+        monkeypatch.setattr(process, "join", noop)
+
+        def run() -> None:
+            loop = asyncio.new_event_loop()
+            try:
+                main_task: asyncio.Future[None] = loop.create_future()
+                main_task.set_result(None)
+                process._reactorless_main_task = main_task
+                # The main task is already done, so it is not cancelled.
+                loop.run_until_complete(process._shutdown_graceful_reactorless())
+                assert not main_task.cancelled()
+            finally:
+                loop.close()
+
+        self._run_in_thread(run)
+
+    def test_create_shutdown_task_closed_loop(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, _ = self._bare_process(monkeypatch)
+        loop = asyncio.new_event_loop()
+        loop.close()
+        process._reactorless_loop = loop
+        process._stop_after_crawl = True
+        # create_task() raises RuntimeError on a closed loop; the coroutine
+        # must be closed instead of leaking.
+        process._create_shutdown_task()
+
+    def test_cancel_all_tasks_logs_task_exception(self) -> None:
+        contexts: list[dict[str, Any]] = []
+        task_was_cancelled: list[bool] = []
+
+        def run() -> None:
+            loop = asyncio.new_event_loop()
+            loop.set_exception_handler(lambda _loop, context: contexts.append(context))
+
+            async def fail_on_cancel() -> None:
+                try:
+                    await asyncio.sleep(10)
+                except asyncio.CancelledError:
+                    raise RuntimeError("boom")
+
+            try:
+                task = loop.create_task(fail_on_cancel())
+                # Let the task start and suspend on the sleep so the
+                # cancellation is raised inside its body and turned into a
+                # RuntimeError rather than cancelling the task cleanly.
+                loop.run_until_complete(asyncio.sleep(0))
+                AsyncCrawlerProcess._cancel_all_tasks(loop)
+                task_was_cancelled.append(task.cancelled())
+            finally:
+                loop.close()
+
+        self._run_in_thread(run)
+
+        # The task raised instead of being cancelled, so its exception is
+        # reported to the loop exception handler.
+        assert task_was_cancelled == [False]
+        assert any(
+            context.get("message")
+            == "unhandled exception during AsyncCrawlerProcess shutdown"
+            for context in contexts
+        )
+
+
 @pytest.mark.parametrize("runner_cls", [AsyncCrawlerRunner, CrawlerRunner])
 def test_runner_settings_applied_to_crawler_instance(
     runner_cls: type[CrawlerRunnerBase],
@@ -687,6 +832,22 @@ def test_create_crawler_instance_consistent_with_spider_class() -> None:
     assert pre_built.settings["FOO"] == "runner"
 
 
+@pytest.mark.parametrize("runner_cls", [AsyncCrawlerRunner, CrawlerRunner])
+def test_create_crawler_rejects_spider_object(
+    runner_cls: type[CrawlerRunnerBase],
+) -> None:
+    runner = runner_cls()
+    with pytest.raises(ValueError, match="cannot be a spider object"):
+        runner.create_crawler(DefaultSpider())  # type: ignore[arg-type]
+
+
+@pytest.mark.parametrize("runner_cls", [AsyncCrawlerRunner, CrawlerRunner])
+def test_crawl_rejects_spider_object(runner_cls: type[CrawlerRunnerBase]) -> None:
+    runner = runner_cls()
+    with pytest.raises(ValueError, match="cannot be a spider object"):
+        runner.crawl(DefaultSpider())  # type: ignore[arg-type]
+
+
 class ExceptionSpider(scrapy.Spider):
     name = "exception"
 

tests/test_crawler_subprocess.py

@@ -126,6 +126,16 @@ class TestCrawlerProcessSubprocessBase(ScriptRunnerMixin):
         assert "TimeoutError" not in log
         assert "scrapy.exceptions.CannotResolveHostError" not in log
 
+    def test_dns_resolver_deprecated(self) -> None:
+        log = self.run_script("dns_resolver_deprecated.py")
+        assert "Spider closed (finished)" in log
+        assert "The DNS_RESOLVER setting is deprecated" in log
+
+    def test_dns_resolver_deprecated_twisted_dns_resolver(self) -> None:
+        log = self.run_script("dns_resolver_deprecated.py", "twisted-wins")
+        assert "Spider closed (finished)" in log
+        assert "The DNS_RESOLVER setting is deprecated" in log
+
     def test_twisted_reactor_asyncio(self) -> None:
         log = self.run_script("twisted_reactor_asyncio.py")
         assert "Spider closed (finished)" in log
@@ -205,9 +215,11 @@ class TestCrawlerProcessSubprocessBase(ScriptRunnerMixin):
         assert "Spider closed (finished)" in log
         assert "The value of FOO is 42" in log
 
-    def _test_shutdown_graceful(self, script: str = "sleeping.py") -> None:
+    def _test_shutdown_graceful(
+        self, script: str = "sleeping.py", *extra_args: str
+    ) -> None:
         sig = signal.SIGINT if sys.platform != "win32" else signal.SIGBREAK  # type: ignore[attr-defined]
-        args = self.get_script_args(script, "3")
+        args = self.get_script_args(script, "3", *extra_args)
         p = PopenSpawn(args, timeout=5, env=get_script_run_env())
         p.expect_exact("Spider opened")
         p.expect_exact("Crawled (200)")
@@ -245,6 +257,9 @@ class TestCrawlerProcessSubprocessBase(ScriptRunnerMixin):
     async def test_shutdown_forced(self) -> None:
         await self._test_shutdown_forced()
 
+    def test_shutdown_graceful_no_stop(self) -> None:
+        self._test_shutdown_graceful("sleeping.py", "--no-stop")
+
 
 class TestCrawlerProcessSubprocess(TestCrawlerProcessSubprocessBase):
     @property
@@ -429,6 +444,17 @@ class TestAsyncCrawlerProcessSubprocess(TestCrawlerProcessSubprocessBase):
     async def test_shutdown_forced(self) -> None:
         await self._test_shutdown_forced("reactorless_sleeping.py")
 
+    def test_shutdown_graceful_reactorless_no_stop(self) -> None:
+        self._test_shutdown_graceful("reactorless_sleeping.py", "--no-stop")
+
+    def test_asyncio_enabled_reactor_same_loop_default(self) -> None:
+        log = self.run_script("asyncio_enabled_reactor_same_loop_default.py")
+        assert "Spider closed (finished)" in log
+        assert (
+            "Using reactor: twisted.internet.asyncioreactor.AsyncioSelectorReactor"
+            in log
+        )
+
 
 class TestCrawlerRunnerSubprocessBase(ScriptRunnerMixin):
     """Common tests between CrawlerRunner and AsyncCrawlerRunner,