Merge pull request #105 from khardix/fix/add-package-naming

Fix naming of packages added to target koji tag

rpmlb/builder/koji.py

@@ -130,27 +130,36 @@ def prepare_extra_steps(self, source, package_dict):
     def build(self, package_dict, **kwargs):
         """Build a package using Koji instance"""
 
-        srpm_path = self._make_srpm(
-            name=package_dict['name'],
-            collection=self.collection,
-            epel=self.epel,
-        )
+        base_name = package_dict['name']
+        full_name = self._full_package_name(base_name)
+        srpm_path = self._make_srpm(name=base_name)
 
-        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(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.
+
+        Returns:
+            Canonical full name.
+        """
+
+        # The metapackage
+        if base_name == self.collection:
+            return base_name
+        else:
+            return '-'.join((self.collection, base_name))
+
+    def _make_srpm(self, name: str) -> Path:
         """Create SRPM of the specified name in current directory.
 
         Keyword arguments:
-            name: Name of the package to create.
-            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.
@@ -169,9 +178,11 @@ def _make_srpm(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']
@@ -180,15 +191,9 @@ 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 contains the collection prefix
+        srpm_glob = '{}-*.src.rpm'.format(self._full_package_name(name))
+        srpm_path, = cwd.glob(srpm_glob)
         return srpm_path
 
     @property

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,7 +193,7 @@ 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)
@@ -192,22 +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}-{name}-1.0-1.el{epel}.src.rpm'.format_map(arguments),
+        '{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(name='test', collection='test', epel=7)
+        builder._make_srpm(name='test')
 
 
 def test_prepare_adjusts_bootstrap_release(builder, spec_contents):
@@ -229,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
@@ -269,32 +277,32 @@ 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'
     )
 
     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
+
+
+@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):
+    """Ensure the correct form of full package name."""
+
+    assert builder._full_package_name(package_name) == expected