From af6e0cf42bf9f2f7d4408c5914ebf1b86568d1a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Palancher?= Date: Tue, 29 Jul 2025 15:50:43 +0200 Subject: [PATCH 1/3] Controller: split action_build() Refactor action_build() function in Controller module by moving part of its code in new function build_pkgs(). The purpose is to reduce complexity of action_build() and avoid reaching Pylint branching limit. --- lib/rift/Controller.py | 64 ++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/lib/rift/Controller.py b/lib/rift/Controller.py index 6a466fce..246a04fa 100644 --- a/lib/rift/Controller.py +++ b/lib/rift/Controller.py @@ -742,6 +742,38 @@ def action_vm(args, config): ret = vm_build(vm, args, config) return ret +def build_pkgs(config, args, pkgs, arch, results): + """Build a list of packages on a given architecture and report results.""" + 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) + def action_build(args, config): """Action for 'build' command.""" @@ -761,36 +793,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) + build_pkgs(config, args, pkgs, arch, results) if getattr(args, 'junit', False): logging.info('Writing test results in %s', args.junit) From 9833125f6b621e160755f883887ac3e6a5e2ebb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Palancher?= Date: Tue, 29 Jul 2025 17:24:24 +0200 Subject: [PATCH 2/3] TestResults: introduce extend() method This notably allows removing results argument from many functions in Controller. One motivation of this change is to satisfy pylint which emits warnings when there are more than 5 arguments on a function. A testcase is also introduced to cover all method of TestResults class. --- lib/rift/Controller.py | 89 +++++++++++++++++++++++----------------- lib/rift/TestResults.py | 9 ++++ tests/TestResults.py | 91 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 38 deletions(-) create mode 100644 tests/TestResults.py diff --git a/lib/rift/Controller.py b/lib/rift/Controller.py index 246a04fa..bf52f3d1 100644 --- a/lib/rift/Controller.py +++ b/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,20 +559,21 @@ 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) @@ -580,6 +581,8 @@ def validate_pkgs(config, args, results, pkgs, 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,8 +747,12 @@ def action_vm(args, config): ret = vm_build(vm, args, config) return ret -def build_pkgs(config, args, pkgs, arch, results): - """Build a list of packages on a given architecture and report results.""" +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() @@ -774,6 +783,8 @@ def build_pkgs(config, args, pkgs, arch, results): else: results.add_success(case, time.time() - now) + return results + def action_build(args, config): """Action for 'build' command.""" @@ -794,7 +805,7 @@ def action_build(args, config): for arch in config.get('arch'): pkgs = Package.list(config, staff, modules, args.packages) - build_pkgs(config, args, pkgs, arch, results) + results.extend(build_pkgs(config, args, pkgs, arch)) if getattr(args, 'junit', False): logging.info('Writing test results in %s', args.junit) @@ -825,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) @@ -856,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') @@ -894,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): diff --git a/lib/rift/TestResults.py b/lib/rift/TestResults.py index 73e79725..5fb60009 100644 --- a/lib/rift/TestResults.py +++ b/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. diff --git a/tests/TestResults.py b/tests/TestResults.py new file mode 100644 index 00000000..e64a34bf --- /dev/null +++ b/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""" + ) + ) From fe134a17eb34484bda80ff45a3a41e6bcfac0244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Palancher?= Date: Fri, 1 Aug 2025 09:47:37 +0200 Subject: [PATCH 3/3] CI: increase pylint score threshold Thanks to refactorings brought by some parent commits, the minimum expected Pylint score can be increased. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index f78d80be..9ed04d23 100644 --- a/pyproject.toml +++ b/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.