Validate cylc clean --timeout option more thoroughly

Author: MetRonnie

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

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))

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"

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 <http://www.gnu.org/licenses/>.
 
-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)

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(