diff mbox series

[03/11] patchtest: tests: cleanup test suites

Message ID 20260514194207.1958325-4-tgamblin@baylibre.com
State New
Headers show
Series patchtest: improve testing coverage | expand

Commit Message

Trevor Gamblin May 14, 2026, 7:41 p.m. UTC
Do a sweep of the test suites to remove redundant code, make variable
names more explicit, and fix some formatting. Some logic (such as the
bits for checking valid Upstream-Status tags) are also moved directly to
the relevant functions, instead of being left in the setup stage. This
should make maintenance easier going forward.

AI-Generated: Uses Claude Code

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 meta/lib/patchtest/tests/base.py              |  4 ---
 meta/lib/patchtest/tests/test_mbox.py         | 10 +++---
 meta/lib/patchtest/tests/test_metadata.py     | 32 ++++++------------
 meta/lib/patchtest/tests/test_patch.py        | 33 ++++++++-----------
 .../lib/patchtest/tests/test_python_pylint.py |  8 ++---
 5 files changed, 33 insertions(+), 54 deletions(-)
diff mbox series

Patch

diff --git a/meta/lib/patchtest/tests/base.py b/meta/lib/patchtest/tests/base.py
index 919ca136bb..8263932dda 100644
--- a/meta/lib/patchtest/tests/base.py
+++ b/meta/lib/patchtest/tests/base.py
@@ -22,10 +22,6 @@  info = logger.info
 warn = logger.warn
 error = logger.error
 
-Commit = collections.namedtuple(
-    "Commit", ["author", "subject", "commit_message", "shortlog", "payload"]
-)
-
 Commit = collections.namedtuple('Commit', ['author', 'subject', 'commit_message', 'shortlog', 'payload'])
 
 class PatchtestOEError(Exception):
diff --git a/meta/lib/patchtest/tests/test_mbox.py b/meta/lib/patchtest/tests/test_mbox.py
index 55d1068df3..f158f4472c 100644
--- a/meta/lib/patchtest/tests/test_mbox.py
+++ b/meta/lib/patchtest/tests/test_mbox.py
@@ -65,11 +65,11 @@  class TestMbox(base.Base):
             shortlog = re.sub(r'^(\[.*?\])+ ', '', commit.shortlog)
             if shortlog.startswith('Revert "'):
                 continue
-            l = len(shortlog)
-            if l > patchtest_patterns.mbox_shortlog_maxlength:
+            length = len(shortlog)
+            if length > patchtest_patterns.mbox_shortlog_maxlength:
                 self.fail(
                     "Edit shortlog so that it is %d characters or less (currently %d characters)"
-                    % (patchtest_patterns.mbox_shortlog_maxlength, l),
+                    % (patchtest_patterns.mbox_shortlog_maxlength, length),
                     commit=commit,
                 )
 
@@ -108,7 +108,7 @@  class TestMbox(base.Base):
             folders = patch.path.split('/')
             base_path = folders[0]
             for project in [self.bitbake, self.doc, self.oe, self.poky]:
-                if base_path in  project.paths:
+                if base_path in project.paths:
                     self.fail('Series sent to the wrong mailing list or some patches from the series correspond to different mailing lists',
                               data=[('Suggested ML', '%s [%s]' % (project.listemail, project.gitrepo)),
                                     ('Patch\'s path:', patch.path)])
@@ -161,7 +161,7 @@  class TestMbox(base.Base):
         for commit in self.commits:
             if patchtest_patterns.auh_email in commit.commit_message:
                 self.fail(
-                    "Invalid author %s. Resend the series with a valid patch author"
+                    "Commit message contains AUH email address %s. Resend the series with a valid patch author"
                     % patchtest_patterns.auh_email,
                     commit=commit,
                 )
diff --git a/meta/lib/patchtest/tests/test_metadata.py b/meta/lib/patchtest/tests/test_metadata.py
index 30da8dbe60..442e80973d 100644
--- a/meta/lib/patchtest/tests/test_metadata.py
+++ b/meta/lib/patchtest/tests/test_metadata.py
@@ -8,7 +8,6 @@  import base
 import collections
 import os
 import patchtest_patterns
-import pyparsing
 from patchtest_parser import PatchtestParser
 
 # Data store commonly used to share values between pre and post-merge tests
@@ -108,39 +107,28 @@  class TestMetadata(base.Metadata):
                             ],
                         )
 
-    def pretest_src_uri_left_files(self):
-        # these tests just make sense on patches that can be merged
-        if not PatchtestParser.repo.canbemerged():
-            self.skip("Patch cannot be merged")
-        if not self.modified:
-            self.skip('No modified recipes, skipping pretest')
-
-        # get the proper metadata values
+    def _collect_src_uri(self, key_prefix):
         for pn in self.modified:
-            # we are not interested in images
             if 'core-image' in pn:
                 continue
             rd = self.tinfoil.parse_recipe(pn)
             PatchTestDataStore[
-                "%s-%s-%s" % (self.shortid(), patchtest_patterns.metadata_src_uri, pn)
+                "%s-%s-%s" % (key_prefix, patchtest_patterns.metadata_src_uri, pn)
             ] = rd.getVar(patchtest_patterns.metadata_src_uri)
 
-    def test_src_uri_left_files(self):
-        # these tests just make sense on patches that can be merged
+    def pretest_src_uri_left_files(self):
         if not PatchtestParser.repo.canbemerged():
             self.skip("Patch cannot be merged")
         if not self.modified:
             self.skip('No modified recipes, skipping pretest')
+        self._collect_src_uri(self.shortid())
 
-        # get the proper metadata values
-        for pn in self.modified:
-            # we are not interested in images
-            if 'core-image' in pn:
-                continue
-            rd = self.tinfoil.parse_recipe(pn)
-            PatchTestDataStore[
-                "%s-%s-%s" % (self.shortid(), patchtest_patterns.metadata_src_uri, pn)
-            ] = rd.getVar(patchtest_patterns.metadata_src_uri)
+    def test_src_uri_left_files(self):
+        if not PatchtestParser.repo.canbemerged():
+            self.skip("Patch cannot be merged")
+        if not self.modified:
+            self.skip('No modified recipes, skipping test')
+        self._collect_src_uri(self.shortid())
 
         for pn in self.modified:
             pretest_src_uri = PatchTestDataStore[
diff --git a/meta/lib/patchtest/tests/test_patch.py b/meta/lib/patchtest/tests/test_patch.py
index 3b33b5a199..9f1d7d2c8f 100644
--- a/meta/lib/patchtest/tests/test_patch.py
+++ b/meta/lib/patchtest/tests/test_patch.py
@@ -28,21 +28,16 @@  class TestPatch(base.Base):
     def setUp(self):
         if self.unidiff_parse_error:
             self.skip('Parse error %s' % self.unidiff_parse_error)
-
-        self.valid_status = ", ".join(patchtest_patterns.upstream_status_nonliteral_valid_status)
-        self.standard_format = "Upstream-Status: <Valid status>"
-
-        # we are just interested in series that introduce CVE patches, thus discard other
-        # possibilities: modification to current CVEs, patch directly introduced into the
-        # recipe, upgrades already including the CVE, etc.
-        new_cves = [p for p in self.patchset if p.path.endswith('.patch') and p.is_added_file]
-        if not new_cves:
+        if not TestPatch.newpatches:
             self.skip('No new CVE patches introduced')
 
     def test_upstream_status_presence_format(self):
         if not TestPatch.newpatches:
             self.skip("There are no new software patches, no reason to test Upstream-Status presence/format")
 
+        valid_status = ", ".join(patchtest_patterns.upstream_status_nonliteral_valid_status)
+        standard_format = "Upstream-Status: <Valid status>"
+
         for newpatch in TestPatch.newpatches:
             payload = newpatch.__str__()
             lines = payload.splitlines()
@@ -71,16 +66,16 @@  class TestPatch(base.Base):
                         'Upstream-Status is present only after the patch scissors. '
                         "It must be placed in the patch header before the scissors line.",
                         data=[
-                            ("Standard format", self.standard_format),
-                            ("Valid status", self.valid_status),
+                            ("Standard format", standard_format),
+                            ("Valid status", valid_status),
                         ],
                     )
                 else:
                     self.fail(
                         "Added patch file is missing Upstream-Status: <Valid status> in the commit message",
                         data=[
-                            ("Standard format", self.standard_format),
-                            ("Valid status", self.valid_status),
+                            ("Standard format", standard_format),
+                            ("Valid status", valid_status),
                         ],
                     )
 
@@ -97,8 +92,8 @@  class TestPatch(base.Base):
                         "but was found afterwards.",
                         data=[
                             ("Current", line.lstrip("+")),
-                            ("Standard format", self.standard_format),
-                            ("Valid status", self.valid_status),
+                            ("Standard format", standard_format),
+                            ("Valid status", valid_status),
                         ],
                     )
 
@@ -142,14 +137,14 @@  class TestPatch(base.Base):
                             "Upstream-Status is in incorrect format",
                             data=[
                                 ("Current", pe.pstr),
-                                ("Standard format", self.standard_format),
-                                ("Valid status", self.valid_status),
+                                ("Standard format", standard_format),
+                                ("Valid status", valid_status),
                             ],
                         )
 
     def test_signed_off_by_presence(self):
         if not TestPatch.newpatches:
-            self.skip("There are no new software patches, no reason to test %s presence" % PatchSignedOffBy.mark)
+            self.skip("There are no new software patches, no reason to test %s presence" % TestPatch.mark)
 
         for newpatch in TestPatch.newpatches:
             payload = newpatch.__str__()
@@ -162,7 +157,7 @@  class TestPatch(base.Base):
                 self.fail('A patch file has been added without a Signed-off-by tag: \'%s\'' % os.path.basename(newpatch.path))
 
     def test_cve_tag_format(self):
-        for commit in TestPatch.commits:
+        for commit in self.commits:
             if patchtest_patterns.cve.search_string(
                 commit.shortlog
             ) or patchtest_patterns.cve.search_string(commit.commit_message):
diff --git a/meta/lib/patchtest/tests/test_python_pylint.py b/meta/lib/patchtest/tests/test_python_pylint.py
index ec9129bc79..37d1de9436 100644
--- a/meta/lib/patchtest/tests/test_python_pylint.py
+++ b/meta/lib/patchtest/tests/test_python_pylint.py
@@ -47,7 +47,7 @@  class PyLint(base.Base):
         for pythonpatch in self.pythonpatches:
             # a condition checking whether a file is renamed or not
             # unidiff doesn't support this yet
-            if pythonpatch.target_file is not pythonpatch.path:
+            if pythonpatch.target_file != pythonpatch.path:
                 path = pythonpatch.target_file[2:]
             else:
                 path = pythonpatch.path
@@ -55,9 +55,9 @@  class PyLint(base.Base):
             reporter = TextReporter(pylint_output)
             lint.Run([self.pylint_options, pythonpatch.path], reporter=reporter, exit=False)
             for line in pylint_output.readlines():
-                    if not '*' in line:
-                        if line.strip():
-                            self.pylint_test[line.strip().split(' ',1)[0]] = line.strip().split(' ',1)[1]
+                if not '*' in line:
+                    if line.strip():
+                        self.pylint_test[line.strip().split(' ',1)[0]] = line.strip().split(' ',1)[1]
 
         for issue in self.pylint_test:
              if self.pylint_test[issue] not in self.pylint_pretest.values():