From b3e248d6ec851c93f2d162fc87f1e1b619d9549c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Stan=C4=9Bk?= Date: Tue, 27 Feb 2018 14:55:13 +0100 Subject: [PATCH 1/2] Fix names of packages added to target koji tag --- rpmlb/builder/koji.py | 48 ++++++++++++++++++++++++-------------- tests/builder/test_koji.py | 19 ++++++++++++--- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/rpmlb/builder/koji.py b/rpmlb/builder/koji.py index d9ebacf..a16edf7 100644 --- a/rpmlb/builder/koji.py +++ b/rpmlb/builder/koji.py @@ -130,25 +130,44 @@ def prepare_extra_steps(self, source, package_dict): def build(self, package_dict, **kwargs): """Build a package using Koji instance""" + base_name = package_dict['name'] + full_name = self._full_package_name(base_name, self.collection) srpm_path = self._make_srpm( - name=package_dict['name'], + name=base_name, collection=self.collection, epel=self.epel, ) - self._add_package(name='{collection}-{name}'.format( - collection=self.collection, - name=package_dict['name'], - )) + self._add_package(full_name) self._submit_build(srpm_path) self._wait_for_repo() @staticmethod - def _make_srpm(name: str, collection: str, epel: int) -> Path: + def _full_package_name(base_name: str, collection: str) -> str: + """Determine what the full name of the package should be + after applying the collection prefix. + + Keyword arguments: + base_name: Name of the package, without the collection part. + collection: Name/identification of the package's collection. + + Returns: + Canonical full name. + """ + + # The metapackage + if base_name == collection: + return base_name + else: + return '-'.join((collection, base_name)) + + @staticmethod + def _make_srpm(base_name: str, collection: str, epel: int) -> Path: """Create SRPM of the specified name in current directory. Keyword arguments: - name: Name of the package to create. + base_name: Name of the package to create, + without collection prefix. collection: Name/identification of the package's collection. epel: The EPEL version to build for. @@ -156,7 +175,7 @@ def _make_srpm(name: str, collection: str, epel: int) -> Path: Path to the created SRPM. """ - spec_path = Path('.'.join((name, 'spec'))) + spec_path = Path('.'.join((base_name, 'spec'))) if not spec_path.exists(): raise FileNotFoundError(spec_path) @@ -180,15 +199,10 @@ def _make_srpm(name: str, collection: str, epel: int) -> Path: run_cmd_with_capture(' '.join(command)) - if name == collection: # metapackage build - glob_format = '{collection}-*.src.rpm' - else: - glob_format = '{collection}-{name}-*.src.rpm' - - srpm_path, = cwd.glob(glob_format.format( - collection=collection, - name=name, - )) + srpm_glob = '{full_name}-*.src.rpm'.format( + full_name=KojiBuilder._full_package_name(base_name, collection), + ) + srpm_path, = cwd.glob(srpm_glob) return srpm_path @property diff --git a/tests/builder/test_koji.py b/tests/builder/test_koji.py index 6e104ed..4610fe7 100644 --- a/tests/builder/test_koji.py +++ b/tests/builder/test_koji.py @@ -183,7 +183,7 @@ def test_make_srpm_creates_srpm(spec_path, collection_id, epel): configure_logging(DEBUG) arguments = { - 'name': spec_path.stem, + 'base_name': spec_path.stem, 'collection': collection_id, 'epel': epel, } @@ -192,7 +192,9 @@ def test_make_srpm_creates_srpm(spec_path, collection_id, epel): # Metapackage '{collection}-1-1.el{epel}.src.rpm'.format_map(arguments), # Regular package - '{collection}-{name}-1.0-1.el{epel}.src.rpm'.format_map(arguments), + '{collection}-{base_name}-1.0-1.el{epel}.src.rpm'.format_map( + arguments, + ), } srpm_path = KojiBuilder._make_srpm(**arguments) @@ -207,7 +209,7 @@ def test_missing_spec_is_reported(tmpdir): configure_logging(DEBUG) with tmpdir.as_cwd(), pytest.raises(FileNotFoundError): - KojiBuilder._make_srpm(name='test', collection='test', epel=7) + KojiBuilder._make_srpm(base_name='test', collection='test', epel=7) def test_prepare_adjusts_bootstrap_release(builder, spec_contents): @@ -298,3 +300,14 @@ def test_add_package_respects_owner(monkeypatch, builder): cmd, = commands assert '--owner' in cmd assert builder.owner in cmd + + +@pytest.mark.parametrize('package_name,expected', [ + ('rh-ror50', 'rh-ror50'), + ('ruby', 'rh-ror50-ruby'), +]) +def test_full_name_is_resolved_correctly(builder, package_name, expected): + """Assert that build adds the package with correct name.""" + + full_name = builder._full_package_name(package_name, builder.collection) + assert full_name == expected From 27364adcfbb3abb8ab8336882f527fe914c05634 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Stan=C4=9Bk?= Date: Wed, 28 Feb 2018 10:56:22 +0100 Subject: [PATCH 2/2] Refactor Koji build flow - Turn static methods for rebuild steps into regular ones - Extract mocking in tests into dedicated fixture --- rpmlb/builder/koji.py | 37 +++++++---------- tests/builder/test_koji.py | 85 ++++++++++++++++++-------------------- 2 files changed, 54 insertions(+), 68 deletions(-) diff --git a/rpmlb/builder/koji.py b/rpmlb/builder/koji.py index a16edf7..3db7b20 100644 --- a/rpmlb/builder/koji.py +++ b/rpmlb/builder/koji.py @@ -131,51 +131,41 @@ def build(self, package_dict, **kwargs): """Build a package using Koji instance""" base_name = package_dict['name'] - full_name = self._full_package_name(base_name, self.collection) - srpm_path = self._make_srpm( - name=base_name, - collection=self.collection, - epel=self.epel, - ) + full_name = self._full_package_name(base_name) + srpm_path = self._make_srpm(name=base_name) self._add_package(full_name) self._submit_build(srpm_path) self._wait_for_repo() - @staticmethod - def _full_package_name(base_name: str, collection: str) -> str: + def _full_package_name(self, base_name: str) -> str: """Determine what the full name of the package should be after applying the collection prefix. Keyword arguments: base_name: Name of the package, without the collection part. - collection: Name/identification of the package's collection. Returns: Canonical full name. """ # The metapackage - if base_name == collection: + if base_name == self.collection: return base_name else: - return '-'.join((collection, base_name)) + return '-'.join((self.collection, base_name)) - @staticmethod - def _make_srpm(base_name: str, collection: str, epel: int) -> Path: + def _make_srpm(self, name: str) -> Path: """Create SRPM of the specified name in current directory. Keyword arguments: - base_name: Name of the package to create, - without collection prefix. - collection: Name/identification of the package's collection. - epel: The EPEL version to build for. + name: Name of the package to create, without collection prefix. Returns: Path to the created SRPM. """ - spec_path = Path('.'.join((base_name, 'spec'))) + spec_path = Path('.'.join((name, 'spec'))) if not spec_path.exists(): raise FileNotFoundError(spec_path) @@ -188,9 +178,11 @@ def _make_srpm(base_name: str, collection: str, epel: int) -> Path: '_{kind}dir {cwd}'.format(kind=k, cwd=cwd) for k in directory_kinds ] # dist tag - define_list.append('dist .el{epel}'.format(epel=epel)) + define_list.append('dist .el{epel}'.format(epel=self.epel)) # collection name – needed to generate prefixed package - define_list.append('scl {collection}'.format(collection=collection)) + define_list.append('scl {collection}'.format( + collection=self.collection, + )) # Assemble the command command = ['rpmbuild'] @@ -199,9 +191,8 @@ def _make_srpm(base_name: str, collection: str, epel: int) -> Path: run_cmd_with_capture(' '.join(command)) - srpm_glob = '{full_name}-*.src.rpm'.format( - full_name=KojiBuilder._full_package_name(base_name, collection), - ) + # SRPM contains the collection prefix + srpm_glob = '{}-*.src.rpm'.format(self._full_package_name(name)) srpm_path, = cwd.glob(srpm_glob) return srpm_path diff --git a/tests/builder/test_koji.py b/tests/builder/test_koji.py index 4610fe7..ab350ae 100644 --- a/tests/builder/test_koji.py +++ b/tests/builder/test_koji.py @@ -6,6 +6,7 @@ from logging import DEBUG from pathlib import Path from textwrap import dedent +from unittest.mock import Mock import pytest @@ -61,13 +62,6 @@ def work(valid_recipe_path, collection_id): return Work(valid_recipe) -@pytest.fixture -def builder(work): - """Provide minimal KojiBuilder instance.""" - - return KojiBuilder(work, koji_epel=7, koji_owner='test-owner') - - @pytest.fixture(params=[ KojiBuilder.DEFAULT_TARGET_FORMAT, 'test-target', @@ -86,6 +80,13 @@ def epel(request): return request.param +@pytest.fixture +def builder(work, epel): + """Provide minimal KojiBuilder instance.""" + + return KojiBuilder(work, koji_epel=epel, koji_owner='test-owner') + + @pytest.fixture(params=[None, 'koji', 'cbs']) def profile(request): """Koji profile name""" @@ -116,6 +117,21 @@ def cli_parameters(target_format, epel, profile, scratch_build): } +@pytest.fixture +def mock_runner(monkeypatch): + """Mock shell command runner with command recording.""" + + runner = Mock(return_value=None) + monkeypatch.setattr('rpmlb.builder.koji.run_cmd', runner) + monkeypatch.setattr('rpmlb.builder.koji.run_cmd_with_capture', runner) + + # Also patch _make_srpm which fails if runner has no side effect + mock_srpm = Mock(return_value=Path('test.src.rpm')) + monkeypatch.setattr(KojiBuilder, '_make_srpm', mock_srpm) + + return runner + + def test_init_sets_attributes(work, cli_parameters): """Ensure that the parameters are set to appropriate values.""" @@ -177,13 +193,13 @@ def test_target_respects_format( assert builder.target == expected_target -def test_make_srpm_creates_srpm(spec_path, collection_id, epel): +def test_make_srpm_creates_srpm(builder, spec_path, collection_id, epel): """Ensure that make_srpm works as expected""" configure_logging(DEBUG) arguments = { - 'base_name': spec_path.stem, + 'name': spec_path.stem, 'collection': collection_id, 'epel': epel, } @@ -192,24 +208,24 @@ def test_make_srpm_creates_srpm(spec_path, collection_id, epel): # Metapackage '{collection}-1-1.el{epel}.src.rpm'.format_map(arguments), # Regular package - '{collection}-{base_name}-1.0-1.el{epel}.src.rpm'.format_map( + '{collection}-{name}-1.0-1.el{epel}.src.rpm'.format_map( arguments, ), } - srpm_path = KojiBuilder._make_srpm(**arguments) + srpm_path = builder._make_srpm(arguments['name']) assert srpm_path.exists(), srpm_path assert srpm_path.name in possible_names -def test_missing_spec_is_reported(tmpdir): +def test_missing_spec_is_reported(tmpdir, builder): """make_srpm does not attempt to build nonexistent SPEC file""" configure_logging(DEBUG) with tmpdir.as_cwd(), pytest.raises(FileNotFoundError): - KojiBuilder._make_srpm(base_name='test', collection='test', epel=7) + builder._make_srpm(name='test') def test_prepare_adjusts_bootstrap_release(builder, spec_contents): @@ -231,38 +247,28 @@ def test_prepare_adjusts_bootstrap_release(builder, spec_contents): (False, ['koji add-pkg', 'koji build', 'koji wait-repo']), ]) def test_build_emit_correct_commands( - monkeypatch, builder, scratch, expected_commands + monkeypatch, mock_runner, builder, scratch, expected_commands ): """Builder emits expected commands on build.""" - # Gather all emitted commands - commands = [] - monkeypatch.setattr( - 'rpmlb.builder.koji.run_cmd', - lambda cmd, **__: commands.append(cmd), - ) - - # Skip make_srpm, as it requires the run_cmd to actually do something - monkeypatch.setattr( - KojiBuilder, '_make_srpm', - lambda *_, **__: Path('test.src.rpm'), - ) - # Provide hardcoded destination tag monkeypatch.setattr(KojiBuilder, '_destination_tag', 'test_tag_name') builder.scratch_build = scratch builder.build({'name': 'test'}) + commands = mock_runner.call_args_list command_pairs = zip_longest(commands, expected_commands) assert all(cmd.startswith(exp) for cmd, exp in command_pairs), commands -def test_destination_tag_parsed_correctly(monkeypatch, builder): +def test_destination_tag_parsed_correctly(mock_runner, builder): """Assert that the destination tag is correctly extracted from output.""" # Simulate output of `koji list-targets` - raw_output = namedtuple('MockCommandOutput', ['stdout', 'stderr'])( + mock_runner.return_value = namedtuple( + 'MockCommandOutput', ['stdout', 'stderr'] + )( stderr=b'', stdout=dedent('''\ Name Buildroot Destination @@ -271,23 +277,13 @@ def test_destination_tag_parsed_correctly(monkeypatch, builder): ''').encode('utf-8') ) - monkeypatch.setattr( - 'rpmlb.builder.koji.run_cmd_with_capture', - lambda *_, **__: raw_output, - ) - builder.target_format = 'test-target' assert builder._destination_tag == 'test-tag-candidate' -def test_add_package_respects_owner(monkeypatch, builder): +def test_add_package_respects_owner(monkeypatch, mock_runner, builder): """Assert that the owner is used by the _add_package method.""" - commands = [] - monkeypatch.setattr( - 'rpmlb.builder.koji.run_cmd', - lambda cmd, **__: commands.append(cmd), - ) monkeypatch.setattr( KojiBuilder, '_destination_tag', 'test-destination' ) @@ -295,9 +291,9 @@ def test_add_package_respects_owner(monkeypatch, builder): builder.owner = 'expected-owner' builder._add_package(name='test') - assert len(commands) == 1 + assert mock_runner.call_count == 1 - cmd, = commands + cmd = mock_runner.call_args[0][0] assert '--owner' in cmd assert builder.owner in cmd @@ -307,7 +303,6 @@ def test_add_package_respects_owner(monkeypatch, builder): ('ruby', 'rh-ror50-ruby'), ]) def test_full_name_is_resolved_correctly(builder, package_name, expected): - """Assert that build adds the package with correct name.""" + """Ensure the correct form of full package name.""" - full_name = builder._full_package_name(package_name, builder.collection) - assert full_name == expected + assert builder._full_package_name(package_name) == expected