From 5518b48dccb8d785c998b6c3173a045534ffa49e Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 12 Dec 2023 12:54:56 +0000 Subject: [PATCH] Validate `cylc clean --timeout` option more thoroughly --- cylc/flow/clean.py | 6 ---- cylc/flow/scripts/clean.py | 33 ++++++++++++++++++- tests/functional/cylc-clean/01-remote.t | 11 ++----- tests/unit/scripts/test_clean.py | 44 +++++++++++++++++++++++-- tests/unit/test_clean.py | 25 +++++--------- 5 files changed, 86 insertions(+), 33 deletions(-) diff --git a/cylc/flow/clean.py b/cylc/flow/clean.py index f16996ee635..c4434dc9244 100644 --- a/cylc/flow/clean.py +++ b/cylc/flow/clean.py @@ -44,9 +44,6 @@ Union, ) -from metomi.isodatetime.exceptions import ISO8601SyntaxError -from metomi.isodatetime.parsers import DurationParser - from cylc.flow import LOG from cylc.flow.cfgspec.glbl_cfg import glbl_cfg from cylc.flow.exceptions import ( @@ -363,9 +360,6 @@ def remote_clean( f"Cannot clean {id_} on remote platforms as the workflow database " f"is out of date/inconsistent with the global config - {exc}") - with suppress(ISO8601SyntaxError): - timeout = str(DurationParser().parse(timeout).get_seconds()) - queue: Deque[RemoteCleanQueueTuple] = deque() remote_clean_cmd = partial( _remote_clean_cmd, id_=id_, rm_dirs=rm_dirs, timeout=timeout diff --git a/cylc/flow/scripts/clean.py b/cylc/flow/scripts/clean.py index a002f1d90ab..2166a98f055 100644 --- a/cylc/flow/scripts/clean.py +++ b/cylc/flow/scripts/clean.py @@ -64,6 +64,9 @@ import sys from typing import TYPE_CHECKING, Iterable, List, Tuple +from metomi.isodatetime.exceptions import ISO8601SyntaxError +from metomi.isodatetime.parsers import DurationParser + from cylc.flow import LOG from cylc.flow.clean import init_clean, get_contained_workflows from cylc.flow.exceptions import CylcError, InputError @@ -140,6 +143,24 @@ def get_option_parser(): CleanOptions = Options(get_option_parser()) +def parse_timeout(opts: 'Values') -> None: + """Parse timeout as ISO 8601 duration or number of seconds.""" + if opts.remote_timeout: + try: + timeout = int( + DurationParser().parse(opts.remote_timeout).get_seconds() + ) + except ISO8601SyntaxError: + try: + timeout = int(opts.remote_timeout) + except ValueError: + raise InputError( + f"Invalid timeout: {opts.remote_timeout}. Must be " + "an ISO 8601 duration or number of seconds." + ) + opts.remote_timeout = str(timeout) + + def prompt(workflows: Iterable[str]) -> None: """Ask user if they want to clean the given set of workflows.""" print("Would clean the following workflows:") @@ -220,7 +241,15 @@ async def run(*ids: str, opts: 'Values') -> None: @cli_function(get_option_parser) -def main(_, opts: 'Values', *ids: str): +def main(_parser, opts: 'Values', *ids: str): + _main(opts, *ids) + + +def _main(opts: 'Values', *ids: str): + """Run the clean command. + + This is a separate function for ease of testing. + """ if cylc.flow.flags.verbosity < 2: set_timestamps(LOG, False) @@ -229,4 +258,6 @@ def main(_, opts: 'Values', *ids: str): "--local and --remote options are mutually exclusive" ) + parse_timeout(opts) + asyncio.run(run(*ids, opts=opts)) diff --git a/tests/functional/cylc-clean/01-remote.t b/tests/functional/cylc-clean/01-remote.t index 8b15caa4162..67148478eb0 100644 --- a/tests/functional/cylc-clean/01-remote.t +++ b/tests/functional/cylc-clean/01-remote.t @@ -25,7 +25,7 @@ SSH_CMD="$(cylc config -d -i "[platforms][${CYLC_TEST_PLATFORM}]ssh command") ${ if ! $SSH_CMD command -v 'tree' > '/dev/null'; then skip_all "'tree' command not available on remote host ${CYLC_TEST_HOST}" fi -set_test_number 12 +set_test_number 10 # Generate random name for symlink dirs to avoid any clashes with other tests SYM_NAME="$(mktemp -u)" @@ -108,14 +108,9 @@ __TREE__ # ----------------------------------------------------------------------------- -TEST_NAME="cylc-clean-timeout" -run_fail "$TEST_NAME" cylc clean --timeout 'PT0,1S' "$WORKFLOW_NAME" -dump_std "$TEST_NAME" -grep_ok 'cylc clean timed out after 0.1s' "${TEST_NAME}.stderr" - - TEST_NAME="cylc-clean-ok" -run_ok "$TEST_NAME" cylc clean "$WORKFLOW_NAME" +run_ok "$TEST_NAME" cylc clean "$WORKFLOW_NAME" --timeout PT2M +# (timeout opt is covered by unit tests but no harm double-checking here) dump_std "$TEST_NAME" TEST_NAME="run-dir-not-exist-post-clean.local" diff --git a/tests/unit/scripts/test_clean.py b/tests/unit/scripts/test_clean.py index dc29d7ba1b6..cc1bf2eff2f 100644 --- a/tests/unit/scripts/test_clean.py +++ b/tests/unit/scripts/test_clean.py @@ -16,11 +16,14 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . -from typing import Callable, List +from typing import Callable, List, Type, Union import pytest -from cylc.flow.scripts.clean import CleanOptions, scan, run +from cylc.flow.exceptions import InputError +from cylc.flow.scripts.clean import ( + CleanOptions, _main, parse_timeout, scan, run +) async def test_scan(tmp_run_dir): @@ -88,3 +91,40 @@ async def test_multi(tmp_run_dir: Callable, mute: List[str]): mute.clear() await run('*', opts=opts) assert mute == ['bar/pub/beer', 'baz/run1', 'foo'] + + +@pytest.mark.parametrize( + 'timeout, expected', + [('100', '100'), + ('PT1M2S', '62'), + ('', ''), + ('oopsie', InputError), + (' ', InputError)] +) +def test_parse_timeout( + timeout: str, + expected: Union[str, Type[InputError]] +): + """It should accept ISO 8601 format or number of seconds.""" + opts = CleanOptions(remote_timeout=timeout) + + if expected is InputError: + with pytest.raises(expected): + parse_timeout(opts) + else: + parse_timeout(opts) + assert opts.remote_timeout == expected + + +@pytest.mark.parametrize( + 'opts, expected_msg', + [ + ({'local_only': True, 'remote_only': True}, "mutually exclusive"), + ({'remote_timeout': 'oops'}, "Invalid timeout"), + ] +) +def test_bad_user_input(opts: dict, expected_msg: str, mute): + """It should raise an InputError for bad user input.""" + with pytest.raises(InputError) as exc_info: + _main(CleanOptions(**opts), 'blah') + assert expected_msg in str(exc_info.value) diff --git a/tests/unit/test_clean.py b/tests/unit/test_clean.py index f5c46ee3f73..a0c0ccdda1e 100644 --- a/tests/unit/test_clean.py +++ b/tests/unit/test_clean.py @@ -964,25 +964,15 @@ def mocked_remote_clean_cmd_side_effect(id_, platform, timeout, rm_dirs): assert f"{p_name} - {PlatformError.MSG_TIDY}" in caplog.text -@pytest.mark.parametrize( - 'timeout, expected', - [('100', '100'), - ('PT1M2S', '62.0')] -) def test_remote_clean__timeout( monkeymock: MonkeyMock, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, - timeout: str, - expected: str, ): - """Test remote_clean() timeout. - - - It should accept ISO 8601 format or number of seconds. - - It should give a sensible error message for return code 124. + """Test remote_clean() gives a sensible error message for return code 124. """ caplog.set_level(logging.ERROR, CYLC_LOG) - mock_remote_clean_cmd = monkeymock( + monkeymock( 'cylc.flow.clean._remote_clean_cmd', spec=_remote_clean_cmd, return_value=mock.Mock( @@ -995,10 +985,13 @@ def test_remote_clean__timeout( ) with pytest.raises(CylcError): - cylc_clean.remote_clean('blah', 'blah', timeout) - _, kwargs = mock_remote_clean_cmd.call_args - assert kwargs['timeout'] == expected - assert f"cylc clean timed out after {expected}s" in caplog.text + cylc_clean.remote_clean( + 'blah', platform_names=['blah'], timeout='blah' + ) + assert "cylc clean timed out" in caplog.text + # No need to log the remote clean cmd etc. for timeout + assert "ssh" not in caplog.text.lower() + assert "stderr" not in caplog.text.lower() @pytest.mark.parametrize(