Fix backport checks

In change I85501b4bff97d1c1e1873c3329ef998a8e501134, we added the
ability for stdlib backport-style libraries to have different version
specifiers (i.e. 'python_version<=3.9') from what we define in our
global requirements file. At least, that was the intention. However, we
used a regex that wouldn't actually work since it was checking for
Python versions greater than or equal to rather than the inverse. Fix
the regex, adding tests in the process (like we should have done last
time) to prove that things actually work now.

While we're here, add some whitespace and test docstrings to make the
whole thing a little more readable.

Change-Id: I6a8566d086761148d2e7a8e8f8288b2b0e447d08
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

openstack_requirements/check.py

@@ -115,17 +115,20 @@ def _is_requirement_in_global_reqs(
                 # likewise, if a package is one of the backport packages then
                 # we're okay with a potential marker (e.g. if a package
                 # requires a feature that is only available in a newer Python
-                # library, while other packages are happy without this feeature
+                # library, while other packages are happy without this feature
                 if (
-                    allow_3_only and
                     matching and
                     aname == 'markers' and
                     local_req.package in backports
                 ):
-                    if (
-                        PY3_SPECIFIER_RE.match(global_req_val) or
-                        PY3_SPECIFIER_RE.match(local_req_val)
+                    if re.match(
+                        r'python_version(==|<=|<)[\'"]3\.\d+[\'"]',
+                        local_req_val,
                     ):
+                        print(
+                            'Ignoring backport package with python_version '
+                            'marker'
+                        )
                         continue
 
                 print(f'WARNING: possible mismatch found for package "{local_req.package}"')  # noqa: E501
@@ -212,9 +215,11 @@ def _validate_one(
         # by project teams as they see fit, so no further
         # testing is needed.
         return False
+
     if name not in global_reqs:
         print("ERROR: Requirement '%s' not in openstack/requirements" % reqs)
         return True
+
     counts = {}
     for req in reqs:
         if req.extras:
@@ -222,16 +227,19 @@ def _validate_one(
                 counts[extra] = counts.get(extra, 0) + 1
         else:
             counts[''] = counts.get('', 0) + 1
+
         if not _is_requirement_in_global_reqs(
             req, global_reqs[name], backports, allow_3_only,
         ):
             return True
+
         # check for minimum being defined
         min = [s for s in req.specifiers.split(',') if '>' in s]
         if not min:
             print("ERROR: Requirement for package '%s' has no lower bound" %
                   name)
             return True
+
     for extra, count in counts.items():
         # Make sure the number of entries matches. If allow_3_only, then we
         # just need to make sure we have at least the number of entries for
@@ -253,6 +261,7 @@ def _validate_one(
                       ('[%s]' % extra) if extra else '',
                       len(global_reqs[name])))
             return True
+
     return False
 
 

openstack_requirements/tests/test_check.py

@@ -22,11 +22,11 @@ import testtools
 class TestIsReqInGlobalReqs(testtools.TestCase):
 
     def setUp(self):
-        super(TestIsReqInGlobalReqs, self).setUp()
+        super().setUp()
 
         self._stdout_fixture = fixtures.StringStream('stdout')
         self.stdout = self.useFixture(self._stdout_fixture).stream
-        self.backports = dict()
+        self.backports = set()
         self.useFixture(fixtures.MonkeyPatch('sys.stdout', self.stdout))
 
         self.global_reqs = check.get_global_reqs(textwrap.dedent("""
@@ -34,9 +34,9 @@ class TestIsReqInGlobalReqs(testtools.TestCase):
         withmarker>=1.5;python_version=='3.5'
         withmarker>=1.2,!=1.4;python_version=='2.7'
         """))
-        print('global_reqs', self.global_reqs)
 
     def test_match(self):
+        """Test a basic package."""
         req = requirement.parse('name>=1.2,!=1.4')['name'][0][0]
         self.assertTrue(
             check._is_requirement_in_global_reqs(
@@ -47,6 +47,7 @@ class TestIsReqInGlobalReqs(testtools.TestCase):
         )
 
     def test_match_with_markers(self):
+        """Test a package specified with python 3 markers."""
         req = requirement.parse(textwrap.dedent("""
         withmarker>=1.5;python_version=='3.5'
         """))['withmarker'][0][0]
@@ -59,6 +60,11 @@ class TestIsReqInGlobalReqs(testtools.TestCase):
         )
 
     def test_match_without_python3_markers(self):
+        """Test a package specified without python 3 markers.
+
+        Python 3 packages are a thing. On those, it's totally unnecessary to
+        specify e.g. a "python_version>'3" marker for packages.
+        """
         req = requirement.parse(textwrap.dedent("""
         withmarker>=1.5'
         """))['withmarker'][0][0]
@@ -71,7 +77,26 @@ class TestIsReqInGlobalReqs(testtools.TestCase):
             )
         )
 
+    def test_backport(self):
+        """Test a stdlib backport pacakge.
+
+        The python_version marker should be ignored for stdlib backport-type
+        packages.
+        """
+        req = requirement.parse("name;python_version<'3.9'")['name'][0][0]
+        self.assertTrue(
+            check._is_requirement_in_global_reqs(
+                req,
+                self.global_reqs['name'],
+                {'name'},
+            )
+        )
+
     def test_name_mismatch(self):
+        """Test a mismatch in package names.
+
+        Obviously a package with a different name is not the same thing.
+        """
         req = requirement.parse('wrongname>=1.2,!=1.4')['wrongname'][0][0]
         self.assertFalse(
             check._is_requirement_in_global_reqs(
@@ -81,7 +106,28 @@ class TestIsReqInGlobalReqs(testtools.TestCase):
             )
         )
 
+    def test_marker_mismatch(self):
+        """Test a mismatch in markers.
+
+        This should be a failure since the only marker we allow to be different
+        is the python_version marker.
+        """
+        req = requirement.parse("name; sys_platform == 'win32'")['name'][0][0]
+        self.assertFalse(
+            check._is_requirement_in_global_reqs(
+                req,
+                self.global_reqs['name'],
+                self.backports,
+            )
+        )
+
     def test_min_mismatch(self):
+        """Test a mismatch in minimum version.
+
+        We actually allow this since we only enforce a common upper constraint.
+        Packages can specify whatever minimum they like so long as it doesn't
+        exceed the upper-constraint value.
+        """
         req = requirement.parse('name>=1.3,!=1.4')['name'][0][0]
         self.assertTrue(
             check._is_requirement_in_global_reqs(
@@ -92,6 +138,11 @@ class TestIsReqInGlobalReqs(testtools.TestCase):
         )
 
     def test_extra_exclusion(self):
+        """Test that we validate exclusions.
+
+        A package can't exclude a version unless that is also excluded in
+        global requirements.
+        """
         req = requirement.parse('name>=1.2,!=1.4,!=1.5')['name'][0][0]
         self.assertFalse(
             check._is_requirement_in_global_reqs(
@@ -102,6 +153,10 @@ class TestIsReqInGlobalReqs(testtools.TestCase):
         )
 
     def test_missing_exclusion(self):
+        """Test that we ignore missing exclusions.
+
+        A package can specify fewer exclusions than global requirements.
+        """
         req = requirement.parse('name>=1.2')['name'][0][0]
         self.assertTrue(
             check._is_requirement_in_global_reqs(