From de3ed2087aeb6e660f12801b72d1cee5d1e0ded2 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] 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 | 106 ++++++++++++++----------------------- 2 files changed, 55 insertions(+), 88 deletions(-) diff --git a/rpmlb/builder/koji.py b/rpmlb/builder/koji.py index fa004c9..c2f7da0 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, 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 diff --git a/tests/builder/test_koji.py b/tests/builder/test_koji.py index bfded41..ab350ae 100644 --- a/tests/builder/test_koji.py +++ b/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,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' ) @@ -296,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,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