Repository: scrapinghub/spidermon (language: python)

Issue:
Fix "ModuleNotFoundError" when adding LocalStorageStatsHistoryCollector
This error only happens when #340 is fixed (we need https://github.com/scrapinghub/spidermon/issues/342 to be merged so we can reproduce this error) and we add the following value for `STATS_CLASS` in our project settings:

```
STATS_CLASS = (
    "spidermon.contrib.stats.statscollectors.LocalStorageStatsHistoryCollector"
)
```

Because `DashCollectionsStatsHistoryCollector` uses `scrapinghub` library, we have the following error:

```
2022-03-25 18:02:06 [scrapy.utils.log] INFO: Scrapy 2.6.1 started (bot: tutorial)
2022-03-25 18:02:06 [scrapy.utils.log] INFO: Versions: lxml 4.8.0.0, libxml2 2.9.12, cssselect 1.1.0, parsel 1.6.0, w3lib 1.22.0, Twisted 22.2.0, Python 3.8.10 (default, Nov 26 2021, 20:14:08) - [GCC 9.3.0], pyOpenSSL 22.0.0 (OpenSSL 1.1.1n  15 Mar 2022), cryptography 36.0.2, Platform Linux-5.4.0-105-generic-x86_64-with-glibc2.29
Traceback (most recent call last):
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/bin/scrapy", line 8, in <module>
    sys.exit(execute())
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/cmdline.py", line 145, in execute
    _run_print_help(parser, _run_command, cmd, args, opts)
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/cmdline.py", line 100, in _run_print_help
    func(*a, **kw)
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/cmdline.py", line 153, in _run_command
    cmd.run(args, opts)
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/commands/crawl.py", line 22, in run
    crawl_defer = self.crawler_process.crawl(spname, **opts.spargs)
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/crawler.py", line 205, in crawl
    crawler = self.create_crawler(crawler_or_spidercls)
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/crawler.py", line 238, in create_crawler
    return self._create_crawler(crawler_or_spidercls)
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/crawler.py", line 313, in _create_crawler
    return Crawler(spidercls, self.settings, init_reactor=True)
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/crawler.py", line 54, in __init__
    self.stats = load_object(self.settings['STATS_CLASS'])(self)
  File "/home/renne/.pyenv/versions/spidermon-test-clean-install/lib/python3.8/site-packages/scrapy/utils/misc.py", line 61, in load_object
    mod = import_module(module)
  File "/usr/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 848, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/renne/projects/rennerocha/spidermon/spidermon/contrib/stats/statscollectors.py", line 6, in <module>
    import scrapinghub
ModuleNotFoundError: No module named 'scrapinghub'
```


Discussion / hints:

Fix patch:
diff --git a/docs/source/howto/stats-collection.rst b/docs/source/howto/stats-collection.rst
index a95c32c9..60c83bd8 100644
--- a/docs/source/howto/stats-collection.rst
+++ b/docs/source/howto/stats-collection.rst
@@ -22,7 +22,7 @@ To enable it, include the following code in your project settings:
 
     # tutorial/settings.py
     STATS_CLASS = (
-        "spidermon.contrib.stats.statscollectors.LocalStorageStatsHistoryCollector"
+        "spidermon.contrib.stats.statscollectors.local_storage.LocalStorageStatsHistoryCollector"
     )
 
     # Stores the stats of the last 10 spider execution (default=100)
@@ -70,12 +70,12 @@ returned in the previous spider executions.
     class SpiderCloseMonitorSuite(MonitorSuite):
         monitors = [HistoryMonitor]
 
-When running on `Scrapy Cloud`_ you can use ``spidermon.contrib.stats.statscollectors.DashCollectionsStatsHistoryCollector`` instead.
+When running on `Scrapy Cloud`_ you can use ``spidermon.contrib.stats.statscollectors.sc_collections.ScrapyCloudCollectionsStatsHistoryCollector`` instead.
 This will save your stats in a `collection`_ on your scrapy dashboard, named like ``{your_spider_name}_stats_history``. The rest of the sample code presented previously will work unchanged.
 
 
 .. warning::
-    `STATS_CLASS`_ is overriden by default in `Scrapy Cloud`_. You need to manually include ``spidermon.contrib.stats.statscollectors.DashCollectionsStatsHistoryCollector`` in your `spider settings`_.
+    `STATS_CLASS`_ is overriden by default in `Scrapy Cloud`_. You need to manually include ``spidermon.contrib.stats.statscollectors.sc_collections.DashCollectionsStatsHistoryCollector`` in your `spider settings`_.
 
 .. _`STATS_CLASS`: https://docs.scrapy.org/en/latest/topics/settings.html#stats-class
 .. _`spider settings`: https://support.zyte.com/support/solutions/articles/22000200670-customizing-scrapy-settings-in-scrapy-cloud
diff --git a/examples/tutorial/tutorial/settings.py b/examples/tutorial/tutorial/settings.py
index b6f9b92e..61cc8e2f 100644
--- a/examples/tutorial/tutorial/settings.py
+++ b/examples/tutorial/tutorial/settings.py
@@ -19,9 +19,7 @@
 
 SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS = True
 
-STATS_CLASS = (
-    "spidermon.contrib.stats.statscollectors.LocalStorageStatsHistoryCollector"
-)
+STATS_CLASS = "spidermon.contrib.stats.statscollectors.local_storage.LocalStorageStatsHistoryCollector"
 
 SPIDERMON_MAX_STORED_STATS = 10  # Stores the stats of the last 10 spider execution
 
diff --git a/spidermon/contrib/stats/statscollectors/__init__.py b/spidermon/contrib/stats/statscollectors/__init__.py
new file mode 100644
index 00000000..c2c519b7
--- /dev/null
+++ b/spidermon/contrib/stats/statscollectors/__init__.py
@@ -0,0 +1,3 @@
+from spidermon.contrib.stats.statscollectors.local_storage import (
+    LocalStorageStatsHistoryCollector,
+)
diff --git a/spidermon/contrib/stats/statscollectors/local_storage.py b/spidermon/contrib/stats/statscollectors/local_storage.py
new file mode 100644
index 00000000..ae6a99d0
--- /dev/null
+++ b/spidermon/contrib/stats/statscollectors/local_storage.py
@@ -0,0 +1,37 @@
+import os
+import pickle
+from collections import deque
+
+from scrapy.statscollectors import StatsCollector
+from scrapy.utils.project import data_path
+
+
+class LocalStorageStatsHistoryCollector(StatsCollector):
+    def _stats_location(self, spider):
+        statsdir = data_path("stats", createdir=True)
+        return os.path.join(statsdir, f"{spider.name}_stats_history")
+
+    def open_spider(self, spider):
+        stats_location = self._stats_location(spider)
+
+        max_stored_stats = spider.crawler.settings.getint(
+            "SPIDERMON_MAX_STORED_STATS", default=100
+        )
+
+        if os.path.isfile(stats_location):
+            with open(stats_location, "rb") as stats_file:
+                _stats_history = pickle.load(stats_file)
+        else:
+            _stats_history = deque([], maxlen=max_stored_stats)
+
+        if _stats_history.maxlen != max_stored_stats:
+            _stats_history = deque(_stats_history, maxlen=max_stored_stats)
+
+        spider.stats_history = _stats_history
+
+    def _persist_stats(self, stats, spider):
+        stats_location = self._stats_location(spider)
+
+        spider.stats_history.appendleft(self._stats)
+        with open(stats_location, "wb") as stats_file:
+            pickle.dump(spider.stats_history, stats_file)
diff --git a/spidermon/contrib/stats/statscollectors.py b/spidermon/contrib/stats/statscollectors/sc_collections.py
similarity index 60%
rename from spidermon/contrib/stats/statscollectors.py
rename to spidermon/contrib/stats/statscollectors/sc_collections.py
index 13cf7cb6..2ca98b0e 100644
--- a/spidermon/contrib/stats/statscollectors.py
+++ b/spidermon/contrib/stats/statscollectors/sc_collections.py
@@ -1,48 +1,14 @@
 import logging
 import os
-import pickle
 from collections import deque
 
 import scrapinghub
-from scrapy.statscollectors import StatsCollector
-from scrapy.utils.project import data_path
 from sh_scrapy.stats import HubStorageStatsCollector
 
 logger = logging.getLogger(__name__)
 
 
-class LocalStorageStatsHistoryCollector(StatsCollector):
-    def _stats_location(self, spider):
-        statsdir = data_path("stats", createdir=True)
-        return os.path.join(statsdir, f"{spider.name}_stats_history")
-
-    def open_spider(self, spider):
-        stats_location = self._stats_location(spider)
-
-        max_stored_stats = spider.crawler.settings.getint(
-            "SPIDERMON_MAX_STORED_STATS", default=100
-        )
-
-        if os.path.isfile(stats_location):
-            with open(stats_location, "rb") as stats_file:
-                _stats_history = pickle.load(stats_file)
-        else:
-            _stats_history = deque([], maxlen=max_stored_stats)
-
-        if _stats_history.maxlen != max_stored_stats:
-            _stats_history = deque(_stats_history, maxlen=max_stored_stats)
-
-        spider.stats_history = _stats_history
-
-    def _persist_stats(self, stats, spider):
-        stats_location = self._stats_location(spider)
-
-        spider.stats_history.appendleft(self._stats)
-        with open(stats_location, "wb") as stats_file:
-            pickle.dump(spider.stats_history, stats_file)
-
-
-class DashCollectionsStatsHistoryCollector(HubStorageStatsCollector):
+class ScrapyCloudCollectionsStatsHistoryCollector(HubStorageStatsCollector):
     def _open_collection(self, spider):
         sh_client = scrapinghub.ScrapinghubClient()
         proj_id = os.environ.get("SCRAPY_PROJECT_ID")


Test patch:
diff --git a/tests/contrib/stats/statscollectors/__init__.py b/tests/contrib/stats/statscollectors/__init__.py
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/contrib/stats/test_localstoragestats.py b/tests/contrib/stats/statscollectors/test_local_storage.py
similarity index 79%
rename from tests/contrib/stats/test_localstoragestats.py
rename to tests/contrib/stats/statscollectors/test_local_storage.py
index bc5d4885..1e117cb1 100644
--- a/tests/contrib/stats/test_localstoragestats.py
+++ b/tests/contrib/stats/statscollectors/test_local_storage.py
@@ -5,7 +5,9 @@
 
 from scrapy import Spider
 from scrapy.utils.test import get_crawler
-from spidermon.contrib.stats.statscollectors import LocalStorageStatsHistoryCollector
+from spidermon.contrib.stats.statscollectors.local_storage import (
+    LocalStorageStatsHistoryCollector,
+)
 
 
 @pytest.fixture
@@ -21,7 +23,7 @@ def stats_temporary_location(monkeypatch, tmp_path):
 def test_settings():
     return {
         "STATS_CLASS": (
-            "spidermon.contrib.stats.statscollectors.LocalStorageStatsHistoryCollector"
+            "spidermon.contrib.stats.statscollectors.local_storage.LocalStorageStatsHistoryCollector"
         )
     }
 
@@ -126,3 +128,25 @@ def test_spider_limit_number_of_stored_stats(test_settings, stats_temporary_loca
     assert "third_execution" in crawler.spider.stats_history[0].keys()
     assert "second_execution" in crawler.spider.stats_history[1].keys()
     crawler.stop()
+
+
+def test_able_to_import_deprecated_local_storage_stats_collector_module():
+    """
+    To avoid an error when importing this stats collector with the old location
+    in legacy code, we need to ensure that LocalStorageStatsHistoryCollector can
+    be imported as the old module.
+
+    Original module:
+    spidermon.contrib.stats.statscollectors.LocalStorageStatsHistoryCollector
+
+    New module:
+    spidermon.contrib.stats.statscollectors.local_storage.LocalStorageStatsHistoryCollector
+    """
+    try:
+        from spidermon.contrib.stats.statscollectors import (
+            LocalStorageStatsHistoryCollector,
+        )
+    except ModuleNotFoundError:
+        assert (
+            False
+        ), f"Unable to import spidermon.contrib.stats.statscollectors.LocalStorageStatsHistoryCollector"
diff --git a/tests/contrib/stats/test_dashcollectionsstats.py b/tests/contrib/stats/statscollectors/test_sc_collections.py
similarity index 84%
rename from tests/contrib/stats/test_dashcollectionsstats.py
rename to tests/contrib/stats/statscollectors/test_sc_collections.py
index 232c5f7e..57419598 100644
--- a/tests/contrib/stats/test_dashcollectionsstats.py
+++ b/tests/contrib/stats/statscollectors/test_sc_collections.py
@@ -6,7 +6,9 @@
 
 from scrapy import Spider
 from scrapy.utils.test import get_crawler
-from spidermon.contrib.stats.statscollectors import DashCollectionsStatsHistoryCollector
+from spidermon.contrib.stats.statscollectors.sc_collections import (
+    ScrapyCloudCollectionsStatsHistoryCollector,
+)
 
 
 class StoreMock:
@@ -27,7 +29,7 @@ def set(self, data):
 def stats_collection(monkeypatch):
     store = StoreMock()
     monkeypatch.setattr(
-        DashCollectionsStatsHistoryCollector,
+        ScrapyCloudCollectionsStatsHistoryCollector,
         "_open_collection",
         lambda *args: store,
     )
@@ -38,7 +40,7 @@ def stats_collection_not_exist(monkeypatch):
     store = StoreMock()
     store.raise_iter_error = True
     monkeypatch.setattr(
-        DashCollectionsStatsHistoryCollector,
+        ScrapyCloudCollectionsStatsHistoryCollector,
         "_open_collection",
         lambda *args: store,
     )
@@ -48,31 +50,31 @@ def stats_collection_not_exist(monkeypatch):
 def test_settings():
     return {
         "STATS_CLASS": (
-            "spidermon.contrib.stats.statscollectors.DashCollectionsStatsHistoryCollector"
+            "spidermon.contrib.stats.statscollectors.sc_collections.ScrapyCloudCollectionsStatsHistoryCollector"
         )
     }
 
 
-@patch("spidermon.contrib.stats.statscollectors.scrapinghub")
+@patch("spidermon.contrib.stats.statscollectors.sc_collections.scrapinghub")
 def test_open_spider_without_api(scrapinghub_mock, test_settings):
     mock_spider = MagicMock()
     crawler = get_crawler(Spider, test_settings)
-    pipe = DashCollectionsStatsHistoryCollector(crawler)
+    pipe = ScrapyCloudCollectionsStatsHistoryCollector(crawler)
 
     pipe.open_spider(mock_spider)
 
     assert pipe.store is None
 
 
-@patch("spidermon.contrib.stats.statscollectors.scrapinghub")
-@patch("spidermon.contrib.stats.statscollectors.os.environ.get")
-def test_open_collection_with_api(scrapinghub_mock, os_enviorn_mock, test_settings):
+@patch("spidermon.contrib.stats.statscollectors.sc_collections.scrapinghub")
+@patch("spidermon.contrib.stats.statscollectors.sc_collections.os.environ.get")
+def test_open_collection_with_api(scrapinghub_mock, os_environ_mock, test_settings):
     mock_spider = MagicMock()
     mock_spider.name = "test"
 
-    os_enviorn_mock.return_value = 1234
+    os_environ_mock.return_value = 1234
     crawler = get_crawler(Spider, test_settings)
-    pipe = DashCollectionsStatsHistoryCollector(crawler)
+    pipe = ScrapyCloudCollectionsStatsHistoryCollector(crawler)
 
     store = pipe._open_collection(mock_spider)
 
@@ -181,7 +183,7 @@ def test_spider_limit_number_of_stored_stats(test_settings, stats_collection):
     crawler.stop()
 
 
-@patch("spidermon.contrib.stats.statscollectors.os.environ.get")
+@patch("spidermon.contrib.stats.statscollectors.sc_collections.os.environ.get")
 def test_job_id_added(mock_os_enviorn_get, test_settings, stats_collection):
     mock_os_enviorn_get.return_value = "test/test/test"
     crawler = get_crawler(Spider, test_settings)
@@ -195,7 +197,7 @@ def test_job_id_added(mock_os_enviorn_get, test_settings, stats_collection):
     )
 
 
-@patch("spidermon.contrib.stats.statscollectors.os.environ.get")
+@patch("spidermon.contrib.stats.statscollectors.sc_collections.os.environ.get")
 def test_job_id_not_available(mock_os_enviorn_get, test_settings, stats_collection):
     mock_os_enviorn_get.return_value = None
     crawler = get_crawler(Spider, test_settings)
@@ -206,7 +208,7 @@ def test_job_id_not_available(mock_os_enviorn_get, test_settings, stats_collecti
     assert "job_url" not in crawler.spider.stats_history[0]
 
 
-@patch("spidermon.contrib.stats.statscollectors.os.environ.get")
+@patch("spidermon.contrib.stats.statscollectors.sc_collections.os.environ.get")
 def test_stats_history_when_no_collection(
     os_enviorn_mock, stats_collection_not_exist, test_settings
 ):
@@ -216,6 +218,6 @@ def test_stats_history_when_no_collection(
 
     os_enviorn_mock.return_value = 1234
     crawler = get_crawler(Spider, test_settings)
-    pipe = DashCollectionsStatsHistoryCollector(crawler)
+    pipe = ScrapyCloudCollectionsStatsHistoryCollector(crawler)
     pipe.open_spider(mock_spider)
     assert mock_spider.stats_history == deque([], maxlen=100)