PR#1739: kiwibuild: Tests and minor fixes - pungi

pungi

#1739 kiwibuild: Tests and minor fixes

Merged a year ago by lsedlar. Opened a year ago by lsedlar.

lsedlar/pungi kiwibuild-tests into master

kiwibuild: Add tests for the basic functionality

Lubomír Sedlář • a year ago

f15b8e0

kiwibuild: Remove repos as dicts

Lubomír Sedlář • a year ago

442bb3c

Fix additional image metadata

Lubomír Sedlář • a year ago

8c5c929

Drop kiwibuild_version option

Lubomír Sedlář • a year ago

0114466

pungi/checks.py

file modified

-1

		`@@ -1250,7 +1250,6 @@`
		`},`
		`"kiwibuild_target": {"type": "string"},`
		`"kiwibuild_release": {"$ref": "#/definitions/optional_string"},`
		`- "kiwibuild_version": {"type": "string"},`
		`"osbuild_target": {"type": "string"},`
		`"osbuild_release": {"$ref": "#/definitions/optional_string"},`
		`"osbuild_version": {"type": "string"},`

pungi/phases/kiwibuild.py

file modified

+5 -18

		`@@ -36,23 +36,10 @@`
		`resolved_repos = []`

		`for repo in repos:`
		`- if isinstance(repo, dict):`
		`- try:`
		`- url = repo["baseurl"]`
		`- except KeyError:`
		`- raise RuntimeError(`
		- "`baseurl` is required in repo dict %s" % str(repo)
		`- )`
		`- url = util.get_repo_url(compose, url, arch=arch)`
		`- if url is None:`
		`- raise RuntimeError("Failed to resolve repo URL for %s" % str(repo))`
		`- repo["baseurl"] = url`
		`- resolved_repos.append(repo)`
		`- else:`
		`- repo = util.get_repo_url(compose, repo, arch=arch)`
		`- if repo is None:`
		`- raise RuntimeError("Failed to resolve repo URL for %s" % repo)`
		`- resolved_repos.append(repo)`
		`+ repo = util.get_repo_url(compose, repo, arch=arch)`
		`+ if repo is None:`
		`+ raise RuntimeError("Failed to resolve repo URL for %s" % repo)`
		`+ resolved_repos.append(repo)`

		`return resolved_repos`

		`@@ -212,7 +199,7 @@`
		`img.disc_count = 1`
		`img.bootable = False`
		`img.subvariant = config.get("subvariant", variant.uid)`
		`- setattr(img, "can_fail", self.can_fail)`
		`+ setattr(img, "can_fail", arch in self.failable_arches)`
		`setattr(img, "deliverable", "kiwibuild")`
		`compose.im.add(variant=variant.uid, arch=arch, image=img)`

tests/test_kiwibuildphase.py

file added

+380

		`@@ -0,0 +1,380 @@`
		`+ import os`
		`+`
		`+ try:`
		`+ from unittest import mock`
		`+ except ImportError:`
		`+ import mock`
		`+`
		`+ from pungi.phases.kiwibuild import KiwiBuildPhase, RunKiwiBuildThread`
		`+ from tests.helpers import DummyCompose, PungiTestCase`
		`+`
		`+`
		`+ MINIMAL_CONF = {`
		`+ "description_scm": "https://example.com/kiwi.git",`
		`+ "description_path": "Fedora.kiwi",`
		`+ "kiwi_profile": "Cloud-Base-Generic",`
		`+ }`
		`+`
		`+`
		`+ def _merge(a, b):`
		`+ """This would be a \| b on 3.9 and later, or {a, b} or 3.5 and later."""`
		`+ c = a.copy()`
		`+ c.update(b)`
		`+ return c`
		`+`
		`+`
		`+ @mock.patch("pungi.phases.kiwibuild.ThreadPool")`
		`+ class TestKiwiBuildPhase(PungiTestCase):`
		`+ def test_minimal(self, ThreadPool):`
		`+ cfg = _merge({"target": "f40"}, MINIMAL_CONF)`
		`+ compose = DummyCompose(self.topdir, {"kiwibuild": {"^Server$": [cfg]}})`
		`+`
		`+ self.assertValidConfig(compose.conf)`
		`+`
		`+ phase = KiwiBuildPhase(compose)`
		`+`
		`+ phase.run()`
		`+ phase.pool.add.assert_called()`
		`+ assert phase.pool.queue_put.call_args_list == [`
		`+ mock.call(`
		`+ (`
		`+ compose,`
		`+ compose.variants["Server"],`
		`+ cfg,`
		`+ ["amd64", "x86_64"],`
		`+ None, # release`
		`+ "f40",`
		`+ [self.topdir + "/compose/Server/$arch/os"],`
		`+ [], # failable arches`
		`+ )`
		`+ )`
		`+ ]`
		`+`
		`+ def test_full(self, ThreadPool):`
		`+ cfg = _merge(`
		`+ {`
		`+ "target": "f40",`
		`+ "release": "1234",`
		`+ "arches": ["x86_64"],`
		`+ "repos": ["https://example.com/repo/", "Client"],`
		`+ "failable": ["*"],`
		`+ "subvariant": "Test",`
		`+ },`
		`+ MINIMAL_CONF,`
		`+ )`
		`+ compose = DummyCompose(self.topdir, {"kiwibuild": {"^Server$": [cfg]}})`
		`+`
		`+ self.assertValidConfig(compose.conf)`
		`+`
		`+ phase = KiwiBuildPhase(compose)`
		`+`
		`+ phase.run()`
		`+ phase.pool.add.assert_called()`
		`+ assert phase.pool.queue_put.call_args_list == [`
		`+ mock.call(`
		`+ (`
		`+ compose,`
		`+ compose.variants["Server"],`
		`+ cfg,`
		`+ ["x86_64"],`
		`+ "1234",`
		`+ "f40",`
		`+ [`
		`+ "https://example.com/repo/",`
		`+ self.topdir + "/compose/Client/$arch/os",`
		`+ self.topdir + "/compose/Server/$arch/os",`
		`+ ],`
		`+ ["x86_64"],`
		`+ )`
		`+ )`
		`+ ]`
		`+`
		`+ def test_failable(self, ThreadPool):`
		`+ cfg = _merge({"target": "f40", "failable": ["x86_64"]}, MINIMAL_CONF)`
		`+ compose = DummyCompose(self.topdir, {"kiwibuild": {"^Server$": [cfg]}})`
		`+`
		`+ self.assertValidConfig(compose.conf)`
		`+`
		`+ phase = KiwiBuildPhase(compose)`
		`+`
		`+ phase.run()`
		`+ phase.pool.add.assert_called()`
		`+ assert phase.pool.queue_put.call_args_list == [`
		`+ mock.call(`
		`+ (`
		`+ compose,`
		`+ compose.variants["Server"],`
		`+ cfg,`
		`+ ["amd64", "x86_64"],`
		`+ None, # release`
		`+ "f40",`
		`+ [self.topdir + "/compose/Server/$arch/os"],`
		`+ ["x86_64"], # failable arches`
		`+ )`
		`+ )`
		`+ ]`
		`+`
		`+ def test_with_phase_opts(self, ThreadPool):`
		`+ cfg = MINIMAL_CONF`
		`+ compose = DummyCompose(`
		`+ self.topdir,`
		`+ {`
		`+ "kiwibuild": {"^Server$": [cfg]},`
		`+ "kiwibuild_target": "f40",`
		`+ "kiwibuild_release": "1234",`
		`+ },`
		`+ )`
		`+`
		`+ self.assertValidConfig(compose.conf)`
		`+`
		`+ phase = KiwiBuildPhase(compose)`
		`+`
		`+ phase.run()`
		`+ phase.pool.add.assert_called()`
		`+ assert phase.pool.queue_put.call_args_list == [`
		`+ mock.call(`
		`+ (`
		`+ compose,`
		`+ compose.variants["Server"],`
		`+ cfg,`
		`+ ["amd64", "x86_64"],`
		`+ "1234", # release`
		`+ "f40",`
		`+ [self.topdir + "/compose/Server/$arch/os"],`
		`+ [], # failable arches`
		`+ )`
		`+ )`
		`+ ]`
		`+`
		`+ def test_with_global_opts(self, ThreadPool):`
		`+ cfg = MINIMAL_CONF`
		`+ compose = DummyCompose(`
		`+ self.topdir,`
		`+ {`
		`+ "kiwibuild": {"^Server$": [cfg]},`
		`+ "global_target": "f40",`
		`+ "global_release": "1234",`
		`+ },`
		`+ )`
		`+`
		`+ self.assertValidConfig(compose.conf)`
		`+`
		`+ phase = KiwiBuildPhase(compose)`
		`+`
		`+ phase.run()`
		`+ phase.pool.add.assert_called()`
		`+ assert phase.pool.queue_put.call_args_list == [`
		`+ mock.call(`
		`+ (`
		`+ compose,`
		`+ compose.variants["Server"],`
		`+ cfg,`
		`+ ["amd64", "x86_64"],`
		`+ "1234", # release`
		`+ "f40",`
		`+ [self.topdir + "/compose/Server/$arch/os"],`
		`+ [], # failable arches`
		`+ )`
		`+ )`
		`+ ]`
		`+`
		`+`
		`+ @mock.patch("pungi.phases.kiwibuild.Linker")`
		`+ @mock.patch("pungi.util.get_mtime")`
		`+ @mock.patch("pungi.util.get_file_size")`
		`+ @mock.patch("pungi.wrappers.kojiwrapper.KojiWrapper")`
		`+ class TestKiwiBuildThread(PungiTestCase):`
		`+ def _img_path(self, arch, filename=None):`
		`+ path = self.topdir + "/compose/Server/%s/images" % arch`
		`+ if filename:`
		`+ path += "/" + filename`
		`+ return path`
		`+`
		`+ def test_process(self, KojiWrapper, get_file_size, get_mtime, Linker):`
		`+ self.repo = self.topdir + "/compose/Server/$arch/os"`
		`+ compose = DummyCompose(self.topdir, {"koji_profile": "koji"})`
		`+ config = _merge({"subvariant": "Test"}, MINIMAL_CONF)`
		`+ pool = mock.Mock()`
		`+`
		`+ get_image_paths = KojiWrapper.return_value.get_image_paths`
		`+ get_image_paths.return_value = {`
		`+ "x86_64": [`
		`+ "/koji/task/1234/FCBG.x86_64-Rawhide-1.6.packages",`
		`+ "/koji/task/1234/FCBG.x86_64-Rawhide-1.6.qcow2",`
		`+ ],`
		`+ "amd64": [`
		`+ "/koji/task/1234/FCBG.amd64-Rawhide-1.6.packages",`
		`+ "/koji/task/1234/FCBG.amd64-Rawhide-1.6.qcow2",`
		`+ ],`
		`+ }`
		`+`
		`+ KojiWrapper.return_value.koji_proxy.kiwiBuild.return_value = 1234`
		`+ KojiWrapper.return_value.watch_task.return_value = 0`
		`+`
		`+ t = RunKiwiBuildThread(pool)`
		`+ get_file_size.return_value = 1024`
		`+ get_mtime.return_value = 13579`
		`+ t.process(`
		`+ (`
		`+ compose,`
		`+ compose.variants["Server"],`
		`+ config,`
		`+ ["amd64", "x86_64"],`
		`+ "1.6",`
		`+ "f40",`
		`+ [self.repo],`
		`+ [],`
		`+ ),`
		`+ 1,`
		`+ )`
		`+`
		`+ assert KojiWrapper.return_value.koji_proxy.kiwiBuild.mock_calls == [`
		`+ mock.call(`
		`+ "f40",`
		`+ ["amd64", "x86_64"],`
		`+ config["description_scm"],`
		`+ config["description_path"],`
		`+ profile=config["kiwi_profile"],`
		`+ release="1.6",`
		`+ repos=[self.repo],`
		`+ optional_arches=[],`
		`+ )`
		`+ ]`
		`+ assert get_image_paths.mock_calls == [mock.call(1234)]`
		`+ assert os.path.isdir(self._img_path("x86_64"))`
		`+ assert os.path.isdir(self._img_path("amd64"))`
		`+ assert Linker.return_value.link.has_calls(`
adamwill commented a year ago To get this to pass locally on Python 3.12, I had to change this to `Linker.return_value.link.assert_has_calls` and add `any_order=True`. Without the former I got "AttributeError: 'has_calls' is not a valid assertion. Use a spec for the mock if 'has_calls' is meant to be an attribute.", and without the latter, the test failed because the calls were the other way around from what the assertion expected (not sure why).
		`+ [`
		`+ mock.call(`
		`+ "/koji/task/1234/FCBG.amd64-Rawhide-1.6.qcow2",`
		`+ self._img_path("amd64", "FCBG.amd64-Rawhide-1.6.qcow2"),`
		`+ link_type="hardlink-or-copy",`
		`+ ),`
		`+ mock.call(`
		`+ "/koji/task/1234/FCBG.x86_64-Rawhide-1.6.qcow2",`
		`+ self._img_path("x86_64", "FCBG.x86_64-Rawhide-1.6.qcow2"),`
		`+ link_type="hardlink-or-copy",`
		`+ ),`
		`+ ]`
		`+ )`
		`+`
		`+ assert len(compose.im.add.call_args_list) == 2`
		`+ for call in compose.im.add.call_args_list:`
		`+ _, kwargs = call`
		`+ image = kwargs["image"]`
		`+ expected_path = (`
		`+ "Server/{0.arch}/images/FCBG.{0.arch}-Rawhide-1.6.qcow2".format(image)`
		`+ )`
		`+ assert kwargs["variant"] == "Server"`
		`+ assert kwargs["arch"] in ("amd64", "x86_64")`
		`+ assert kwargs["arch"] == image.arch`
		`+ assert image.path == expected_path`
		`+ assert "qcow2" == image.format`
		`+ assert "qcow2" == image.type`
		`+ assert "Test" == image.subvariant`
		`+`
		`+ def test_handle_koji_fail(self, KojiWrapper, get_file_size, get_mtime, Linker):`
		`+ self.repo = self.topdir + "/compose/Server/$arch/os"`
		`+ compose = DummyCompose(self.topdir, {"koji_profile": "koji"})`
		`+ config = MINIMAL_CONF`
		`+ pool = mock.Mock()`
		`+`
		`+ get_image_paths = KojiWrapper.return_value.get_image_paths`
		`+`
		`+ KojiWrapper.return_value.koji_proxy.kiwiBuild.return_value = 1234`
		`+ KojiWrapper.return_value.watch_task.return_value = 1`
		`+`
		`+ t = RunKiwiBuildThread(pool)`
		`+ try:`
		`+ t.process(`
		`+ (`
		`+ compose,`
		`+ compose.variants["Server"],`
		`+ config,`
		`+ ["amd64", "x86_64"],`
		`+ "1.6",`
		`+ "f40",`
		`+ [self.repo],`
		`+ [],`
		`+ ),`
		`+ 1,`
		`+ )`
		`+ assert False, "Exception should have been raised"`
		`+ except RuntimeError:`
		`+ pass`
		`+`
		`+ assert len(KojiWrapper.return_value.koji_proxy.kiwiBuild.mock_calls) == 1`
		`+ assert get_image_paths.mock_calls == []`
		`+ assert Linker.return_value.link.mock_calls == []`
		`+ assert len(compose.im.add.call_args_list) == 0`
		`+`
		`+ def test_handle_fail_on_optional_arch(`
		`+ self, KojiWrapper, get_file_size, get_mtime, Linker`
		`+ ):`
		`+ self.repo = self.topdir + "/compose/Server/$arch/os"`
		`+ compose = DummyCompose(self.topdir, {"koji_profile": "koji"})`
		`+ config = MINIMAL_CONF`
		`+ pool = mock.Mock()`
		`+`
		`+ get_image_paths = KojiWrapper.return_value.get_image_paths`
		`+ get_image_paths.return_value = {`
		`+ "x86_64": [`
		`+ "/koji/task/1234/FCBG.x86_64-Rawhide-1.6.packages",`
		`+ "/koji/task/1234/FCBG.x86_64-Rawhide-1.6.qcow2",`
		`+ ],`
		`+ }`
		`+`
		`+ KojiWrapper.return_value.koji_proxy.kiwiBuild.return_value = 1234`
		`+ KojiWrapper.return_value.watch_task.return_value = 0`
		`+`
		`+ t = RunKiwiBuildThread(pool)`
		`+ get_file_size.return_value = 1024`
		`+ get_mtime.return_value = 13579`
		`+ t.process(`
		`+ (`
		`+ compose,`
		`+ compose.variants["Server"],`
		`+ config,`
		`+ ["amd64", "x86_64"],`
		`+ "1.6",`
		`+ "f40",`
		`+ [self.repo],`
		`+ ["amd64"],`
		`+ ),`
		`+ 1,`
		`+ )`
		`+`
		`+ assert KojiWrapper.return_value.koji_proxy.kiwiBuild.mock_calls == [`
		`+ mock.call(`
		`+ "f40",`
		`+ ["amd64", "x86_64"],`
		`+ config["description_scm"],`
		`+ config["description_path"],`
		`+ profile=config["kiwi_profile"],`
		`+ release="1.6",`
		`+ repos=[self.repo],`
		`+ optional_arches=["amd64"],`
		`+ )`
		`+ ]`
		`+ assert get_image_paths.mock_calls == [mock.call(1234)]`
		`+ assert os.path.isdir(self._img_path("x86_64"))`
		`+ assert not os.path.isdir(self._img_path("amd64"))`
		`+ assert Linker.return_value.link.mock_calls == [`
		`+ mock.call(`
		`+ "/koji/task/1234/FCBG.x86_64-Rawhide-1.6.qcow2",`
		`+ self._img_path("x86_64", "FCBG.x86_64-Rawhide-1.6.qcow2"),`
		`+ link_type="hardlink-or-copy",`
		`+ ),`
		`+ ]`
		`+`
		`+ assert len(compose.im.add.call_args_list) == 1`
		`+ _, kwargs = compose.im.add.call_args_list[0]`
		`+ image = kwargs["image"]`
		`+ expected_path = "Server/x86_64/images/FCBG.x86_64-Rawhide-1.6.qcow2"`
		`+ assert kwargs["variant"] == "Server"`
		`+ assert kwargs["arch"] == "x86_64"`
		`+ assert kwargs["arch"] == image.arch`
		`+ assert image.path == expected_path`
		`+ assert "qcow2" == image.format`
		`+ assert "qcow2" == image.type`
		`+ assert "Server" == image.subvariant`

no initial comment

4 new commits added

kiwibuild: Add tests for the basic functionality
kiwibuild: Remove repos as dicts
Fix additional image metadata
Drop kiwibuild_version option

a year ago

4 new commits added

kiwibuild: Add tests for the basic functionality
kiwibuild: Remove repos as dicts
Fix additional image metadata
Drop kiwibuild_version option

a year ago

4 new commits added

kiwibuild: Add tests for the basic functionality
kiwibuild: Remove repos as dicts
Fix additional image metadata
Drop kiwibuild_version option

a year ago

ngompa commented a year ago

Oh crap, we had full repo definition support? Alas, while kiwi can support that, the koji plugin does not.

4 new commits added

kiwibuild: Add tests for the basic functionality
kiwibuild: Remove repos as dicts
Fix additional image metadata
Drop kiwibuild_version option

a year ago

lsedlar commented a year ago

Oh crap, we had full repo definition support? Alas, while kiwi can support that, the koji plugin does not.

Well not really. The code was copied from osbuild phase, which can process repos specified as dicts with baseurl and potentially other options. But the schema didn't allow that, so rather than testing something that can't even be used I removed the code. If kiwiBuild koji method gets support for it, Pungi could certainly expose such functionality.

ngompa commented a year ago

The koji plugin rewrites the repo definitions in the XML as part of processing it, so if we actually want to expose a dictionary that provides full repo settings, that is certainly a future expansion option.

ngompa commented a year ago

Anyway, LGTM. :thumbsup:

adamwill commented a year ago

:thumbsup:

adamwill commented a year ago

Something I notice: the CI is set up to run the tests in a container. This seems like an excessive amount of isolation for a pure Python project; shouldn't just using tox be enough? More importantly, said container hasn't been updated for three years, so it's using rather old versions of things. It seems to be based on Fedora 33. This means it could be missing bugs caused by more recent changes; e.g. none of the tests runs on Python 3.12, because:

ERROR tests/test_test_phase.py - AttributeError: module 'collections' has no attribute 'MutableMapping'

there are dozens of those errors. Importing MutableMapping from collections (rather than collections.abc was deprecated in 3.3 and was finally removed in Python 3.10. The container has Python 3.9.

I would suggest either making sure the container gets updated more often, or just skipping the container and relying on tox to provide necessary isolation, if viable...

Edited a year ago by adamwill

adamwill commented a year ago

huh, that error actually comes from unittest2:

tests/test_util.py:12: in <module>
    import unittest2 as unittest
.tox/py3/lib/python3.12/site-packages/unittest2/__init__.py:40: in <module>
    from unittest2.collector import collector
.tox/py3/lib/python3.12/site-packages/unittest2/collector.py:3: in <module>
    from unittest2.loader import defaultTestLoader
.tox/py3/lib/python3.12/site-packages/unittest2/loader.py:13: in <module>
    from unittest2 import case, suite, util
.tox/py3/lib/python3.12/site-packages/unittest2/case.py:18: in <module>
    from unittest2 import result
.tox/py3/lib/python3.12/site-packages/unittest2/result.py:10: in <module>
    from unittest2.compatibility import wraps
.tox/py3/lib/python3.12/site-packages/unittest2/compatibility.py:143: in <module>
    class ChainMap(collections.MutableMapping):
E   AttributeError: module 'collections' has no attribute 'MutableMapping'

but the general point stands, this is being hidden in CI because it's running on old Python.

adamwill commented a year ago

ah, I see the tests use createrepo_c, which is indeed a bit tricky to get into tox environments, IIRC.

adamwill commented on line 247 of tests/test_kiwibuildphase.py a year ago

To get this to pass locally on Python 3.12, I had to change this to Linker.return_value.link.assert_has_calls and add any_order=True. Without the former I got "AttributeError: 'has_calls' is not a valid assertion. Use a spec for the mock if 'has_calls' is meant to be an attribute.", and without the latter, the test failed because the calls were the other way around from what the assertion expected (not sure why).

adamwill commented a year ago

To fix the unittest2 stuff I just ditched all the import unittest2 as unittest imports and replaced them with import unittest. unittest2 is not relevant on anything we care about any more, AFAIK.