Merge 3ea6ed1 into 7251e32

dcicutils/qa_utils.py

@@ -6,7 +6,7 @@
 import contextlib
 import os
 import pytz
-from .misc_utils import PRINT
+from .misc_utils import PRINT, ignored
 
 
 def mock_not_called(name):
@@ -179,3 +179,39 @@ def sleep(self, secs: float):
         """
 
         self._just_now += datetime.timedelta(seconds=secs)
+
+
+def notice_pytest_fixtures(*fixtures):
+    """
+    This declares its arguments to be pytest fixtures in use by surrounding code.
+
+    This useful for assuring tools like flake8 and PyCharm that the arguments it is given are not being
+    ignored but instead may just have usage patterns that don't make uses apparent.
+
+    For example, in a file test_file.py, we might see uses of my_fixture and my_autoused_fixture
+    that wrongly appear both globally unused AND also locally unused in the test_something function.
+
+      from module_a import foo, bar
+      from module_b import mock_application  # <-- sample fixture, to be used explicitly
+      from module_c import mock_database     # <-- sample fixture to be used implicitly
+
+      def test_something(mock_application):  # <-- sample of fixture used explicitly
+          assert foo() = bar()
+
+    In both cases, the apparently unused variables may cause flake8, PyCharm, and other
+    'lint' programs to complain that some repair action is needed, such as removing the unused
+    variables, even though such an action might break code. Using this declaration will
+    guard against that, while providing useful documentation for the code:
+
+      from module_a import foo, bar
+      from module_b import mock_application
+      from module_c import mock_database
+      from dcicutils.qa_utils import notice_pytest_fixtures
+
+      notice_pytest_fixtures(application, database_session)
+
+      def test_something(mock_application):
+          notice_pytest_fixtures(mock_application)  # <-- protects bound variable from seeming unused
+          assert foo() = bar()
+    """
+    ignored(fixtures)  # we don't use the given fixtures, but now the tools will think we do

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "dcicutils"
-version = "0.24.0"
+version = "0.25.0"
 description = "Utility package for interacting with the 4DN Data Portal and other 4DN resources"
 authors = ["4DN-DCIC Team <support@4dnucleome.org>"]
 license = "MIT"

test/fixtures/sample_fixtures.py

@@ -0,0 +1,25 @@
+import pytest
+
+from unittest import mock
+
+
+class MockMathError(Exception):
+    pass
+
+
+class MockMath:
+
+    MATH_ENABLED = False
+
+    @classmethod
+    def add(cls, x, y):
+        if cls.MATH_ENABLED:
+            return x + y
+        else:
+            raise MockMathError("Math is not enabled.")
+
+
+@pytest.yield_fixture()
+def math_enabled():
+    with mock.patch.object(MockMath, "MATH_ENABLED", True):
+        yield

test/test_misc_utils.py

@@ -2,7 +2,10 @@
 import json
 import os
 import webtest
-from dcicutils.misc_utils import PRINT, ignored, get_setting_from_context, VirtualApp, _VirtualAppHelper
+from dcicutils.misc_utils import (
+    PRINT, ignored, get_setting_from_context, VirtualApp,
+    _VirtualAppHelper,  # noqa - yes, this is a protected member, but we still want to test it
+)
 from unittest import mock
 
 
@@ -17,10 +20,8 @@ def test_uppercase_print():
 
 
 def test_ignored():
-
     def foo(x, y):
         ignored(x, y)
-
     # Check that no error occurs for having used this.
     assert foo(3, 4) is None
 
@@ -47,13 +48,13 @@ def test_get_setting_from_context():
     with mock.patch.object(os, "environ", {'PIE_FLAVOR': 'cherry', 'PIE_COLOR': 'red'}):
 
         assert get_setting_from_context(sample_settings, ini_var='pie.flavor') == 'cherry'
-        assert get_setting_from_context(sample_settings, ini_var='pie.color') is 'red'
+        assert get_setting_from_context(sample_settings, ini_var='pie.color') == 'red'
 
         # Note that env_var=None means 'use default', not 'no env var'. You'd want env_var=False for 'no env var'.
         assert get_setting_from_context(sample_settings, ini_var='pie.flavor', env_var=None) == 'cherry'
-        assert get_setting_from_context(sample_settings, ini_var='pie.color', env_var=None) is 'red'
+        assert get_setting_from_context(sample_settings, ini_var='pie.color', env_var=None) == 'red'
 
-        assert get_setting_from_context(sample_settings, ini_var='pie.flavor', env_var=False) is 'apple'
+        assert get_setting_from_context(sample_settings, ini_var='pie.flavor', env_var=False) == 'apple'
         assert get_setting_from_context(sample_settings, ini_var='pie.color', env_var=False) is None
 
     with mock.patch.object(os, "environ", {'PIE_FLAVOR': '', 'PIE_COLOR': ''}):
@@ -176,8 +177,7 @@ def test_virtual_app_get():
                 'op': 'get',
                 'url': 'http://no.such.place/',
                 'kwargs': {'params': {'foo': 'bar'}},
-            }
-            ,
+            },
         ]
 
 
@@ -231,8 +231,7 @@ def test_virtual_app_post_json():
                 'url': 'http://no.such.place/',
                 'obj': {'alpha': 'omega'},
                 'kwargs': {'params': {'foo': 'bar'}},
-            }
-            ,
+            },
         ]
 
 
@@ -286,6 +285,5 @@ def test_virtual_app_patch_json():
                 'url': 'http://no.such.place/',
                 'obj': {'alpha': 'omega'},
                 'kwargs': {'params': {'foo': 'bar'}},
-            }
-            ,
+            },
         ]

test/test_qa_utils.py

@@ -3,11 +3,19 @@
 import pytest
 import pytz
 import re
+import subprocess
 import time
-import unittest
 import uuid
 
 from dcicutils.qa_utils import mock_not_called, local_attrs, override_environ, ControlledTime
+from unittest import mock
+
+# The following line needs to be separate from other imports. It is PART OF A TEST.
+from dcicutils.qa_utils import notice_pytest_fixtures   # Use care if editing this line. It is PART OF A TEST.
+from .fixtures.sample_fixtures import MockMathError, MockMath, math_enabled
+
+
+notice_pytest_fixtures(math_enabled)   # Use care if editing this line. It is PART OF A TEST.
 
 
 def test_mock_not_called():
@@ -22,17 +30,18 @@ def test_mock_not_called():
         raise AssertionError("An AssertionError was not raised.")
 
 
-def test_dynamic_properties():
+NORMAL_ATTR0 = 16
+NORMAL_ATTR1 = 17
+NORMAL_ATTR2 = 'foo'
+NORMAL_ATTR3 = 'bar'
+
+OVERRIDDEN_ATTR0 = 61
+OVERRIDDEN_ATTR1 = 71
+OVERRIDDEN_ATTR2 = 'oof'
+OVERRIDDEN_ATTR3 = 'rab'
 
-    NORMAL_ATTR0 = 16
-    NORMAL_ATTR1 = 17
-    NORMAL_ATTR2 = 'foo'
-    NORMAL_ATTR3 = 'bar'
 
-    OVERRIDDEN_ATTR0 = 61
-    OVERRIDDEN_ATTR1 = 71
-    OVERRIDDEN_ATTR2 = 'oof'
-    OVERRIDDEN_ATTR3 = 'rab'
+def test_dynamic_properties():
 
     def test_thing(test_obj):
 
@@ -41,7 +50,7 @@ def test_thing(test_obj):
         assert test_obj.attr2 == NORMAL_ATTR2
         assert test_obj.attr3 == NORMAL_ATTR3
 
-        attrs = ['attr0', 'attr1', 'attr2', 'attr3']
+        # attrs = ['attr0', 'attr1', 'attr2', 'attr3']
 
         # If this were done wrong, we'd bind an inherited attribute
         # and then when we put things back it would become an instance
@@ -103,6 +112,7 @@ def test_thing(test_obj):
     class Foo:
         attr0 = NORMAL_ATTR0
         attr1 = NORMAL_ATTR1
+
         def __init__(self):
             self.attr2 = NORMAL_ATTR2
             self.attr3 = NORMAL_ATTR3
@@ -149,11 +159,11 @@ def test_override_environ():
 
     with override_environ(**{unique_prop1: "something", unique_prop2: "anything"}):
 
-        assert unique_prop1 in os.environ # added
+        assert unique_prop1 in os.environ  # added
         value1a = os.environ.get(unique_prop1)
         assert value1a == "something"
 
-        assert unique_prop2 in os.environ # added
+        assert unique_prop2 in os.environ  # added
         value2a = os.environ.get(unique_prop2)
         assert value2a == "anything"
 
@@ -238,17 +248,17 @@ def test_controlled_time_now():
 
 def test_controlled_time_utcnow():
 
-    HOUR = 60 * 60  # 60 seconds * 60 minutes
+    hour = 60 * 60  # 60 seconds * 60 minutes
 
-    ET = pytz.timezone("US/Eastern")
+    eastern_time = pytz.timezone("US/Eastern")
     t0 = datetime.datetime(2020, 1, 1, 0, 0, 0)
-    t = ControlledTime(initial_time=t0, local_timezone=ET)
+    t = ControlledTime(initial_time=t0, local_timezone=eastern_time)
 
     t1 = t.now()     # initial time + 1 second
     t.set_datetime(t0)
     t2 = t.utcnow()  # initial time UTC + 1 second
     # US/Eastern on 2020-01-01 is not daylight time, so EST (-0500) not EDT (-0400).
-    assert (t2 - t1).total_seconds() == 5 * HOUR
+    assert (t2 - t1).total_seconds() == 5 * hour
 
 
 def test_controlled_time_reset_datetime():
@@ -298,8 +308,8 @@ def sleepy_function():
         time.sleep(10)
 
     dt = ControlledTime()
-    with unittest.mock.patch("datetime.datetime", dt):
-        with unittest.mock.patch("time.sleep", dt.sleep):
+    with mock.patch("datetime.datetime", dt):
+        with mock.patch("time.sleep", dt.sleep):
             t0 = datetime.datetime.now()
             sleepy_function()  # sleeps 10 seconds
             t1 = datetime.datetime.now()  # 1 more second increments
@@ -308,3 +318,59 @@ def sleepy_function():
     end_time = datetime.datetime.now()
     # In reality, whole test takes much less than one second...
     assert (end_time - start_time).total_seconds() < 0.5
+
+
+def test_notice_pytest_fixtures_part_1():
+
+    with pytest.raises(MockMathError):
+        MockMath.add(2, 2)
+
+
+def test_notice_pytest_fixtures_part_2(math_enabled):
+
+    notice_pytest_fixtures(math_enabled)  # Use care if editing this line. It is PART OF A TEST.
+
+    assert MockMath.add(2, 2) == 4
+
+
+THIS_TEST_FILE = __file__
+
+
+def test_notice_pytest_fixtures_part_3():
+
+    # This test will call out to a subprocess to check that this file passes flake8 tests.
+    # So please keep the file up-to-date. :)
+    # Then if it passes, it will filter out lines containing 'PART OF A TEST' (including this one)
+    # and show that their absence causes flake8 warnings.
+
+    line_filter_marker = '[P]ART OF A TEST'  # Using '[P]' instead of 'P' assures this line won't match.
+
+    def get_output(command):
+        print('command=')
+        print(command)
+        try:
+            code = 0
+            output = subprocess.check_output(["bash", "-c", command])
+        except subprocess.CalledProcessError as e:
+            code = e.returncode
+            output = e.output
+        output = output.decode('utf-8')
+        print("output=")
+        print(output)
+        return code, output
+
+    template = "cat '%s' %s | flake8 - --ignore=E303"  # ignore E303 (blank lines) caused by filtering
+
+    # This shows the file passes cleanly. If this fails, someone has let this file get sloppy. Fix that first.
+    code, output = get_output(template % (THIS_TEST_FILE, ""))
+    assert code == 0
+    assert output == ""
+
+    # This shows that if your remove the declaration, it leads to annoying errors from flake8 about fixtures.
+    declaration_usage_filter = "| sed '/%s/d' " % line_filter_marker
+    code, output = get_output(template % (THIS_TEST_FILE, declaration_usage_filter))
+    assert code == 1
+    warnings = output.strip().split('\n')
+    assert len(warnings) == 2  # a global warning about the import, and a local warning about a bound variable
+    for line in warnings:
+        assert "unused" in line  # allow for some variability in message wording, but should be about something unused