Refactor Koji build flow

-   Turn static methods for rebuild steps into regular ones
-   Extract mocking in tests into dedicated fixture

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, or the base_name already has the prefix applied.
-        if base_name.startswith(collection):
+        if base_name.startswith(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
 

tests/builder/test_koji.py

@@ -6,7 +6,7 @@
 from logging import DEBUG
 from pathlib import Path
 from textwrap import dedent
-from unittest.mock import Mock, call
+from unittest.mock import Mock
 
 import pytest
 
@@ -62,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',
@@ -87,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"""
@@ -117,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."""
 
@@ -178,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,
     }
@@ -193,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):
@@ -232,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
@@ -272,33 +277,23 @@ 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
 
@@ -307,26 +302,7 @@ def test_add_package_respects_owner(monkeypatch, builder):
     ('rh-ror50', 'rh-ror50'),
     ('ruby', 'rh-ror50-ruby'),
 ])
-def test_add_package_adds_correct_name(
-    monkeypatch,
-    builder,
-    package_name,
-    expected,
-):
-    """Assert that build adds the package with correct name."""
-
-    # Ignore external calls
-    monkeypatch.setattr('rpmlb.builder.koji.run_cmd', lambda *_, **__: None)
-
-    # Skip make_srpm, as it requires the run_cmd to actually do something
-    monkeypatch.setattr(
-        KojiBuilder, '_make_srpm',
-        lambda *_, **__: Path('test.src.rpm'),
-    )
-
-    monkeypatch.setattr(KojiBuilder, '_add_package', Mock())
-
-    builder.build({'name': package_name})
+def test_full_name_is_resolved_correctly(builder, package_name, expected):
+    """Ensure the correct form of full package name."""
 
-    possible_calls = [call(expected), call(name=expected)]
-    assert builder._add_package.call_args in possible_calls
+    assert builder._full_package_name(package_name) == expected