Merge pull request #6 from rezib/pr/refactor-pylint

TestResults and Controller refactoring to reduce functions complexity

Author: valeriyoann

lib/rift/Controller.py

@@ -481,9 +481,10 @@ def build_pkg(config, args, pkg, arch):
 
     mock.clean()
 
-def test_one_pkg(config, args, pkg, vm, arch, repos, results):
+def test_one_pkg(config, args, pkg, vm, arch, repos):
     """
-    Launch tests on a given package on a specific VM and a set of repositories.
+    Launch tests on a given package on a specific VM and a set of repositories
+    and return results.
     """
     message(f"Preparing {arch} test environment")
     _vm_start(vm)
@@ -495,7 +496,7 @@ def test_one_pkg(config, args, pkg, vm, arch, repos, results):
 
     banner(f"Starting tests of package {pkg.name} on architecture {arch}")
 
-    rc = 0
+    results = TestResults()
 
     tests = list(pkg.tests())
     if not args.noauto:
@@ -509,7 +510,6 @@ def test_one_pkg(config, args, pkg, vm, arch, repos, results):
             results.add_success(case, time.time() - now, out=proc.out, err=proc.err)
             message(f"Test '{case.fullname}' on architecture {arch}: OK")
         else:
-            rc = 1
             results.add_failure(case, time.time() - now, out=proc.out, err=proc.err)
             message(f"Test '{case.fullname}' on architecture {arch}: ERROR")
 
@@ -519,10 +519,10 @@ def test_one_pkg(config, args, pkg, vm, arch, repos, results):
         time.sleep(5)
         vm.stop()
 
-    return rc
+    return results
 
-def test_pkgs(config, args, results, pkgs, arch, extra_repos=None):
-    """Test a list of packages on a specific architecture."""
+def test_pkgs(config, args, pkgs, arch, extra_repos=None):
+    """Test a list of packages on a specific architecture and return results."""
 
     if extra_repos is None:
         extra_repos = []
@@ -533,7 +533,7 @@ def test_pkgs(config, args, results, pkgs, arch, extra_repos=None):
     if vm.running():
         raise RiftError('VM is already running')
 
-    rc = 0
+    results = TestResults()
 
     for pkg in pkgs:
 
@@ -559,27 +559,30 @@ def test_pkgs(config, args, results, pkgs, arch, extra_repos=None):
             continue
 
         pkg.load()
-        rc += test_one_pkg(config, args, pkg, vm, arch, repos, results)
+        results.extend(test_one_pkg(config, args, pkg, vm, arch, repos))
 
     if getattr(args, 'noquit', False):
         message("Not stopping the VM. Use: rift vm connect")
 
-    return rc
+    return results
 
-def validate_pkgs(config, args, results, pkgs, arch):
+
+def validate_pkgs(config, args, pkgs, arch):
     """
-    Validate packages on a specific architecture:
+    Validate packages on a specific architecture and return results:
         - rpmlint on specfile
         - check file patterns
         - build it
-        - lauch tests
+        - launch tests
     """
 
     repos = ProjectArchRepositories(config, arch)
 
     if args.publish and not repos.can_publish():
         raise RiftError("Cannot publish if 'working_repo' is undefined")
 
+    results = TestResults()
+
     for pkg in pkgs:
 
         case = TestCase('build', pkg.name, arch)
@@ -638,20 +641,20 @@ def validate_pkgs(config, args, results, pkgs, arch):
         mock.publish(staging)
         staging.update()
 
-        rc = 0
         if args.test:
-            rc = test_pkgs(
-                config,
-                args,
-                results,
-                [pkg],
-                arch,
-                [staging.consumables[arch]]
+            results.extend(
+                test_pkgs(
+                    config,
+                    args,
+                    [pkg],
+                    arch,
+                    [staging.consumables[arch]]
+                )
             )
 
         # Also publish on working repo if requested
         # XXX: All RPMs should be published when all of them have been validated
-        if rc == 0 and args.publish:
+        if results.global_result and args.publish:
             message("Publishing RPMS...")
             mock.publish(repos.working)
 
@@ -666,6 +669,8 @@ def validate_pkgs(config, args, results, pkgs, arch):
 
     banner(f"All packages checked on architecture {arch}")
 
+    return results
+
 def vm_build(vm, args, config):
     """Build VM image."""
     if not args.deploy and args.output is None:
@@ -742,6 +747,44 @@ def action_vm(args, config):
         ret = vm_build(vm, args, config)
     return ret
 
+def build_pkgs(config, args, pkgs, arch):
+    """
+    Build a list of packages on a given architecture and return results.
+    """
+    results = TestResults()
+
+    for pkg in pkgs:
+        case = TestCase('build', pkg.name, arch)
+        now = time.time()
+        try:
+            spec = Spec(pkg.specfile, config=config)
+        except RiftError as ex:
+            logging.error("Unable to load spec file: %s", str(ex))
+            results.add_failure(case, time.time() - now, err=str(ex))
+            continue  # skip current package
+
+        if not spec.supports_arch(arch):
+            logging.info(
+                "Skipping build on architecture %s not supported by "
+                "package %s",
+                arch,
+                pkg.name
+            )
+            continue
+
+        banner(f"Building package '{pkg.name}' for architecture {arch}")
+        now = time.time()
+        try:
+            pkg.load()
+            build_pkg(config, args, pkg, arch)
+        except RiftError as ex:
+            logging.error("Build failure: %s", str(ex))
+            results.add_failure(case, time.time() - now, err=str(ex))
+        else:
+            results.add_success(case, time.time() - now)
+
+    return results
+
 def action_build(args, config):
     """Action for 'build' command."""
 
@@ -761,36 +804,8 @@ def action_build(args, config):
     # Build all packages for all project supported architectures
     for arch in config.get('arch'):
 
-        for pkg in Package.list(config, staff, modules, args.packages):
-
-            case = TestCase('build', pkg.name, arch)
-            now = time.time()
-            try:
-                spec = Spec(pkg.specfile, config=config)
-            except RiftError as ex:
-                logging.error("Unable to load spec file: %s", str(ex))
-                results.add_failure(case, time.time() - now, err=str(ex))
-                continue  # skip current package
-
-            if not spec.supports_arch(arch):
-                logging.info(
-                    "Skipping build on architecture %s not supported by "
-                    "package %s",
-                    arch,
-                    pkg.name
-                )
-                continue
-
-            banner(f"Building package '{pkg.name}' for architecture {arch}")
-            now = time.time()
-            try:
-                pkg.load()
-                build_pkg(config, args, pkg, arch)
-            except RiftError as ex:
-                logging.error("Build failure: %s", str(ex))
-                results.add_failure(case, time.time() - now, err=str(ex))
-            else:
-                results.add_success(case, time.time() - now)
+        pkgs = Package.list(config, staff, modules, args.packages)
+        results.extend(build_pkgs(config, args, pkgs, arch))
 
         if getattr(args, 'junit', False):
             logging.info('Writing test results in %s', args.junit)
@@ -821,12 +836,13 @@ def action_test(args, config):
     results = TestResults('test')
     # Test package on all project supported architectures
     for arch in config.get('arch'):
-        test_pkgs(
-            config,
-            args,
-            results,
-            Package.list(config, staff, modules, args.packages),
-            arch
+        results.extend(
+            test_pkgs(
+                config,
+                args,
+                Package.list(config, staff, modules, args.packages),
+                arch
+            )
         )
     if getattr(args, 'junit', False):
         logging.info('Writing test results in %s', args.junit)
@@ -852,12 +868,13 @@ def action_validate(args, config):
     results = TestResults('validate')
     # Validate packages on all project supported architectures
     for arch in config.get('arch'):
-        validate_pkgs(
-            config,
-            args,
-            results,
-            Package.list(config, staff, modules, args.packages),
-            arch
+        results.extend(
+            validate_pkgs(
+                config,
+                args,
+                Package.list(config, staff, modules, args.packages),
+                arch
+            )
         )
     banner('All packages checked on all architectures')
 
@@ -890,7 +907,7 @@ def action_validdiff(args, config):
     # Re-validate all updated packages for all architectures supported by the
     # project.
     for arch in config.get('arch'):
-        validate_pkgs(config, args, results, updated.values(), arch)
+        results.extend(validate_pkgs(config, args, updated.values(), arch))
 
 
     if getattr(args, 'junit', False):

lib/rift/TestResults.py

@@ -137,6 +137,15 @@ def _add_result(self, result):
         """
         self.results.append(result)
 
+    def extend(self, other):
+        """
+        Extend TestResults with all TestResult from the other TestResults.
+        """
+        for result in other.results:
+            self.results.append(result)
+            if result.value == 'Failure':
+                self.global_result = False
+
     def junit(self, filename):
         """
         Generate a junit xml file containing all tests results.

pyproject.toml

@@ -8,7 +8,7 @@ addopts = "-v --cov=rift --cov-report=term-missing"
 
 [tool.pylint.main]
 # Specify a score threshold under which the program will exit with error.
-fail-under = 9.74
+fail-under = 9.80
 
 # Minimum Python version to use for version dependent checks. Will default to the
 # version used to run pylint.

tests/TestResults.py

@@ -0,0 +1,91 @@
+#
+# Copyright (C) 2025 CEA
+#
+import textwrap
+import xml.etree.ElementTree as ET
+from io import BytesIO
+
+from TestUtils import RiftTestCase
+from rift.TestResults import TestResults, TestCase
+
+class TestResultsTest(RiftTestCase):
+    """
+    Tests class for TestResults
+    """
+
+    def test_init(self):
+        """ TestResults instance """
+        results = TestResults()
+
+        self.assertCountEqual(results.results, [])
+        self.assertTrue(results.global_result)
+        self.assertEqual(len(results), 0)
+
+    def test_add_success(self):
+        results = TestResults()
+        results.add_success(TestCase('test1', 'pkg', 'x86_64'), 1)
+        results.add_success(TestCase('test2', 'pkg', 'x86_64'), 1)
+        self.assertTrue(results.global_result)
+        self.assertEqual(len(results), 2)
+
+    def test_add_failure(self):
+        results = TestResults()
+        results.add_failure(TestCase('test1', 'pkg', 'x86_64'), 1)
+        results.add_failure(TestCase('test2', 'pkg', 'x86_64'), 1)
+        self.assertFalse(results.global_result)
+        self.assertEqual(len(results), 2)
+
+    def test_add_success_failure(self):
+        results = TestResults()
+        results.add_success(TestCase('test1', 'pkg', 'x86_64'), 1)
+        results.add_failure(TestCase('test2', 'pkg', 'x86_64'), 1)
+        self.assertFalse(results.global_result)
+        self.assertEqual(len(results), 2)
+
+    def test_extend(self):
+        results = TestResults()
+        results.add_success(TestCase('test1', 'pkg1', 'x86_64'), 1)
+        results.add_failure(TestCase('test2', 'pkg1', 'x86_64'), 1)
+        others = TestResults()
+        others.add_success(TestCase('test1', 'pkg2', 'x86_64'), 1)
+        others.add_failure(TestCase('test2', 'pkg2', 'x86_64'), 1)
+        results.extend(others)
+        self.assertFalse(results.global_result)
+        self.assertEqual(len(results), 4)
+
+    def test_junit(self):
+        results = TestResults()
+        results.add_success(TestCase('test1', 'pkg', 'x86_64'), 1, out="output test1")
+        results.add_failure(TestCase('test2', 'pkg', 'x86_64'), 1, out="output test2")
+        output = BytesIO()
+        results.junit(output)
+        root = ET.fromstring(output.getvalue().decode())
+        self.assertEqual(root.tag, 'testsuite')
+        self.assertEqual(root.attrib, { 'tests': '2'})
+        self.assertEqual(len(root.findall('*')), 2)
+        for element in root:
+            self.assertEqual(element.tag, 'testcase')
+            self.assertIn(element.attrib['name'], ['test1', 'test2'])
+            self.assertEqual(element.attrib['classname'], 'rift.pkg')
+            self.assertEqual(element.attrib['time'], '1.00')
+            if element.attrib['name'] == 'test1':
+                self.assertIsNone(element.find('failure'))
+                self.assertEqual(element.find('system-out').text, 'output test1')
+            else:
+                self.assertIsNotNone(element.find('failure'))
+                self.assertEqual(element.find('system-out').text, 'output test2')
+
+    def test_summary(self):
+        results = TestResults()
+        results.add_success(TestCase('test1', 'pkg', 'x86_64'), 1)
+        results.add_failure(TestCase('test2', 'pkg', 'x86_64'), 1)
+        self.assertEqual(
+            results.summary(),
+            textwrap.dedent(
+                """\
+                NAME      ARCH   DURATION RESULT
+                ----      ----   -------- ------
+                pkg.test1 x86_64       1s Success
+                pkg.test2 x86_64       1s FAILURE"""
+            )
+        )