From cd4de398bf0a560ea7909bd8286310d7e6de2091 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 24 Aug 2023 09:31:51 +0100 Subject: [PATCH 1/2] Prevent accidental manual setting of project name with -s= Before `-s=name` would lead to project name being manually set to '' rather than using FCM to setup a project name. This is a correct from the point of view of optparse, but might seem surprising to users. --- CHANGES.md | 7 +++++ cylc/rose/stem.py | 2 +- tests/unit/test_rose_stem_units.py | 46 ++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 7b5489a2..d5fe9cfd 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -6,6 +6,13 @@ creating a new release entry be sure to copy & paste the span tag with the updated. Only the first match gets replaced, so it's fine to leave the old ones in. --> +## __cylc-rose-1.3.1 (Upcoming)__ + +### Fixes + +[#250](https://github.com/cylc/cylc-rose/pull/250) - Prevent project +name being manually set to an empty string. + ## __cylc-rose-1.3.0 (Released 2023-07-21)__ ### Fixes diff --git a/cylc/rose/stem.py b/cylc/rose/stem.py index a5c2b494..76b9086c 100644 --- a/cylc/rose/stem.py +++ b/cylc/rose/stem.py @@ -340,7 +340,7 @@ def _ascertain_project(self, item): if re.search(r'^\.', item): item = os.path.abspath(os.path.join(os.getcwd(), item)) - if project is not None: + if project: print(f"[WARN] Forcing project for '{item}' to be '{project}'") return project, item, item, '', '' diff --git a/tests/unit/test_rose_stem_units.py b/tests/unit/test_rose_stem_units.py index 3f0d868f..53425660 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -20,6 +20,7 @@ import pytest from pytest import param from types import SimpleNamespace +from typing import Any, Tuple from cylc.rose.stem import ( ProjectNotFoundException, @@ -34,6 +35,9 @@ from metomi.rose.fs_util import FileSystemUtil +Fixture = Any + + class MockPopen: def __init__(self, mocked_return): self.mocked_return = mocked_return @@ -291,3 +295,45 @@ def test__deduce_mirror(): } project = 'someproject' StemRunner._deduce_mirror(source_dict, project) + + +@pytest.mark.parametrize( + 'item, expect, stdout', + ( + ( + # Normally formed project=url + 'foo=bar', + ('foo', 'bar'), + "Forcing project for 'bar' to be 'foo'", + ), + ( + # Malformed project=url + '=foo', + ('', 'foo'), + None, + ), + ) +) +def test_ascertain_project_if_name_supplied( + get_StemRunner: Fixture, + capsys: Fixture, + item: str, + expect: Tuple[str], + stdout: str, +) -> None: + """Method gives sensible results for different CLI input. + + Written because `-s=foo` leads to "item" including a leading = sign. + This led to project name being set to '', and skipping the FCM + calling logic, which the user might expect. + """ + stemrunner = get_StemRunner({}) + if stdout: + results = stemrunner._ascertain_project(item) + assert results[:2] == expect + assert stdout in capsys.readouterr().out + else: + with pytest.raises( + ProjectNotFoundException, match='is not a working copy' + ): + stemrunner._ascertain_project(item) From 717412bb6597238e7509b7e7b146968a29071afe Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 24 Aug 2023 14:46:30 +0100 Subject: [PATCH 2/2] 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 53425660..4dfc57fd 100644 --- a/tests/unit/test_rose_stem_units.py +++ b/tests/unit/test_rose_stem_units.py @@ -316,7 +316,7 @@ def test__deduce_mirror(): ) def test_ascertain_project_if_name_supplied( get_StemRunner: Fixture, - capsys: Fixture, + capsys: pytest.CaptureFixture, item: str, expect: Tuple[str], stdout: str,