From 8d137aba9757852f6d76152c544b4a74c96bc627 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 13 Sep 2022 15:15:45 +0100 Subject: [PATCH 1/5] Improve test coverage for Rose Stem --- cylc/rose/stem.py | 31 ++-- setup.cfg | 4 +- tests/functional/test_rose_stem.py | 18 +++ tests/unit/test_rose_stem_units.py | 243 ++++++++++++++++++++++++++++- 4 files changed, 282 insertions(+), 14 deletions(-) diff --git a/cylc/rose/stem.py b/cylc/rose/stem.py index 36951979..a0e3e425 100644 --- a/cylc/rose/stem.py +++ b/cylc/rose/stem.py @@ -153,7 +153,9 @@ class RoseStemVersionException(Exception): def __init__(self, version): Exception.__init__(self, version) if version is None: - self.suite_version = "not rose-stem compatible" + self.suite_version = ( + "does not have ROSE_VERSION set in the rose-suite.conf" + ) else: self.suite_version = "at version %s" % (version) @@ -225,24 +227,32 @@ class StemRunner: def __init__(self, opts, reporter=None, popen=None, fs_util=None): self.opts = opts + if reporter is None: self.reporter = Reporter(opts.verbosity - opts.quietness) else: self.reporter = reporter + if popen is None: self.popen = RosePopener(event_handler=self.reporter) else: self.popen = popen + if fs_util is None: self.fs_util = FileSystemUtil(event_handler=self.reporter) else: self.fs_util = fs_util + self.host_selector = HostSelector(event_handler=self.reporter, popen=self.popen) def _add_define_option(self, var, val): - """Add a define option passed to the SuiteRunner.""" + """Add a define option passed to the SuiteRunner. + Args: + var: Name of variable to set + val: Value of variable to set + """ if self.opts.defines: self.opts.defines.append(SUITE_RC_PREFIX + var + '=' + val) else: @@ -250,7 +260,7 @@ def _add_define_option(self, var, val): self.reporter(ConfigVariableSetEvent(var, val)) return - def _get_base_dir(self, item): + def _get_fcm_loc_layout_info(self, src_tree): """Given a source tree return the following from 'fcm loc-layout': * url * sub_tree @@ -259,15 +269,17 @@ def _get_base_dir(self, item): * project """ - ret_code, output, stderr = self.popen.run('fcm', 'loc-layout', item) + ret_code, output, stderr = self.popen.run( + 'fcm', 'loc-layout', src_tree) if ret_code != 0: - raise ProjectNotFoundException(item, stderr) + raise ProjectNotFoundException(src_tree, stderr) ret = {} for line in output.splitlines(): if ":" not in line: continue key, value = line.split(":", 1) + if key and value: ret[key] = value.strip() @@ -289,7 +301,8 @@ def _get_project_from_url(self, source_dict): break return project - def _deduce_mirror(self, source_dict, project): + @staticmethod + def _deduce_mirror(source_dict, project): """Deduce the mirror location of this source tree.""" # Root location for project @@ -331,7 +344,7 @@ def _ascertain_project(self, item): print(f"[WARN] Forcing project for '{item}' to be '{project}'") return project, item, item, '', '' - source_dict = self._get_base_dir(item) + source_dict = self._get_fcm_loc_layout_info(item) project = self._get_project_from_url(source_dict) if not project: raise ProjectNotFoundException(item) @@ -602,7 +615,3 @@ def rose_stem(parser, opts): ), file=sys.stderr ) - - -if __name__ == "__main__": - main() diff --git a/setup.cfg b/setup.cfg index 7da5cc9e..7e41d2cc 100644 --- a/setup.cfg +++ b/setup.cfg @@ -55,8 +55,8 @@ packages = find_namespace: python_requires = >=3.7 include_package_data = True install_requires = - metomi-rose==2.1.* - cylc-flow==8.1.* + # metomi-rose==2.*.* + # cylc-flow==8.*.* metomi-isodatetime jinja2 diff --git a/tests/functional/test_rose_stem.py b/tests/functional/test_rose_stem.py index 7be8ae13..0240d67a 100644 --- a/tests/functional/test_rose_stem.py +++ b/tests/functional/test_rose_stem.py @@ -244,6 +244,24 @@ def _inner_fn(rose_stem_opts, verbosity=verbosity): yield _inner_fn +@pytest.fixture(scope='class') +def rose_stem_run_really_basic(rose_stem_run_template, setup_stem_repo): + rose_stem_opts = { + 'stem_groups': [], + 'stem_sources': [ + str(setup_stem_repo['workingcopy']), "fcm:foo.x_tr@head" + ], + } + yield rose_stem_run_template(rose_stem_opts) + + +class TestReallyBasic(): + def test_really_basic(self, rose_stem_run_really_basic): + """Check that assorted variables have been exported. + """ + assert rose_stem_run_really_basic['run_stem'].returncode == 0 + + @pytest.fixture(scope='class') def rose_stem_run_basic(rose_stem_run_template, setup_stem_repo): rose_stem_opts = { diff --git a/tests/unit/test_rose_stem_units.py b/tests/unit/test_rose_stem_units.py index b9e5d5e4..f17e7ce1 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -17,9 +17,28 @@ """ import pytest +from pytest import param from types import SimpleNamespace -from cylc.rose.stem import get_source_opt_from_args +from cylc.rose.stem import ( + ProjectNotFoundException, + RoseStemVersionException, + RoseSuiteConfNotFoundException, + StemRunner, SUITE_RC_PREFIX, + get_source_opt_from_args +) + +from metomi.rose.reporter import Reporter +from metomi.rose.popen import RosePopener +from metomi.rose.fs_util import FileSystemUtil + + +class MockPopen: + def __init__(self, mocked_return): + self.mocked_return = mocked_return + + def run(self, *args): + return self.mocked_return @pytest.mark.parametrize( @@ -54,3 +73,225 @@ def test_get_source_opt_from_args(tmp_path, monkeypatch, args, expect): assert result == expect else: assert result == expect.format(tmp_path=str(tmp_path)) + + +@pytest.fixture +def get_StemRunner(): + def _inner(kwargs, options=None): + if options is None: + options = {} + """Create a StemRunner objects with some options set.""" + opts = SimpleNamespace(**{'verbosity': 1, 'quietness': 1}) + for k, v in options.items(): + setattr(opts, k, v) + stemrunner = StemRunner(opts, **kwargs) + return stemrunner + return _inner + + +@pytest.mark.parametrize( + 'attribute, object_, kwargs', [ + param('reporter', Reporter, {}, id='reporter default'), + param('reporter', str, {'reporter': 'foo'}, id='reporter set'), + param('popen', RosePopener, {}, id='popen default'), + param('popen', str, {'popen': 'foo'}, id='popen set'), + param('fs_util', FileSystemUtil, {}, id='fs_util default'), + param('fs_util', str, {'fs_util': 'foo'}, id='fs_util set') + ] +) +def test_StemRunner_init(get_StemRunner, attribute, object_, kwargs): + """It handles __init__ with different kwargs.""" + stemrunner = get_StemRunner(kwargs) + item = getattr(stemrunner, attribute) + if isinstance(object_, str): + assert item == kwargs[attribute] + else: + assert isinstance(item, object_) + + +@pytest.mark.parametrize( + 'exisiting_defines', + [ + param([], id='no existing defines'), + param(['opts=(cylc-install)'], id='existing defines') + ] +) +def test__add_define_option(get_StemRunner, capsys, exisiting_defines): + """It adds to defines, rather than replacing any.""" + stemrunner = get_StemRunner( + {'reporter': print}, {'defines': exisiting_defines}) + assert stemrunner._add_define_option('FOO', '"bar"') is None + assert f'{SUITE_RC_PREFIX}FOO="bar"' in stemrunner.opts.defines + assert 'Variable FOO set to "bar"' in capsys.readouterr().out + + +@pytest.mark.parametrize( + 'mocked_return', + [ + param((1, 'foo', 'SomeError'), id='it fails if fcm-loc-layout fails'), + param( + ( + 0, + 'url: file:///worthwhile/foo/bar/baz/trunk@1\n' + 'project: \n' + 'some waffle which ought to be ignored', + '' + ), + id='Good fcm output' + ) + + ] +) +def test__get_fcm_loc_layout_info(get_StemRunner, capsys, mocked_return): + """It parses information from fcm loc layout""" + + stemrunner = get_StemRunner({'popen': MockPopen(mocked_return)}) + + if mocked_return[0] == 0: + expect = { + 'url': 'file:///worthwhile/foo/bar/baz/trunk@1', + 'project': '' + } + assert expect == stemrunner._get_fcm_loc_layout_info('foo') + else: + with pytest.raises(ProjectNotFoundException) as exc: + stemrunner._get_fcm_loc_layout_info('foo') + assert mocked_return[2] in str(exc.value) + + +@pytest.mark.parametrize( + 'source_dict, mockreturn, expect', + [ + param( + { + 'root': 'svn://subversive', + 'project': 'waltheof', + 'url': 'Irrelevent, it\'s mocked away, but required.' + }, + ( + 0, + ( + "location{primary}[mortimer] = " + "svn://subversive/rogermortimer\n" + "location{primary}[fenwick] = " + "svn://subversive/johnfenwick\n" + "location{primary}[waltheof] = " + "svn://subversive/waltheof\n" + ), + ), + 'waltheof', + id='all paths true' + ), + param( + { + 'root': 'svn://subversive', + 'project': 'waltheof', + 'url': 'Irrelevent, it\'s mocked away, but required.' + }, + (0, "location{primary} = svn://subversive/waltheof\n"), + None, + id='no kp result' + ) + ] +) +def test__get_project_from_url( + get_StemRunner, source_dict, mockreturn, expect +): + stemrunner = get_StemRunner({'popen': MockPopen(mockreturn)}) + project = stemrunner._get_project_from_url(source_dict) + assert project == expect + + +@pytest.mark.parametrize( + 'source, expect', + ( + (None, 'cwd'), + ('foo/bar', 'some_dir'), + ) +) +def test__generate_name(get_StemRunner, monkeypatch, tmp_path, source, expect): + """It generates a name if StemRunner._ascertain_project fails. + + (This happens if the workflow source is not controlled with FCM) + """ + monkeypatch.chdir(tmp_path) + + # Case: we've set source: + source = (tmp_path / source / expect) if expect == 'some_dir' else None + # Case: we've not set source: + expect = tmp_path.name if expect == 'cwd' else expect + + stemrunner = get_StemRunner({}, {'source': source}) + assert stemrunner._generate_name() == expect + + +@pytest.mark.parametrize( + 'stem_sources, expect', + ( + ('given', True), + ('given', False), + ('infer', True), + ('infer', False), + ) +) +def test__this_suite( + get_StemRunner, monkeypatch, tmp_path, stem_sources, expect +): + """It returns a sensible suite-dir.""" + stem_suite_subdir = tmp_path / 'rose-stem' + stem_suite_subdir.mkdir() + + if stem_sources == 'infer': + stem_sources = [] + import cylc + monkeypatch.setattr( + cylc.rose.stem.StemRunner, + '_ascertain_project', + lambda x, y: [0, str(tmp_path)] + ) + else: + stem_sources = [tmp_path] + + if expect: + (stem_suite_subdir / 'rose-suite.conf').write_text( + 'ROSE_STEM_VERSION=1') + stemrunner = get_StemRunner({}, {'stem_sources': stem_sources}) + assert stemrunner._this_suite() == str(stem_suite_subdir) + else: + with pytest.raises(RoseSuiteConfNotFoundException): + stemrunner = get_StemRunner({}, {'stem_sources': stem_sources}) + assert stemrunner._this_suite() == str(stem_suite_subdir) + + +def test__check_suite_version_fails_if_no_stem_source( + get_StemRunner, tmp_path +): + """It fails if path of first stem source is not a file""" + stemrunner = get_StemRunner( + {}, {'stem_sources': str(tmp_path), 'source': None}) + stem_suite_subdir = tmp_path / 'rose-stem' + stem_suite_subdir.mkdir() + with pytest.raises(RoseSuiteConfNotFoundException, match='^\nCannot'): + stemrunner._check_suite_version(str(tmp_path)) + + +def test__check_suite_version_incompatible(get_StemRunner, tmp_path): + """It fails if path of first stem source is not a file""" + (tmp_path / 'rose-suite.conf').write_text('') + stemrunner = get_StemRunner( + {}, {'stem_sources': [], 'source': str(tmp_path)}) + with pytest.raises( + RoseStemVersionException, match='ROSE_VERSION' + ): + stemrunner._check_suite_version(str(tmp_path / 'rose-suite.conf')) + + +def test__deduce_mirror(): + source_dict = { + 'root': 'svn://lab/spaniel.xm', + 'project': 'myproject.xm', + 'url': 'svn://lab/spaniel.xm/myproject/trunk@123', + 'sub_tree': 'foo' + } + project = 'someproject' + StemRunner._deduce_mirror(source_dict, project) From 500d1489976c4251e4edb7c2d75ee28eb984f740 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 12 Oct 2022 16:21:19 +0100 Subject: [PATCH 2/5] removed never used option --- cylc/rose/stem.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cylc/rose/stem.py b/cylc/rose/stem.py index a0e3e425..be866ef5 100644 --- a/cylc/rose/stem.py +++ b/cylc/rose/stem.py @@ -600,10 +600,7 @@ def rose_stem(parser, opts): opts = StemRunner(opts).process() # call cylc install - if hasattr(opts, 'source'): - cylc_install(parser, opts, opts.source) - else: - cylc_install(parser, opts) + cylc_install(parser, opts, opts.source) except CylcError as exc: if opts.verbosity > 1: From 2634c7e04518df23472c026d9bc2440e0d338d46 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 13 Oct 2022 10:47:39 +0100 Subject: [PATCH 3/5] Update setup.cfg --- setup.cfg | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/setup.cfg b/setup.cfg index 7e41d2cc..7da5cc9e 100644 --- a/setup.cfg +++ b/setup.cfg @@ -55,8 +55,8 @@ packages = find_namespace: python_requires = >=3.7 include_package_data = True install_requires = - # metomi-rose==2.*.* - # cylc-flow==8.*.* + metomi-rose==2.1.* + cylc-flow==8.1.* metomi-isodatetime jinja2 From 2952550fbd7bbcb11f21947936808544195c6c31 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 13 Oct 2022 13:26:59 +0100 Subject: [PATCH 4/5] response to review --- tests/unit/test_rose_stem_units.py | 40 ++++++++++++++---------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/tests/unit/test_rose_stem_units.py b/tests/unit/test_rose_stem_units.py index f17e7ce1..ea466056 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -16,6 +16,7 @@ """Functional tests for top-level function record_cylc_install_options and """ +import cylc import pytest from pytest import param from types import SimpleNamespace @@ -81,32 +82,28 @@ def _inner(kwargs, options=None): if options is None: options = {} """Create a StemRunner objects with some options set.""" - opts = SimpleNamespace(**{'verbosity': 1, 'quietness': 1}) - for k, v in options.items(): - setattr(opts, k, v) + opts = SimpleNamespace(verbosity=1, quietness=1, **options) stemrunner = StemRunner(opts, **kwargs) return stemrunner return _inner -@pytest.mark.parametrize( - 'attribute, object_, kwargs', [ - param('reporter', Reporter, {}, id='reporter default'), - param('reporter', str, {'reporter': 'foo'}, id='reporter set'), - param('popen', RosePopener, {}, id='popen default'), - param('popen', str, {'popen': 'foo'}, id='popen set'), - param('fs_util', FileSystemUtil, {}, id='fs_util default'), - param('fs_util', str, {'fs_util': 'foo'}, id='fs_util set') - ] -) -def test_StemRunner_init(get_StemRunner, attribute, object_, kwargs): +def test_StemRunner_init_kwargs_set(get_StemRunner): """It handles __init__ with different kwargs.""" - stemrunner = get_StemRunner(kwargs) - item = getattr(stemrunner, attribute) - if isinstance(object_, str): - assert item == kwargs[attribute] - else: - assert isinstance(item, object_) + stemrunner = get_StemRunner({ + 'reporter': 'foo', 'popen': 'foo', 'fs_util': 'foo' + }) + assert isinstance(stemrunner.reporter, str) + assert isinstance(stemrunner.popen, str) + assert isinstance(stemrunner.popen, str) + + +def test_StemRunner_init_defaults(get_StemRunner): + """It handles __init__ with different kwargs.""" + stemrunner = get_StemRunner({}) + assert isinstance(stemrunner.reporter, Reporter) + assert isinstance(stemrunner.popen, RosePopener) + assert isinstance(stemrunner.fs_util, FileSystemUtil) @pytest.mark.parametrize( @@ -243,7 +240,6 @@ def test__this_suite( if stem_sources == 'infer': stem_sources = [] - import cylc monkeypatch.setattr( cylc.rose.stem.StemRunner, '_ascertain_project', @@ -260,7 +256,7 @@ def test__this_suite( else: with pytest.raises(RoseSuiteConfNotFoundException): stemrunner = get_StemRunner({}, {'stem_sources': stem_sources}) - assert stemrunner._this_suite() == str(stem_suite_subdir) + stemrunner._this_suite() def test__check_suite_version_fails_if_no_stem_source( From d10a387dcec1c70458ac0ebc7b12fa0dc01df5d6 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 13 Oct 2022 14:40:30 +0100 Subject: [PATCH 5/5] Update tests/unit/test_rose_stem_units.py Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- tests/unit/test_rose_stem_units.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_rose_stem_units.py b/tests/unit/test_rose_stem_units.py index ea466056..ffe852a1 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -254,8 +254,8 @@ def test__this_suite( stemrunner = get_StemRunner({}, {'stem_sources': stem_sources}) assert stemrunner._this_suite() == str(stem_suite_subdir) else: + stemrunner = get_StemRunner({}, {'stem_sources': stem_sources}) with pytest.raises(RoseSuiteConfNotFoundException): - stemrunner = get_StemRunner({}, {'stem_sources': stem_sources}) stemrunner._this_suite()