From 2a68e4c1438400e81fd44dad2a84bbff70d46630 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 5 May 2023 11:42:05 +0100 Subject: [PATCH 1/6] improve test coverage --- .coveragerc | 17 +++++ cylc/rose/entry_points.py | 11 +-- cylc/rose/stem.py | 37 ++++----- cylc/rose/utilities.py | 87 +++++++++------------- tests/unit/test_functional_post_install.py | 39 ++++++++-- tests/unit/test_rose_opts.py | 3 +- tests/unit/test_rose_stem_units.py | 68 ++++++++++++++++- 7 files changed, 169 insertions(+), 93 deletions(-) diff --git a/.coveragerc b/.coveragerc index b1a66bde..052afb62 100644 --- a/.coveragerc +++ b/.coveragerc @@ -4,3 +4,20 @@ source=./cylc [report] precision=2 + +exclude_lines = + pragma: no cover + + # Don't complain if tests don't hit defensive assertion code: + raise NotImplementedError + return NotImplemented + + # Ignore type checking code: + if (typing\.)?TYPE_CHECKING: + @overload( |$) + + # Don't complain about ellipsis (exception classes, typing overloads etc): + \.\.\. + + # Ignore abstract methods + @(abc\.)?abstractmethod diff --git a/cylc/rose/entry_points.py b/cylc/rose/entry_points.py index 7e5327f4..cb365e24 100644 --- a/cylc/rose/entry_points.py +++ b/cylc/rose/entry_points.py @@ -71,8 +71,7 @@ def post_install(srcdir=None, opts=None, rundir=None): srcdir=srcdir, opts=opts, rundir=rundir ) # Finally dump a log of the rose-conf in its final state. - if results['fileinstall']: - dump_rose_log(rundir=rundir, node=results['fileinstall']) + dump_rose_log(rundir=rundir, node=results['fileinstall']) return results @@ -177,11 +176,6 @@ def record_cylc_install_options( """ # Create a config based on command line options: cli_config = get_cli_opts_node(opts, srcdir) - # Leave now if there is nothing to do: - if not cli_config: - return False - # raise error if CLI config has multiple templating sections - identify_templating_section(cli_config) # Construct path objects representing our target files. (Path(rundir) / 'opt').mkdir(exist_ok=True) @@ -218,8 +212,7 @@ def record_cylc_install_options( dumper.dump(cli_config, str(conf_filepath)) # Merge the opts section of the rose-suite.conf with those set by CLI: - if not rose_conf_filepath.is_file(): - rose_conf_filepath.touch() + rose_conf_filepath.touch() rose_suite_conf = loader.load(str(rose_conf_filepath)) rose_suite_conf = add_cylc_install_to_rose_conf_node_opts( rose_suite_conf, cli_config diff --git a/cylc/rose/stem.py b/cylc/rose/stem.py index 178c812c..7453dc10 100644 --- a/cylc/rose/stem.py +++ b/cylc/rose/stem.py @@ -100,21 +100,13 @@ def __repr__(self): __str__ = __repr__ -class ConfigSourceTreeSetEvent(Event): - - """Event to report a source tree for config files.""" - - LEVEL = Event.V - - def __repr__(self): - return "Using config files from source %s" % (self.args[0]) - - __str__ = __repr__ - - class NameSetEvent(Event): - """Event to report a name for the suite being set.""" + """Event to report a name for the suite being set. + + Simple parser of output expected to be in the format: + Key: Value. + """ LEVEL = Event.V @@ -433,6 +425,16 @@ def _prepend_localhost(self, url): url = self.host_selector.get_local_host() + ':' + url return url + def _parse_auto_opts(self): + auto_opts = self._read_auto_opts() + if auto_opts: + automatic_options = auto_opts.split() + for option in automatic_options: + elements = option.split("=") + if len(elements) == 2: + self._add_define_option( + elements[0], '"' + elements[1] + '"') + def process(self): """Process STEM options into 'rose suite-run' options.""" # Generate options for source trees @@ -487,14 +489,7 @@ def process(self): str(expanded_groups)) # Load the config file and return any automatic-options - auto_opts = self._read_auto_opts() - if auto_opts: - automatic_options = auto_opts.split() - for option in automatic_options: - elements = option.split("=") - if len(elements) == 2: - self._add_define_option( - elements[0], '"' + elements[1] + '"') + self._parse_auto_opts() # Change into the suite directory if getattr(self.opts, 'workflow_conf_dir', None): diff --git a/cylc/rose/utilities.py b/cylc/rose/utilities.py index 16fa5570..dc7b0f72 100644 --- a/cylc/rose/utilities.py +++ b/cylc/rose/utilities.py @@ -63,8 +63,6 @@ def get_rose_vars_from_config_node(config, config_node, environ): Dictionary of environment variables """ - templating = None - # Don't allow multiple templating sections. templating = identify_templating_section(config_node) @@ -128,54 +126,43 @@ def get_rose_vars_from_config_node(config, config_node, environ): # For each of the template language sections extract items to a simple # dict to be returned. - if 'env' in config_node.value: - - config['env'] = { - item[0][1]: item[1].value for item in - config_node.value['env'].walk() - if item[1].state == ConfigNode.STATE_NORMAL - } - if templating in config_node.value: - config['template_variables'] = { - item[0][1]: item[1].value for item in - config_node.value[templating].walk() - if item[1].state == ConfigNode.STATE_NORMAL - } - elif 'template variables' in config_node.value: - config['template_variables'] = { - item[0][1]: item[1].value for item in - config_node.value['template variables'].walk() - if item[1].state == ConfigNode.STATE_NORMAL - } + config['env'] = { + item[0][1]: item[1].value for item in + config_node.value['env'].walk() + if item[1].state == ConfigNode.STATE_NORMAL + } + config['template_variables'] = { + item[0][1]: item[1].value for item in + config_node.value[templating].walk() + if item[1].state == ConfigNode.STATE_NORMAL + } # Add the entire config to ROSE_SUITE_VARIABLES to allow for programatic # access. - if templating is not None: - with patch_jinja2_leading_zeros(): - # BACK COMPAT: patch_jinja2_leading_zeros - # back support zero-padded integers for a limited time to help - # users migrate before upgrading cylc-flow to Jinja2>=3.1 - parser = Parser() - for key, value in config['template_variables'].items(): - # The special variables are already Python variables. - if key not in ['ROSE_ORIG_HOST', 'ROSE_VERSION', 'ROSE_SITE']: - try: - config['template_variables'][key] = ( - parser.literal_eval(value) - ) - except Exception: - raise ConfigProcessError( - [templating, key], - value, - f'Invalid template variable: {value}' - '\nMust be a valid Python or Jinja2 literal' - ' (note strings "must be quoted").' - ) from None + with patch_jinja2_leading_zeros(): + # BACK COMPAT: patch_jinja2_leading_zeros + # back support zero-padded integers for a limited time to help + # users migrate before upgrading cylc-flow to Jinja2>=3.1 + parser = Parser() + for key, value in config['template_variables'].items(): + # The special variables are already Python variables. + if key not in ['ROSE_ORIG_HOST', 'ROSE_VERSION', 'ROSE_SITE']: + try: + config['template_variables'][key] = ( + parser.literal_eval(value) + ) + except Exception: + raise ConfigProcessError( + [templating, key], + value, + f'Invalid template variable: {value}' + '\nMust be a valid Python or Jinja2 literal' + ' (note strings "must be quoted").' + ) from None # Add ROSE_SUITE_VARIABLES to config of templating engines in use. - if templating is not None: - config['template_variables'][ - 'ROSE_SUITE_VARIABLES'] = config['template_variables'] + config['template_variables'][ + 'ROSE_SUITE_VARIABLES'] = config['template_variables'] def identify_templating_section(config_node): @@ -244,7 +231,7 @@ def rose_config_tree_loader(srcdir=None, opts=None): if opts and 'opt_conf_keys' in dir(opts) and opts.opt_conf_keys: if isinstance(opts.opt_conf_keys, str): opt_conf_keys += opts.opt_conf_keys.split() - elif isinstance(opts.opt_conf_keys, list): + else: opt_conf_keys += opts.opt_conf_keys # Optional definitions @@ -367,6 +354,7 @@ def get_cli_opts_node(opts=None, srcdir=None): # Construct new ouput based on optional Configs: newconfig = ConfigNode() + newconfig.set(['opts'], ConfigNode()) # For each __define__ determine whether it is an env or template define. for define in defines: @@ -407,9 +395,7 @@ def get_cli_opts_node(opts=None, srcdir=None): ) # Specialised treatement of optional configs. - if 'opts' not in newconfig: - newconfig['opts'] = ConfigNode() - newconfig['opts'].value = '' + newconfig['opts'].value = '' newconfig['opts'].value = merge_opts(newconfig, opt_conf_keys) newconfig['opts'].state = '!' @@ -492,8 +478,7 @@ def merge_opts(config, opt_conf_keys): 'aleph bet gimmel' """ all_opt_conf_keys = [] - if 'opts' in config: - all_opt_conf_keys.append(config['opts'].value) + all_opt_conf_keys.append(config['opts'].value) if "ROSE_SUITE_OPT_CONF_KEYS" in os.environ: all_opt_conf_keys.append(os.environ["ROSE_SUITE_OPT_CONF_KEYS"]) if opt_conf_keys and isinstance(opt_conf_keys, str): diff --git a/tests/unit/test_functional_post_install.py b/tests/unit/test_functional_post_install.py index ffb9b79d..4cda96a9 100644 --- a/tests/unit/test_functional_post_install.py +++ b/tests/unit/test_functional_post_install.py @@ -27,15 +27,18 @@ from metomi.isodatetime.datetimeoper import DateTimeOperator +import cylc from cylc.flow.hostuserutil import get_host from cylc.rose.entry_points import ( - record_cylc_install_options, rose_fileinstall, post_install + record_cylc_install_options, rose_fileinstall, post_install, + copy_config_file ) from cylc.rose.utilities import ( ROSE_ORIG_HOST_INSTALLED_OVERRIDE_STRING, MultipleTemplatingEnginesError ) from metomi.rose.config import ConfigLoader +from metomi.rose.config_tree import ConfigTree HOST = get_host() @@ -346,13 +349,24 @@ def test_template_section_conflict( def test_rose_fileinstall_exception(tmp_path, monkeypatch): - def broken(): - raise FileNotFoundError('Any Old Error') - import os - monkeypatch.setattr(os, 'getcwd', broken) - (tmp_path / 'rose-suite.conf').touch() + """It throws an exception if you try to install files to a non existent + destination. + + (And returns to the directory you started at) + """ + def fakenode(_, __): + tree = ConfigTree() + tree.node.value = {'file': ''} + return tree + + monkeypatch.setattr( + cylc.rose.entry_points, 'rose_config_tree_loader', + fakenode + ) + monkeypatch.setattr( + cylc.rose.entry_points, "rose_config_exists", lambda x, y: True) with pytest.raises(FileNotFoundError): - rose_fileinstall(srcdir=tmp_path, rundir=tmp_path) + rose_fileinstall(srcdir=tmp_path, rundir='/oiruhgaqhnaigujhj') def test_cylc_no_rose(tmp_path): @@ -360,3 +374,14 @@ def test_cylc_no_rose(tmp_path): """ from cylc.rose.entry_points import post_install assert post_install(srcdir=tmp_path, rundir=tmp_path) is False + + +def test_copy_config_file_fails(): + """It fails when source or rundir not specified.""" + with pytest.raises(FileNotFoundError, match='both source and rundir'): + copy_config_file() + + +def test_copy_config_file_fails2(): + """It fails if source not a rose suite.""" + copy_config_file(srcdir='/foo', rundir='/bar') diff --git a/tests/unit/test_rose_opts.py b/tests/unit/test_rose_opts.py index fde1eb4b..6bdf44d2 100644 --- a/tests/unit/test_rose_opts.py +++ b/tests/unit/test_rose_opts.py @@ -86,8 +86,7 @@ def test_rose_fileinstall_validate(fixture_provide_flow, cylc_validate_cli): def test_rose_fileinstall_run(fixture_install_flow): """Workflow installs: """ - _, _, _, result, _ = fixture_install_flow - assert result.ret == 0 + fixture_install_flow def test_rose_fileinstall_rose_conf(fixture_install_flow): diff --git a/tests/unit/test_rose_stem_units.py b/tests/unit/test_rose_stem_units.py index e6f18d44..23aa6f4d 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -132,8 +132,9 @@ def test__add_define_option(get_StemRunner, capsys, exisiting_defines): 0, 'url: file:///worthwhile/foo/bar/baz/trunk@1\n' 'project: \n' - 'some waffle which ought to be ignored', - '' + 'key_with_no_value_ignored:\n' + 'some waffle which ought to be ignored\n', + None ), id='Good fcm output' ) @@ -207,7 +208,9 @@ def test__get_project_from_url( ('foo/bar', 'some_dir'), ) ) -def test__generate_name(get_StemRunner, monkeypatch, tmp_path, source, expect): +def test__generate_name( + get_StemRunner, monkeypatch, tmp_path, source, expect, caplog, capsys +): """It generates a name if StemRunner._ascertain_project fails. (This happens if the workflow source is not controlled with FCM) @@ -220,7 +223,9 @@ def test__generate_name(get_StemRunner, monkeypatch, tmp_path, source, expect): expect = tmp_path.name if expect == 'cwd' else expect stemrunner = get_StemRunner({}, {'workflow_conf_dir': source}) + stemrunner.reporter.contexts['stdout'].verbosity = 5 assert stemrunner._generate_name() == expect + assert 'Suite is named' in capsys.readouterr().out @pytest.mark.parametrize( @@ -260,6 +265,13 @@ def test__this_suite( stemrunner._this_suite() +def test__this_suite_raises_if_no_dir(get_StemRunner): + """It raises an exception if there is no suitefile""" + stemrunner = get_StemRunner({}, {'stem_sources': ['/foo']}) + with pytest.raises(RoseSuiteConfNotFoundException): + stemrunner._this_suite() + + def test__check_suite_version_fails_if_no_stem_source( get_StemRunner, tmp_path ): @@ -292,3 +304,53 @@ def test__deduce_mirror(): } project = 'someproject' StemRunner._deduce_mirror(source_dict, project) + + +def test_RoseSuiteConfNotFoundException_repr(): + """It handles dirctory not existing _at all_""" + result = RoseSuiteConfNotFoundException('/foo').__repr__() + expect = 'Suite directory /foo is not a valid directory' + assert expect in result + + +def test__ascertain_project(get_StemRunner, monkeypatch): + """It doesn't handle sub_tree if not available.""" + monkeypatch.setattr( + cylc.rose.stem.StemRunner, + '_get_project_from_url', lambda _, __: 'foo' + ) + monkeypatch.setattr( + cylc.rose.stem.StemRunner, + '_deduce_mirror', lambda _, __, ___: 'foo' + ) + stemrunner = get_StemRunner({'popen': MockPopen(( + 0, + ( + 'root: https://foo.com/\n' + 'url: https://foo.com/helloworld\n' + 'project: helloworld\n' + ), + 'stderr' + ))}) + result = stemrunner._ascertain_project('') + assert result == ('foo', '', '', '', 'foo') + + +def test_process_multiple_auto_opts(monkeypatch, get_StemRunner): + stemrunner = get_StemRunner({}, options={'defines': []}) + monkeypatch.setattr( + cylc.rose.stem.StemRunner, '_read_auto_opts', + lambda _: 'foo=bar baz=qux=quiz' + ) + stemrunner._parse_auto_opts() + assert 'foo="bar"' in stemrunner.opts.defines[0] + + +def test_process_no_auto_opts(monkeypatch, get_StemRunner): + stemrunner = get_StemRunner({}, options={'defines': []}) + monkeypatch.setattr( + cylc.rose.stem.StemRunner, '_read_auto_opts', + lambda _: '' + ) + stemrunner._parse_auto_opts() + assert stemrunner.opts.defines == [] From 79900cf07537799f4b0b8ade5187000a8885ba4c Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 5 May 2023 13:00:58 +0100 Subject: [PATCH 2/6] put back line removed in error --- cylc/rose/entry_points.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cylc/rose/entry_points.py b/cylc/rose/entry_points.py index cb365e24..4a0c8fe3 100644 --- a/cylc/rose/entry_points.py +++ b/cylc/rose/entry_points.py @@ -177,6 +177,9 @@ def record_cylc_install_options( # Create a config based on command line options: cli_config = get_cli_opts_node(opts, srcdir) + # raise error if CLI config has multiple templating sections + identify_templating_section(cli_config) + # Construct path objects representing our target files. (Path(rundir) / 'opt').mkdir(exist_ok=True) conf_filepath = Path(rundir) / 'opt/rose-suite-cylc-install.conf' From 22fad0d423f4e8f3a8f2987b28e880905bdce70d Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 22 Aug 2023 11:14:28 +0100 Subject: [PATCH 3/6] Update cylc/rose/entry_points.py --- cylc/rose/entry_points.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cylc/rose/entry_points.py b/cylc/rose/entry_points.py index 4a0c8fe3..e0d4b4d0 100644 --- a/cylc/rose/entry_points.py +++ b/cylc/rose/entry_points.py @@ -71,7 +71,8 @@ def post_install(srcdir=None, opts=None, rundir=None): srcdir=srcdir, opts=opts, rundir=rundir ) # Finally dump a log of the rose-conf in its final state. - dump_rose_log(rundir=rundir, node=results['fileinstall']) + if results['fileinstall']: + dump_rose_log(rundir=rundir, node=results['fileinstall']) return results From 1c8664377e4b1e0c2320848c563ec8a41370b480 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 9 Oct 2023 09:29:17 +0100 Subject: [PATCH 4/6] Update tests/unit/test_rose_opts.py Co-authored-by: Oliver Sanders --- tests/unit/test_rose_opts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_rose_opts.py b/tests/unit/test_rose_opts.py index 6bdf44d2..14bb9884 100644 --- a/tests/unit/test_rose_opts.py +++ b/tests/unit/test_rose_opts.py @@ -86,7 +86,7 @@ def test_rose_fileinstall_validate(fixture_provide_flow, cylc_validate_cli): def test_rose_fileinstall_run(fixture_install_flow): """Workflow installs: """ - fixture_install_flow + pass # this tests the fixture itself def test_rose_fileinstall_rose_conf(fixture_install_flow): From 15a790eb4d6f376e0d47a8230e6e5f8c49bbd270 Mon Sep 17 00:00:00 2001 From: WXTIM <26465611+wxtim@users.noreply.github.com> Date: Thu, 12 Oct 2023 10:19:03 +0100 Subject: [PATCH 5/6] f --- tests/unit/test_rose_stem_units.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/test_rose_stem_units.py b/tests/unit/test_rose_stem_units.py index f645a691..40319e67 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -358,6 +358,7 @@ def test_process_no_auto_opts(monkeypatch, get_StemRunner): stemrunner._parse_auto_opts() assert stemrunner.opts.defines == [] + @pytest.mark.parametrize( 'item, expect, stdout', ( From f2dc58105df9ef0c6373d4077c2f4c3d07660a1c Mon Sep 17 00:00:00 2001 From: WXTIM <26465611+wxtim@users.noreply.github.com> Date: Tue, 17 Oct 2023 16:46:39 +0100 Subject: [PATCH 6/6] response to review --- cylc/rose/stem.py | 6 +++++- tests/unit/test_rose_stem_units.py | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/cylc/rose/stem.py b/cylc/rose/stem.py index fff37103..11565cd9 100644 --- a/cylc/rose/stem.py +++ b/cylc/rose/stem.py @@ -430,6 +430,11 @@ def _prepend_localhost(self, url): return url def _parse_auto_opts(self): + """Load the site config file and return any automatic-options. + + Parse options in the form of a space separated list of key=value + pairs. + """ auto_opts = self._read_auto_opts() if auto_opts: automatic_options = auto_opts.split() @@ -498,7 +503,6 @@ def process(self): self.opts.defines.append( f"{self.template_section}RUN_NAMES={str(expanded_groups)}") - # Load the config file and return any automatic-options self._parse_auto_opts() # Change into the suite directory diff --git a/tests/unit/test_rose_stem_units.py b/tests/unit/test_rose_stem_units.py index 40319e67..2c142f64 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -339,7 +339,15 @@ def test__ascertain_project(get_StemRunner, monkeypatch): assert result == ('foo', '', '', '', 'foo') -def test_process_multiple_auto_opts(monkeypatch, get_StemRunner): +def test_process_multiple_auto_opts( + monkeypatch: Fixture, get_StemRunner: Fixture +) -> None: + """Read a list of options from site config. + + - Correctly splits list. + - Adds valid key=value pairs to stemrunner.options. + - Rejects malformed items. + """ stemrunner = get_StemRunner({}, options={'defines': []}) monkeypatch.setattr( cylc.rose.stem.StemRunner, '_read_auto_opts', @@ -349,7 +357,11 @@ def test_process_multiple_auto_opts(monkeypatch, get_StemRunner): assert 'foo="bar"' in stemrunner.opts.defines[0] -def test_process_no_auto_opts(monkeypatch, get_StemRunner): +def test_process_no_auto_opts( + monkeypatch: Fixture, get_StemRunner: Fixture +) -> None: + """Read an empty list of options from site config. + """ stemrunner = get_StemRunner({}, options={'defines': []}) monkeypatch.setattr( cylc.rose.stem.StemRunner, '_read_auto_opts',