Merge remote-tracking branch 'scrapy/master' into minimal-async-seeds

.github/workflows/tests-macos.yml

@@ -33,3 +33,7 @@ jobs:
 
     - name: Upload coverage report
       uses: codecov/codecov-action@v5
+
+    - name: Upload test results
+      if: ${{ !cancelled() }}
+      uses: codecov/test-results-action@v1

.github/workflows/tests-ubuntu.yml

@@ -88,3 +88,7 @@ jobs:
 
     - name: Upload coverage report
       uses: codecov/codecov-action@v5
+
+    - name: Upload test results
+      if: ${{ !cancelled() }}
+      uses: codecov/test-results-action@v1

.github/workflows/tests-windows.yml

@@ -64,3 +64,7 @@ jobs:
 
     - name: Upload coverage report
       uses: codecov/codecov-action@v5
+
+    - name: Upload test results
+      if: ${{ !cancelled() }}
+      uses: codecov/test-results-action@v1

.gitignore

@@ -15,6 +15,7 @@ htmlcov/
 .pytest_cache/
 .coverage.*
 coverage.*
+*.junit.xml
 test-output.*
 .cache/
 .mypy_cache/

docs/topics/settings.rst

@@ -2047,6 +2047,21 @@ also used by :class:`~scrapy.downloadermiddlewares.robotstxt.RobotsTxtMiddleware
 if :setting:`ROBOTSTXT_USER_AGENT` setting is ``None`` and
 there is no overriding User-Agent header specified for the request.
 
+.. setting:: WARN_ON_GENERATOR_RETURN_VALUE
+
+WARN_ON_GENERATOR_RETURN_VALUE
+------------------------------
+
+Default: ``True``
+
+When enabled, Scrapy will warn if generator-based callback methods (like
+``parse``) contain return statements with non-``None`` values. This helps detect
+potential mistakes in spider development.
+
+Disable this setting to prevent syntax errors that may occur when dynamically
+modifying generator function source code during runtime, skip AST parsing of
+callback functions, or improve performance in auto-reloading development
+environments.
 
 Settings documented elsewhere:
 ------------------------------

scrapy/core/scraper.py

@@ -246,6 +246,7 @@ class Scraper:
             spider=spider,
         )
         assert self.crawler.stats
+        self.crawler.stats.inc_value("spider_exceptions/count", spider=spider)
         self.crawler.stats.inc_value(
             f"spider_exceptions/{_failure.value.__class__.__name__}", spider=spider
         )

scrapy/settings/default_settings.py

@@ -351,3 +351,5 @@ SPIDER_CONTRACTS_BASE = {
     "scrapy.contracts.default.ReturnsContract": 2,
     "scrapy.contracts.default.ScrapesContract": 3,
 }
+
+WARN_ON_GENERATOR_RETURN_VALUE = True

scrapy/utils/misc.py

@@ -286,6 +286,8 @@ def warn_on_generator_with_return_value(
     Logs a warning if a callable is a generator function and includes
     a 'return' statement with a value different than None
     """
+    if not spider.settings.getbool("WARN_ON_GENERATOR_RETURN_VALUE"):
+        return
     try:
         if is_generator_with_return_value(callable):
             warnings.warn(

tests/test_closespider.py

@@ -88,6 +88,7 @@ class TestCloseSpider(TestCase):
         assert reason == "closespider_errorcount"
         key = f"spider_exceptions/{crawler.spider.exception_cls.__name__}"
         errorcount = crawler.stats.get_value(key)
+        assert crawler.stats.get_value("spider_exceptions/count") >= close_on
         assert errorcount >= close_on
 
     @defer.inlineCallbacks

tests/test_downloadermiddleware_robotstxt.py

@@ -1,9 +1,11 @@
-from typing import Any
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
 from unittest import mock
 
 import pytest
 from twisted.internet import error, reactor
-from twisted.internet.defer import Deferred, DeferredList, maybeDeferred
+from twisted.internet.defer import Deferred, maybeDeferred
 from twisted.python import failure
 from twisted.trial import unittest
 
@@ -13,8 +15,12 @@ from scrapy.exceptions import IgnoreRequest, NotConfigured
 from scrapy.http import Request, Response, TextResponse
 from scrapy.http.request import NO_CALLBACK
 from scrapy.settings import Settings
+from scrapy.utils.defer import deferred_f_from_coro_f, maybe_deferred_to_future
 from tests.test_robotstxt_interface import rerp_available
 
+if TYPE_CHECKING:
+    from scrapy.crawler import Crawler
+
 
 class TestRobotsTxtMiddleware(unittest.TestCase):
     def setUp(self):
@@ -31,7 +37,7 @@ class TestRobotsTxtMiddleware(unittest.TestCase):
         with pytest.raises(NotConfigured):
             RobotsTxtMiddleware(self.crawler)
 
-    def _get_successful_crawler(self):
+    def _get_successful_crawler(self) -> Crawler:
         crawler = self.crawler
         crawler.settings.set("ROBOTSTXT_OBEY", True)
         ROBOTS = """
@@ -54,54 +60,41 @@ Disallow: /some/randome/page.html
         crawler.engine.download.side_effect = return_response
         return crawler
 
-    def test_robotstxt(self):
+    @deferred_f_from_coro_f
+    async def test_robotstxt(self):
         middleware = RobotsTxtMiddleware(self._get_successful_crawler())
-        return DeferredList(
-            [
-                self.assertNotIgnored(Request("http://site.local/allowed"), middleware),
-                maybeDeferred(self.assertRobotsTxtRequested, "http://site.local"),
-                self.assertIgnored(Request("http://site.local/admin/main"), middleware),
-                self.assertIgnored(Request("http://site.local/static/"), middleware),
-                self.assertIgnored(
-                    Request("http://site.local/wiki/K%C3%A4ytt%C3%A4j%C3%A4:"),
-                    middleware,
-                ),
-                self.assertIgnored(
-                    Request("http://site.local/wiki/Käyttäjä:"), middleware
-                ),
-            ],
-            fireOnOneErrback=True,
+        await self.assertNotIgnored(Request("http://site.local/allowed"), middleware)
+        self.assertRobotsTxtRequested("http://site.local")
+        await self.assertIgnored(Request("http://site.local/admin/main"), middleware)
+        await self.assertIgnored(Request("http://site.local/static/"), middleware)
+        await self.assertIgnored(
+            Request("http://site.local/wiki/K%C3%A4ytt%C3%A4j%C3%A4:"), middleware
+        )
+        await self.assertIgnored(
+            Request("http://site.local/wiki/Käyttäjä:"), middleware
         )
 
-    def test_robotstxt_ready_parser(self):
+    @deferred_f_from_coro_f
+    async def test_robotstxt_ready_parser(self):
         middleware = RobotsTxtMiddleware(self._get_successful_crawler())
-        d = self.assertNotIgnored(Request("http://site.local/allowed"), middleware)
-        d.addCallback(
-            lambda _: self.assertNotIgnored(
-                Request("http://site.local/allowed"), middleware
-            )
-        )
-        return d
+        await self.assertNotIgnored(Request("http://site.local/allowed"), middleware)
+        await self.assertNotIgnored(Request("http://site.local/allowed"), middleware)
 
-    def test_robotstxt_meta(self):
+    @deferred_f_from_coro_f
+    async def test_robotstxt_meta(self):
         middleware = RobotsTxtMiddleware(self._get_successful_crawler())
         meta = {"dont_obey_robotstxt": True}
-        return DeferredList(
-            [
-                self.assertNotIgnored(
-                    Request("http://site.local/allowed", meta=meta), middleware
-                ),
-                self.assertNotIgnored(
-                    Request("http://site.local/admin/main", meta=meta), middleware
-                ),
-                self.assertNotIgnored(
-                    Request("http://site.local/static/", meta=meta), middleware
-                ),
-            ],
-            fireOnOneErrback=True,
+        await self.assertNotIgnored(
+            Request("http://site.local/allowed", meta=meta), middleware
+        )
+        await self.assertNotIgnored(
+            Request("http://site.local/admin/main", meta=meta), middleware
+        )
+        await self.assertNotIgnored(
+            Request("http://site.local/static/", meta=meta), middleware
         )
 
-    def _get_garbage_crawler(self):
+    def _get_garbage_crawler(self) -> Crawler:
         crawler = self.crawler
         crawler.settings.set("ROBOTSTXT_OBEY", True)
         response = Response(
@@ -116,22 +109,16 @@ Disallow: /some/randome/page.html
         crawler.engine.download.side_effect = return_response
         return crawler
 
-    def test_robotstxt_garbage(self):
+    @deferred_f_from_coro_f
+    async def test_robotstxt_garbage(self):
         # garbage response should be discarded, equal 'allow all'
         middleware = RobotsTxtMiddleware(self._get_garbage_crawler())
-        return DeferredList(
-            [
-                self.assertNotIgnored(Request("http://site.local"), middleware),
-                self.assertNotIgnored(Request("http://site.local/allowed"), middleware),
-                self.assertNotIgnored(
-                    Request("http://site.local/admin/main"), middleware
-                ),
-                self.assertNotIgnored(Request("http://site.local/static/"), middleware),
-            ],
-            fireOnOneErrback=True,
-        )
+        await self.assertNotIgnored(Request("http://site.local"), middleware)
+        await self.assertNotIgnored(Request("http://site.local/allowed"), middleware)
+        await self.assertNotIgnored(Request("http://site.local/admin/main"), middleware)
+        await self.assertNotIgnored(Request("http://site.local/static/"), middleware)
 
-    def _get_emptybody_crawler(self):
+    def _get_emptybody_crawler(self) -> Crawler:
         crawler = self.crawler
         crawler.settings.set("ROBOTSTXT_OBEY", True)
         response = Response("http://site.local/robots.txt")
@@ -144,21 +131,16 @@ Disallow: /some/randome/page.html
         crawler.engine.download.side_effect = return_response
         return crawler
 
-    def test_robotstxt_empty_response(self):
+    @deferred_f_from_coro_f
+    async def test_robotstxt_empty_response(self):
         # empty response should equal 'allow all'
         middleware = RobotsTxtMiddleware(self._get_emptybody_crawler())
-        return DeferredList(
-            [
-                self.assertNotIgnored(Request("http://site.local/allowed"), middleware),
-                self.assertNotIgnored(
-                    Request("http://site.local/admin/main"), middleware
-                ),
-                self.assertNotIgnored(Request("http://site.local/static/"), middleware),
-            ],
-            fireOnOneErrback=True,
-        )
+        await self.assertNotIgnored(Request("http://site.local/allowed"), middleware)
+        await self.assertNotIgnored(Request("http://site.local/admin/main"), middleware)
+        await self.assertNotIgnored(Request("http://site.local/static/"), middleware)
 
-    def test_robotstxt_error(self):
+    @deferred_f_from_coro_f
+    async def test_robotstxt_error(self):
         self.crawler.settings.set("ROBOTSTXT_OBEY", True)
         err = error.DNSLookupError("Robotstxt address not found")
 
@@ -171,15 +153,13 @@ Disallow: /some/randome/page.html
 
         middleware = RobotsTxtMiddleware(self.crawler)
         middleware._logerror = mock.MagicMock(side_effect=middleware._logerror)
-        deferred = middleware.process_request(Request("http://site.local"), None)
+        await maybe_deferred_to_future(
+            middleware.process_request(Request("http://site.local"), None)
+        )
+        assert middleware._logerror.called
 
-        def check_called(_: Any) -> None:
-            assert middleware._logerror.called
-
-        deferred.addCallback(check_called)
-        return deferred
-
-    def test_robotstxt_immediate_error(self):
+    @deferred_f_from_coro_f
+    async def test_robotstxt_immediate_error(self):
         self.crawler.settings.set("ROBOTSTXT_OBEY", True)
         err = error.DNSLookupError("Robotstxt address not found")
 
@@ -191,9 +171,10 @@ Disallow: /some/randome/page.html
         self.crawler.engine.download.side_effect = immediate_failure
 
         middleware = RobotsTxtMiddleware(self.crawler)
-        return self.assertNotIgnored(Request("http://site.local"), middleware)
+        await self.assertNotIgnored(Request("http://site.local"), middleware)
 
-    def test_ignore_robotstxt_request(self):
+    @deferred_f_from_coro_f
+    async def test_ignore_robotstxt_request(self):
         self.crawler.settings.set("ROBOTSTXT_OBEY", True)
 
         def ignore_request(request):
@@ -206,13 +187,8 @@ Disallow: /some/randome/page.html
         middleware = RobotsTxtMiddleware(self.crawler)
         mw_module_logger.error = mock.MagicMock()
 
-        d = self.assertNotIgnored(Request("http://site.local/allowed"), middleware)
-
-        def check_not_called(_: Any) -> None:
-            assert not mw_module_logger.error.called  # type: ignore[attr-defined]
-
-        d.addCallback(check_not_called)
-        return d
+        await self.assertNotIgnored(Request("http://site.local/allowed"), middleware)
+        assert not mw_module_logger.error.called  # type: ignore[attr-defined]
 
     def test_robotstxt_user_agent_setting(self):
         crawler = self._get_successful_crawler()
@@ -236,19 +212,27 @@ Disallow: /some/randome/page.html
             Deferred,
         )
 
-    def assertNotIgnored(self, request, middleware):
+    async def assertNotIgnored(
+        self, request: Request, middleware: RobotsTxtMiddleware
+    ) -> None:
         spider = None  # not actually used
-        dfd = maybeDeferred(middleware.process_request, request, spider)
-        dfd.addCallback(self.assertIsNone)
-        return dfd
+        result = await maybe_deferred_to_future(
+            maybeDeferred(middleware.process_request, request, spider)  # type: ignore[call-overload]
+        )
+        assert result is None
 
-    def assertIgnored(self, request, middleware):
+    async def assertIgnored(
+        self, request: Request, middleware: RobotsTxtMiddleware
+    ) -> None:
         spider = None  # not actually used
-        return self.assertFailure(
-            maybeDeferred(middleware.process_request, request, spider), IgnoreRequest
+        await maybe_deferred_to_future(
+            self.assertFailure(
+                middleware.process_request(request, spider),  # type: ignore[arg-type]
+                IgnoreRequest,
+            )
         )
 
-    def assertRobotsTxtRequested(self, base_url):
+    def assertRobotsTxtRequested(self, base_url: str) -> None:
         calls = self.crawler.engine.download.call_args_list
         request = calls[0][0][0]
         assert request.url == f"{base_url}/robots.txt"

tests/test_utils_misc/test_return_with_argument_inside_generator.py

@@ -2,6 +2,8 @@ import warnings
 from functools import partial
 from unittest import mock
 
+import pytest
+
 from scrapy.utils.misc import (
     is_generator_with_return_value,
     warn_on_generator_with_return_value,
@@ -40,7 +42,24 @@ def generator_that_returns_stuff():
 
 
 class TestUtilsMisc:
-    def test_generators_return_something(self):
+    @pytest.fixture
+    def mock_spider(self):
+        class MockSettings:
+            def __init__(self, settings_dict=None):
+                self.settings_dict = settings_dict or {
+                    "WARN_ON_GENERATOR_RETURN_VALUE": True
+                }
+
+            def getbool(self, name, default=False):
+                return self.settings_dict.get(name, default)
+
+        class MockSpider:
+            def __init__(self):
+                self.settings = MockSettings()
+
+        return MockSpider()
+
+    def test_generators_return_something(self, mock_spider):
         def f1():
             yield 1
             return 2
@@ -75,30 +94,30 @@ https://example.org
         assert is_generator_with_return_value(i1)
 
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, top_level_return_something)
+            warn_on_generator_with_return_value(mock_spider, top_level_return_something)
             assert len(w) == 1
             assert (
-                'The "NoneType.top_level_return_something" method is a generator'
+                'The "MockSpider.top_level_return_something" method is a generator'
                 in str(w[0].message)
             )
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, f1)
+            warn_on_generator_with_return_value(mock_spider, f1)
             assert len(w) == 1
-            assert 'The "NoneType.f1" method is a generator' in str(w[0].message)
+            assert 'The "MockSpider.f1" method is a generator' in str(w[0].message)
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, g1)
+            warn_on_generator_with_return_value(mock_spider, g1)
             assert len(w) == 1
-            assert 'The "NoneType.g1" method is a generator' in str(w[0].message)
+            assert 'The "MockSpider.g1" method is a generator' in str(w[0].message)
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, h1)
+            warn_on_generator_with_return_value(mock_spider, h1)
             assert len(w) == 1
-            assert 'The "NoneType.h1" method is a generator' in str(w[0].message)
+            assert 'The "MockSpider.h1" method is a generator' in str(w[0].message)
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, i1)
+            warn_on_generator_with_return_value(mock_spider, i1)
             assert len(w) == 1
-            assert 'The "NoneType.i1" method is a generator' in str(w[0].message)
+            assert 'The "MockSpider.i1" method is a generator' in str(w[0].message)
 
-    def test_generators_return_none(self):
+    def test_generators_return_none(self, mock_spider):
         def f2():
             yield 1
 
@@ -142,31 +161,31 @@ https://example.org
         assert not is_generator_with_return_value(l2)
 
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, top_level_return_none)
+            warn_on_generator_with_return_value(mock_spider, top_level_return_none)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, f2)
+            warn_on_generator_with_return_value(mock_spider, f2)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, g2)
+            warn_on_generator_with_return_value(mock_spider, g2)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, h2)
+            warn_on_generator_with_return_value(mock_spider, h2)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, i2)
+            warn_on_generator_with_return_value(mock_spider, i2)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, j2)
+            warn_on_generator_with_return_value(mock_spider, j2)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, k2)
+            warn_on_generator_with_return_value(mock_spider, k2)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, l2)
+            warn_on_generator_with_return_value(mock_spider, l2)
             assert len(w) == 0
 
-    def test_generators_return_none_with_decorator(self):
+    def test_generators_return_none_with_decorator(self, mock_spider):
         def decorator(func):
             def inner_func():
                 func()
@@ -223,36 +242,36 @@ https://example.org
         assert not is_generator_with_return_value(l3)
 
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, top_level_return_none)
+            warn_on_generator_with_return_value(mock_spider, top_level_return_none)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, f3)
+            warn_on_generator_with_return_value(mock_spider, f3)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, g3)
+            warn_on_generator_with_return_value(mock_spider, g3)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, h3)
+            warn_on_generator_with_return_value(mock_spider, h3)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, i3)
+            warn_on_generator_with_return_value(mock_spider, i3)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, j3)
+            warn_on_generator_with_return_value(mock_spider, j3)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, k3)
+            warn_on_generator_with_return_value(mock_spider, k3)
             assert len(w) == 0
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, l3)
+            warn_on_generator_with_return_value(mock_spider, l3)
             assert len(w) == 0
 
     @mock.patch(
         "scrapy.utils.misc.is_generator_with_return_value", new=_indentation_error
     )
-    def test_indentation_error(self):
+    def test_indentation_error(self, mock_spider):
         with warnings.catch_warnings(record=True) as w:
-            warn_on_generator_with_return_value(None, top_level_return_none)
+            warn_on_generator_with_return_value(mock_spider, top_level_return_none)
             assert len(w) == 1
             assert "Unable to determine" in str(w[0].message)
 
@@ -262,3 +281,32 @@ https://example.org
 
         partial_cb = partial(cb, arg1=42)
         assert not is_generator_with_return_value(partial_cb)
+
+    def test_warn_on_generator_with_return_value_settings_disabled(self):
+        class MockSettings:
+            def __init__(self, settings_dict=None):
+                self.settings_dict = settings_dict or {}
+
+            def getbool(self, name, default=False):
+                return self.settings_dict.get(name, default)
+
+        class MockSpider:
+            def __init__(self):
+                self.settings = MockSettings({"WARN_ON_GENERATOR_RETURN_VALUE": False})
+
+        spider = MockSpider()
+
+        def gen_with_return():
+            yield 1
+            return "value"
+
+        with warnings.catch_warnings(record=True) as w:
+            warn_on_generator_with_return_value(spider, gen_with_return)
+            assert len(w) == 0
+
+        spider.settings.settings_dict["WARN_ON_GENERATOR_RETURN_VALUE"] = True
+
+        with warnings.catch_warnings(record=True) as w:
+            warn_on_generator_with_return_value(spider, gen_with_return)
+            assert len(w) == 1
+            assert "is a generator" in str(w[0].message)

tox.ini

@@ -39,7 +39,7 @@ passenv =
 #allow tox virtualenv to upgrade pip/wheel/setuptools
 download = true
 commands =
-    pytest {posargs:--cov-config=pyproject.toml --cov=scrapy --cov-report= --cov-report=term-missing --cov-report=xml --durations=10 docs scrapy tests --doctest-modules}
+    pytest {posargs:--cov-config=pyproject.toml --cov=scrapy --cov-report= --cov-report=term-missing --cov-report=xml --junitxml=testenv.junit.xml -o junit_family=legacy --durations=10 docs scrapy tests --doctest-modules}
 install_command =
     python -I -m pip install -ctests/upper-constraints.txt {opts} {packages}
 
@@ -118,7 +118,7 @@ install_command =
     python -I -m pip install {opts} {packages}
 commands =
     ; tests for docs fail with parsel < 1.8.0
-    pytest {posargs:--cov-config=pyproject.toml --cov=scrapy --cov-report=xml --cov-report= --durations=10 scrapy tests}
+    pytest {posargs:--cov-config=pyproject.toml --cov=scrapy --cov-report=xml --cov-report= --junitxml=pinned.junit.xml -o junit_family=legacy --durations=10 scrapy tests}
 
 [testenv:pinned]
 basepython = {[pinned]basepython}
@@ -254,7 +254,7 @@ deps =
     {[testenv]deps}
     botocore>=1.4.87
 commands =
-    pytest {posargs:--cov-config=pyproject.toml --cov=scrapy --cov-report=xml --cov-report= tests -m requires_botocore}
+    pytest {posargs:--cov-config=pyproject.toml --cov=scrapy --cov-report=xml --cov-report= tests --junitxml=botocore.junit.xml -o junit_family=legacy -m requires_botocore}
 
 [testenv:botocore-pinned]
 basepython = {[pinned]basepython}
@@ -265,4 +265,4 @@ install_command = {[pinned]install_command}
 setenv =
     {[pinned]setenv}
 commands =
-    pytest {posargs:--cov-config=pyproject.toml --cov=scrapy --cov-report=xml --cov-report= tests -m requires_botocore}
+    pytest {posargs:--cov-config=pyproject.toml --cov=scrapy --cov-report=xml --cov-report= tests --junitxml=botocore-pinned.junit.xml -o junit_family=legacy -m requires_botocore}