From 361f689df785959a59cf939b9efaefc72079f037 Mon Sep 17 00:00:00 2001 From: Adrian Date: Wed, 1 Jul 2026 08:53:07 +0200 Subject: [PATCH] Improve test coverage for crawler.py (#7682) * Improve test coverage for crawler.py * Silence mypy warnings * Improve test coverage for crawler.py --- ...yncio_enabled_reactor_same_loop_default.py | 31 ++++ .../dns_resolver_deprecated.py | 31 ++++ .../reactorless_sleeping.py | 2 +- tests/AsyncCrawlerProcess/sleeping.py | 2 +- .../CrawlerProcess/dns_resolver_deprecated.py | 31 ++++ tests/CrawlerProcess/sleeping.py | 2 +- tests/test_crawler.py | 163 +++++++++++++++++- tests/test_crawler_subprocess.py | 30 +++- 8 files changed, 286 insertions(+), 6 deletions(-) create mode 100644 tests/AsyncCrawlerProcess/asyncio_enabled_reactor_same_loop_default.py create mode 100644 tests/AsyncCrawlerProcess/dns_resolver_deprecated.py create mode 100644 tests/CrawlerProcess/dns_resolver_deprecated.py diff --git a/tests/AsyncCrawlerProcess/asyncio_enabled_reactor_same_loop_default.py b/tests/AsyncCrawlerProcess/asyncio_enabled_reactor_same_loop_default.py new file mode 100644 index 000000000..c519e123b --- /dev/null +++ b/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() diff --git a/tests/AsyncCrawlerProcess/dns_resolver_deprecated.py b/tests/AsyncCrawlerProcess/dns_resolver_deprecated.py new file mode 100644 index 000000000..8c8df96bb --- /dev/null +++ b/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() diff --git a/tests/AsyncCrawlerProcess/reactorless_sleeping.py b/tests/AsyncCrawlerProcess/reactorless_sleeping.py index 12101d221..0d11a0996 100644 --- a/tests/AsyncCrawlerProcess/reactorless_sleeping.py +++ b/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) diff --git a/tests/AsyncCrawlerProcess/sleeping.py b/tests/AsyncCrawlerProcess/sleeping.py index 88caf5032..dad6a3f20 100644 --- a/tests/AsyncCrawlerProcess/sleeping.py +++ b/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) diff --git a/tests/CrawlerProcess/dns_resolver_deprecated.py b/tests/CrawlerProcess/dns_resolver_deprecated.py new file mode 100644 index 000000000..b8cd24325 --- /dev/null +++ b/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() diff --git a/tests/CrawlerProcess/sleeping.py b/tests/CrawlerProcess/sleeping.py index cb8f869e1..a577b1909 100644 --- a/tests/CrawlerProcess/sleeping.py +++ b/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) diff --git a/tests/test_crawler.py b/tests/test_crawler.py index 0cddfd0ed..adac32df1 100644 --- a/tests/test_crawler.py +++ b/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" diff --git a/tests/test_crawler_subprocess.py b/tests/test_crawler_subprocess.py index e3f9ac161..018a2b31b 100644 --- a/tests/test_crawler_subprocess.py +++ b/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,