Repository: scrapinghub/spidermon

Issue:
bug: AttributeError when using ValidationMonitorMixin
When executing a monitor using `ValidationMonitorMixin`, a `AttributeError` is raised because a needed attribute is not set:
`AttributeError: 'CoverageValidationMonitor' object has no attribute 'correct_field_list_handling'`

This answers this comment:
> @ejulio I've implemented that and wrote tests for all methods for both old and new behavior.
> 
> My only concern about setting the flag in `__init__`: as this is a mixin I'm not sure how should this be used in user code. In the test I just set the variable instead of calling mixin's `__init__`.
> 
> _Originally posted by @wRAR in https://github.com/scrapinghub/spidermon/pull/164#issuecomment-508067602_

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/scrapinghub/spidermon/pull/248?src=pr&el=h1) Report
> Merging [#248](https://codecov.io/gh/scrapinghub/spidermon/pull/248?src=pr&el=desc) into [master](https://codecov.io/gh/scrapinghub/spidermon/commit/e379613b4d0b4d2d16e0c558793cd4910519bc65&el=desc) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/scrapinghub/spidermon/pull/248/graphs/tree.svg?width=650&height=150&src=pr&token=zsI7aJ5kxl)](https://codecov.io/gh/scrapinghub/spidermon/pull/248?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   71.65%   71.70%   +0.05%     
==========================================
  Files          67       67              
  Lines        2759     2757       -2     
  Branches      309      309              
==========================================
  Hits         1977     1977              
+ Misses        718      716       -2     
  Partials       64       64              
```


| [Impacted Files](https://codecov.io/gh/scrapinghub/spidermon/pull/248?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [spidermon/contrib/monitors/mixins/validation.py](https://codecov.io/gh/scrapinghub/spidermon/pull/248/diff?src=pr&el=tree#diff-c3BpZGVybW9uL2NvbnRyaWIvbW9uaXRvcnMvbWl4aW5zL3ZhbGlkYXRpb24ucHk=) | `90.68% <100.00%> (+1.11%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/scrapinghub/spidermon/pull/248?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/scrapinghub/spidermon/pull/248?src=pr&el=footer). Last update [e379613...d06b1a3](https://codecov.io/gh/scrapinghub/spidermon/pull/248?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Gallaecio: https://github.com/scrapinghub/spidermon/issues/246 sounds to me like a bad usage in the code using the mixing (defining `__init__` without calling `super().__init__`), so I’m not sure it is a bug. But I guess the change results in better usability. So +0 from me.
rennerocha: > #246 sounds to me like a bad usage in the code using the mixin (defining `__init__` without calling `super().__init__`), so I’m not sure it is a bug. But I guess the change results in better usability. So +0 from me.

Mixin `__init__` is never called so that is way we have this bug. I agree that this is not the right usage for this attribute anyway. Maybe in a next release we can get rid of it. For now, let's make the mixin usable.
Gallaecio: I’m not walking about using `super` in the mixin, but in the classes using the mixin.

Whenever you inherit from a class, be it a mixin or something else, you are still inheriting from a class, and you must use `super` in your `__init__`. Otherwise, you are preventing the `__init__` of parent classes to be executed.

This specific mixin can be modified not to use `__init__`, but that will not always be the case.

Patch:
diff --git a/spidermon/contrib/monitors/mixins/validation.py b/spidermon/contrib/monitors/mixins/validation.py
index dba5b54..105aa8e 100644
--- a/spidermon/contrib/monitors/mixins/validation.py
+++ b/spidermon/contrib/monitors/mixins/validation.py
@@ -86,9 +86,8 @@ class ValidationInfo(object):
 
 
 class ValidationMonitorMixin(StatsMonitorMixin):
-    def __init__(self, correct_field_list_handling=False):
-        super(ValidationMonitorMixin, self).__init__()
-        self.correct_field_list_handling = correct_field_list_handling
+
+    correct_field_list_handling = False
 
     @property
     def validation(self):
@@ -111,8 +110,8 @@ class ValidationMonitorMixin(StatsMonitorMixin):
         """
         Checks that the number of "missing_required_field" errors for the ``field_names`` fields is less or equal than
         ``allowed_count`` and raises an error with all problematic fields.
-        If ``field_names`` is None and ``new_behavior`` is True, checks all fields.
-        If ``field_names`` is None and ``new_behavior`` is False, checks that the total number of
+        If ``field_names`` is None and ``self.correct_field_list_handling`` is True, checks all fields.
+        If ``field_names`` is None and ``self.correct_field_list_handling`` is False, checks that the total number of
         "missing_required_field" errors is less or equal than ``allowed_count``.
         """
         if not self.correct_field_list_handling and not field_names:
@@ -178,8 +177,8 @@ class ValidationMonitorMixin(StatsMonitorMixin):
         """
         Checks that the number of "missing_required_field" errors for the ``field_names`` fields divided by the number
         of items is less or equal than ``allowed_percent`` and raises an error with all problematic fields.
-        If ``field_names`` is None and ``self.new_behavior`` is True, checks all fields.
-        If ``field_names`` is None and ``self.new_behavior`` is False, checks that the total number of
+        If ``field_names`` is None and ``self.correct_field_list_handling`` is True, checks all fields.
+        If ``field_names`` is None and ``self.correct_field_list_handling`` is False, checks that the total number of
         "missing_required_field" errors is less or equal than ``allowed_count``.
         """
         if not self.correct_field_list_handling and not field_names:
@@ -246,8 +245,8 @@ class ValidationMonitorMixin(StatsMonitorMixin):
         """
         Checks that the number of errors for the ``field_names`` fields is less or equal than ``allowed_count`` and
         raises an error with all problematic fields.
-        If ``field_names`` is None and ``new_behavior`` is True, checks all fields.
-        If ``field_names`` is None and ``new_behavior`` is False, checks that the total number of errors is less or
+        If ``field_names`` is None and ``self.correct_field_list_handling`` is True, checks all fields.
+        If ``field_names`` is None and ``self.correct_field_list_handling`` is False, checks that the total number of errors is less or
         equal than ``allowed_count``.
         """
         if not self.correct_field_list_handling and not field_names:
@@ -314,8 +313,8 @@ class ValidationMonitorMixin(StatsMonitorMixin):
         """
         Checks that the number of errors for the ``field_names`` fields divided by the number of items is less or equal
         than ``allowed_percent`` and raises an error with all problematic fields.
-        If ``field_names`` is None and ``new_behavior`` is True, checks all fields.
-        If ``field_names`` is None and ``new_behavior`` is False, checks that the total number of errors divided by the
+        If ``field_names`` is None and ``self.correct_field_list_handling`` is True, checks all fields.
+        If ``field_names`` is None and ``self.correct_field_list_handling`` is False, checks that the total number of errors divided by the
         number of items is less or equal than ``allowed_count``
         """
         if not self.correct_field_list_handling and not field_names:


Test patch:
diff --git a/tests/contrib/monitors/mixins/test_validation.py b/tests/contrib/monitors/mixins/test_validation.py
index d8acf66..19f970c 100644
--- a/tests/contrib/monitors/mixins/test_validation.py
+++ b/tests/contrib/monitors/mixins/test_validation.py
@@ -19,25 +19,30 @@ stats = {
 
 
 class DummyValidationMonitor(BaseScrapyMonitor, ValidationMonitorMixin):
-    def __init__(
-        self, stats, correct_field_list_handling, methodName="runTest", name=None
-    ):
-        super(DummyValidationMonitor, self).__init__(methodName, name)
-        self.data = Data({"stats": stats})
-        self.correct_field_list_handling = correct_field_list_handling
-
     def runTest(self):
         pass
 
 
 @pytest.fixture
 def monitor():
-    return DummyValidationMonitor(stats, True)
-
+    monitor = DummyValidationMonitor()
+    monitor.correct_field_list_handling = True
+    monitor.data = Data({"stats": stats})
+    return monitor
 
 @pytest.fixture
 def old_monitor():
-    return DummyValidationMonitor(stats, False)
+    monitor = DummyValidationMonitor()
+    monitor.correct_field_list_handling = False
+    monitor.data = Data({"stats": stats})
+    return monitor
+
+
+@pytest.fixture
+def old_monitor_without_attr():
+    monitor = DummyValidationMonitor()
+    monitor.data = Data({"stats": stats})
+    return monitor
 
 
 def test_check_missing_required_fields_no_fields(monitor):
@@ -82,6 +87,12 @@ def test_check_missing_required_fields_no_fields_old(old_monitor):
         old_monitor.check_missing_required_fields()
 
 
+def test_check_missing_required_fields_no_fields_old_without_attribute(old_monitor_without_attr):
+    msg = "15 required fields are missing!"
+    with pytest.raises(AssertionError, match=msg):
+        old_monitor_without_attr.check_missing_required_fields()
+
+
 def check_missing_required_field(monitor):
     monitor.check_missing_required_field(field_name="field1")
     msg = "Required field field2 is missing in 5 items!"