Repository: scrapy/scrapy

Issue:
Add note that CrawlerProcess.crawl() needs a crawler or spider class, not a spider instance
Even if [it's in the docs](http://doc.scrapy.org/en/latest/topics/api.html#scrapy.crawler.CrawlerProcess.crawl), 

> crawler_or_spidercls (Crawler instance, Spider subclass or string) – already created crawler, or a spider class or spider’s name inside the project to create it

users still miss sometimes that `.crawl()` expects a Spider class (or a crawler) but not a Spider instance.

e.g. http://stackoverflow.com/q/39639568

> I overlooked the fact that the class itself has to be passed as an argument, and not an instance of that class.
> This is quite subtle on the eye but makes all the difference

This ticket is to make it a bit more prominent.


Discussion:
Gallaecio: I’ll review the failing text next week, it probably needs updated expectations.
codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/3610?src=pr&el=h1) Report
> Merging [#3610](https://codecov.io/gh/scrapy/scrapy/pull/3610?src=pr&el=desc) into [master](https://codecov.io/gh/scrapy/scrapy/commit/b364bfb68b481a696141aa6b7b3312679670c38e?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `14.28%`.


```diff
@@            Coverage Diff             @@
##           master    #3610      +/-   ##
==========================================
- Coverage    84.5%   84.46%   -0.05%     
==========================================
  Files         167      167              
  Lines        9406     9413       +7     
  Branches     1397     1400       +3     
==========================================
+ Hits         7949     7951       +2     
- Misses       1199     1202       +3     
- Partials      258      260       +2
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/3610?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/crawler.py](https://codecov.io/gh/scrapy/scrapy/pull/3610/diff?src=pr&el=tree#diff-c2NyYXB5L2NyYXdsZXIucHk=) | `88.88% <14.28%> (-3.19%)` | :arrow_down: |
| [scrapy/utils/trackref.py](https://codecov.io/gh/scrapy/scrapy/pull/3610/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL3RyYWNrcmVmLnB5) | `86.48% <0%> (+2.7%)` | :arrow_up: |

Gallaecio: Oh, right, I should cover the checks with automated tests.

Patch:
diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst
index 76360b15f..e29914dbf 100644
--- a/docs/topics/request-response.rst
+++ b/docs/topics/request-response.rst
@@ -489,11 +489,6 @@ method for this job. Here's an example spider which uses it::
 
     import scrapy
 
-    def authentication_failed(response):
-        # TODO: Check the contents of the response and return True if it failed
-        # or False if it succeeded.
-        pass
-
     class LoginSpider(scrapy.Spider):
         name = 'example.com'
         start_urls = ['http://www.example.com/users/login.php']
@@ -506,7 +501,8 @@ method for this job. Here's an example spider which uses it::
             )
 
         def after_login(self, response):
-            if authentication_failed(response):
+            # check login succeed before going on
+            if "authentication failed" in response.body:
                 self.logger.error("Login failed")
                 return
 
diff --git a/scrapy/crawler.py b/scrapy/crawler.py
index 04aee18ed..ee00d27b4 100644
--- a/scrapy/crawler.py
+++ b/scrapy/crawler.py
@@ -7,6 +7,7 @@ import sys
 from twisted.internet import reactor, defer
 from zope.interface.verify import verifyClass, DoesNotImplement
 
+from scrapy import Spider
 from scrapy.core.engine import ExecutionEngine
 from scrapy.resolver import CachingThreadedResolver
 from scrapy.interfaces import ISpiderLoader
@@ -27,6 +28,10 @@ logger = logging.getLogger(__name__)
 class Crawler(object):
 
     def __init__(self, spidercls, settings=None):
+        if isinstance(spidercls, Spider):
+            raise ValueError(
+                'The spidercls argument must be a class, not an object')
+
         if isinstance(settings, dict) or settings is None:
             settings = Settings(settings)
 
@@ -168,6 +173,10 @@ class CrawlerRunner(object):
 
         :param dict kwargs: keyword arguments to initialize the spider
         """
+        if isinstance(crawler_or_spidercls, Spider):
+            raise ValueError(
+                'The crawler_or_spidercls argument cannot be a spider object, '
+                'it must be a spider class (or a Crawler object)')
         crawler = self.create_crawler(crawler_or_spidercls)
         return self._crawl(crawler, *args, **kwargs)
 
@@ -195,6 +204,10 @@ class CrawlerRunner(object):
           a spider with this name in a Scrapy project (using spider loader),
           then creates a Crawler instance for it.
         """
+        if isinstance(crawler_or_spidercls, Spider):
+            raise ValueError(
+                'The crawler_or_spidercls argument cannot be a spider object, '
+                'it must be a spider class (or a Crawler object)')
         if isinstance(crawler_or_spidercls, Crawler):
             return crawler_or_spidercls
         return self._create_crawler(crawler_or_spidercls)


Test patch:
diff --git a/tests/test_crawler.py b/tests/test_crawler.py
index 268948a70..37cea3ad3 100644
--- a/tests/test_crawler.py
+++ b/tests/test_crawler.py
@@ -4,6 +4,7 @@ import warnings
 
 from twisted.internet import defer
 from twisted.trial import unittest
+from pytest import raises
 
 import scrapy
 from scrapy.crawler import Crawler, CrawlerRunner, CrawlerProcess
@@ -66,6 +67,10 @@ class CrawlerTestCase(BaseCrawlerTest):
         crawler = Crawler(DefaultSpider)
         self.assertOptionIsDefault(crawler.settings, 'RETRY_ENABLED')
 
+    def test_crawler_rejects_spider_objects(self):
+        with raises(ValueError):
+            Crawler(DefaultSpider())
+
 
 class SpiderSettingsTestCase(unittest.TestCase):
     def test_spider_custom_settings(self):
@@ -177,6 +182,14 @@ class CrawlerRunnerTestCase(BaseCrawlerTest):
             self.assertEqual(len(w), 1)
             self.assertIn('Please use SPIDER_LOADER_CLASS', str(w[0].message))
 
+    def test_crawl_rejects_spider_objects(self):
+        with raises(ValueError):
+            CrawlerRunner().crawl(DefaultSpider())
+
+    def test_create_crawler_rejects_spider_objects(self):
+        with raises(ValueError):
+            CrawlerRunner().create_crawler(DefaultSpider())
+
 
 class CrawlerProcessTest(BaseCrawlerTest):
     def test_crawler_process_accepts_dict(self):
diff --git a/tests/test_downloadermiddleware_httpproxy.py b/tests/test_downloadermiddleware_httpproxy.py
index 537126613..30920b2da 100644
--- a/tests/test_downloadermiddleware_httpproxy.py
+++ b/tests/test_downloadermiddleware_httpproxy.py
@@ -25,7 +25,7 @@ class TestHttpProxyMiddleware(TestCase):
 
     def test_not_enabled(self):
         settings = Settings({'HTTPPROXY_ENABLED': False})
-        crawler = Crawler(spider, settings)
+        crawler = Crawler(Spider, settings)
         self.assertRaises(NotConfigured, partial(HttpProxyMiddleware.from_crawler, crawler))
 
     def test_no_environment_proxies(self):