From 268348f6da9809750e3116535f4f3fe9defc7ab1 Mon Sep 17 00:00:00 2001 From: Yoo HoJun Date: Wed, 15 Jul 2026 15:04:48 +0900 Subject: [PATCH 1/4] Add support for boolean CLI flag parameters in TOPP tools Allows input_TOPP() to designate parameters as flags (present/absent) instead of key-value pairs, persisted to params.json and session_state so run_topp() can correctly build the command line. --- src/workflow/CommandExecutor.py | 53 +++++++++++++++++++++++---------- src/workflow/StreamlitUI.py | 13 ++++++++ 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/workflow/CommandExecutor.py b/src/workflow/CommandExecutor.py index 11bb1486..042c5e11 100644 --- a/src/workflow/CommandExecutor.py +++ b/src/workflow/CommandExecutor.py @@ -268,6 +268,14 @@ def run_topp(self, tool: str, input_output: dict, custom_params: dict = {}, tool # Load merged parameters (_defaults + user overrides) for this tool instance merged_params = self.parameter_manager.get_merged_params(params_key) + + # Load flag parameter names: params.json takes priority (survives session restart), + # session_state is the live fallback during the current session. + flag_map = self.parameter_manager.get_parameters_from_json().get("_flag_params", {}) + if not flag_map: + flag_map = st.session_state.get("_topp_flag_params", {}) + flag_params: set = set(flag_map.get(params_key, [])) + # Construct commands for each process for i in range(n_processes): command = [tool] @@ -288,25 +296,40 @@ def run_topp(self, tool: str, input_output: dict, custom_params: dict = {}, tool command += [value[i]] # Add merged TOPP tool parameters (_defaults + user overrides) for k, v in merged_params.items(): - command += [f"-{k}"] - # Skip only empty strings (pass flag with no value) - # Note: 0 and 0.0 are valid values, so use explicit check - if v != "" and v is not None: - if isinstance(v, str) and "\n" in v: - command += v.split("\n") + if k in flag_params: + # CLI flag: include "-k" only when truthy, omit when false + if isinstance(v, str): + is_enabled = v.lower() == "true" else: - command += [str(v)] + is_enabled = bool(v) + if is_enabled: + command += [f"-{k}"] + continue + # Regular parameter: skip empty/None, append value otherwise + if v == "" or v is None: + continue + command += [f"-{k}"] + if isinstance(v, str) and "\n" in v: + command += v.split("\n") + else: + command += [str(v)] # Add custom parameters for k, v in custom_params.items(): - command += [f"-{k}"] - - # Skip only empty strings (pass flag with no value) - # Note: 0 and 0.0 are valid values, so use explicit check - if v != "" and v is not None: - if isinstance(v, list): - command += [str(x) for x in v] + if k in flag_params: + if isinstance(v, str): + is_enabled = v.lower() == "true" else: - command += [str(v)] + is_enabled = bool(v) + if is_enabled: + command += [f"-{k}"] + continue + if v == "" or v is None: + continue + command += [f"-{k}"] + if isinstance(v, list): + command += [str(x) for x in v] + else: + command += [str(v)] # Add threads parameter for TOPP tools command += ["-threads", str(threads_per_command)] commands.append(command) diff --git a/src/workflow/StreamlitUI.py b/src/workflow/StreamlitUI.py index 9c24dc2c..4bd53b18 100644 --- a/src/workflow/StreamlitUI.py +++ b/src/workflow/StreamlitUI.py @@ -826,6 +826,7 @@ def input_TOPP( num_cols: int = 4, exclude_parameters: List[str] = [], include_parameters: List[str] = [], + flag_parameters: List[str] = [], display_tool_name: bool = True, display_subsections: bool = True, display_subsection_tabs: bool = False, @@ -862,6 +863,18 @@ def input_TOPP( st.session_state["_topp_tool_instance_map"] = {} st.session_state["_topp_tool_instance_map"][tool_instance_name] = topp_tool_name + # Persist flag_parameters to session_state and params.json so run_topp + # can skip appending a value for these boolean CLI flags. + if "_topp_flag_params" not in st.session_state: + st.session_state["_topp_flag_params"] = {} + st.session_state["_topp_flag_params"][tool_instance_name] = list(flag_parameters) + _fp = self.parameter_manager.get_parameters_from_json() + if "_flag_params" not in _fp: + _fp["_flag_params"] = {} + _fp["_flag_params"][tool_instance_name] = list(flag_parameters) + with open(self.parameter_manager.params_file, "w", encoding="utf-8") as _f: + json.dump(_fp, _f, indent=4) + if not display_subsections: display_subsection_tabs = False if display_subsection_tabs: From 0c393bfb8a651f58833bb8458c5c3bbd2763666b Mon Sep 17 00:00:00 2001 From: Tom David Mueller Date: Thu, 16 Jul 2026 13:40:11 +0200 Subject: [PATCH 2/4] Add unit tests for TOPP flag parameter support Exercise run_topp()'s flag-aware command construction and the flag-source contract written by input_TOPP(). Working-behaviour tests cover: bare flag emission for truthy bool/"true" values, omission for falsy/"false", empty and None regular params skipped, 0/0.0 preserved, multiline splitting, custom_params scalar/list/flag handling, and params.json-vs-session_state precedence. A TestKnownCodeRabbitBugs group asserts the correct behaviour for the two CodeRabbit findings (per-tool flag fallback; list expansion / empty-list skipping). Those tests fail against the current code, documenting the defects until they are fixed. --- tests/test_topp_flag_parameters.py | 370 +++++++++++++++++++++++++++++ 1 file changed, 370 insertions(+) create mode 100644 tests/test_topp_flag_parameters.py diff --git a/tests/test_topp_flag_parameters.py b/tests/test_topp_flag_parameters.py new file mode 100644 index 00000000..dfcd1f81 --- /dev/null +++ b/tests/test_topp_flag_parameters.py @@ -0,0 +1,370 @@ +""" +Unit tests for PR #397 — flag parameter support for TOPP tools. + +PR #397 lets a caller mark certain TOPP parameters as CLI *flags*: parameters +passed by presence only (e.g. ``-force``), without a trailing value. The flag +names are persisted per tool instance by ``input_TOPP()`` into both +``st.session_state["_topp_flag_params"]`` and ``params.json["_flag_params"]``, +and consumed by ``run_topp()`` in ``src/workflow/CommandExecutor.py`` when it +builds the command line. + +These tests exercise ``run_topp()`` — the consumer that turns the persisted flag +definitions and merged parameters into an actual command. Driving ``run_topp()`` +also validates the persistence *contract* (the exact ``_flag_params`` / +``_topp_flag_params`` shapes that ``input_TOPP()`` writes), which is where the two +halves of the feature meet. + +Two groups of tests: + +* Working behaviour — the parts of the feature that function correctly. All of + these pass against the current PR code. +* ``TestKnownCodeRabbitBugs`` — tests that assert the *correct* behaviour for the + two scenarios CodeRabbit flagged as bugs. They FAIL against the current PR + code (that is the point — they document the defects) and turn green only once + the bugs are fixed. See: + - Finding 1 (per-tool flag fallback): + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023551 + - Finding 2 (list expansion / empty-list skipping): + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 +""" +import os +import sys +import json +import tempfile +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +# Add project root to path for imports +PROJECT_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +sys.path.append(PROJECT_ROOT) + +# --------------------------------------------------------------------------- +# Import the modules under test with `streamlit` (and `pyopenms`) mocked at the +# sys.modules level. Both CommandExecutor and ParameterManager do +# `import streamlit as st` at module top (ParameterManager also +# `import pyopenms as poms`). run_topp() only needs `st.session_state` to behave +# like a plain dict and never touches pyopenms, so lightweight mocks keep the +# test runnable without those heavy deps installed while still exercising the +# real command-construction logic. Mirrors tests/test_tool_instance_name.py. +# --------------------------------------------------------------------------- +mock_streamlit = MagicMock() +mock_streamlit.session_state = {} + +_original_streamlit = sys.modules.get("streamlit") +_original_pyopenms = sys.modules.get("pyopenms") +sys.modules["streamlit"] = mock_streamlit +if _original_pyopenms is None: + sys.modules["pyopenms"] = MagicMock() + +from src.workflow.ParameterManager import ParameterManager +from src.workflow.CommandExecutor import CommandExecutor + +# Restore the original modules so other test files import the real ones. The +# classes imported above keep their module-level `st`/`poms` bound to the mocks. +if _original_streamlit is not None: + sys.modules["streamlit"] = _original_streamlit +else: + sys.modules.pop("streamlit", None) +if _original_pyopenms is None: + sys.modules.pop("pyopenms", None) + +for _key in list(sys.modules.keys()): + if _key.startswith("src.workflow"): + sys.modules.pop(_key, None) + + +TOOL = "FeatureFinderMetabo" + + +@pytest.fixture(autouse=True) +def reset_session_state(): + """Give each test a fresh, empty mocked session_state.""" + mock_streamlit.session_state = {} + yield + mock_streamlit.session_state = {} + + +def build_command( + params_json=None, + session_state=None, + *, + tool=TOOL, + input_output=None, + custom_params=None, + tool_instance_name=None, +): + """ + Invoke ``run_topp()`` with the supplied ``params.json`` content and + ``session_state``, and return the single command list it builds. + + ``run_command`` / ``run_multiple_commands`` are stubbed so nothing is + executed; the built command is captured from the ``run_command`` mock. + ``max_threads`` is pinned to 1 so the trailing ``-threads`` argument is + deterministic. + """ + if input_output is None: + input_output = {"in": ["input.mzML"], "out": ["output.featureXML"]} + + params_json = dict(params_json or {}) + params_json.setdefault("max_threads", 1) + + with tempfile.TemporaryDirectory() as tmpdir: + workflow_dir = Path(tmpdir) + pm = ParameterManager(workflow_dir) + with open(pm.params_file, "w", encoding="utf-8") as f: + json.dump(params_json, f) + + mock_streamlit.session_state = dict(session_state or {}) + + executor = CommandExecutor(workflow_dir, MagicMock(), pm) + executor.run_command = MagicMock(return_value=True) + executor.run_multiple_commands = MagicMock(return_value=True) + + executor.run_topp( + tool, + input_output, + custom_params=custom_params or {}, + tool_instance_name=tool_instance_name or tool, + ) + + assert executor.run_command.call_count == 1, ( + "expected exactly one single-process command, got " + f"{executor.run_command.call_count}" + ) + return executor.run_command.call_args.args[0] + + +# --------------------------- assertion helpers ----------------------------- + +def has_flag(cmd, name): + """True if ``-name`` appears anywhere in the command.""" + return f"-{name}" in cmd + + +def token_after(cmd, name): + """The single token immediately following ``-name`` (or None if it is last).""" + idx = cmd.index(f"-{name}") + return cmd[idx + 1] if idx + 1 < len(cmd) else None + + +def values_after(cmd, name): + """All value tokens following ``-name`` up to the next ``-flag`` token.""" + idx = cmd.index(f"-{name}") + vals = [] + for tok in cmd[idx + 1:]: + if tok.startswith("-"): + break + vals.append(tok) + return vals + + +def is_bare_flag(cmd, name): + """True if ``-name`` is present with no value (next token is another flag).""" + if not has_flag(cmd, name): + return False + nxt = token_after(cmd, name) + return nxt is None or nxt.startswith("-") + + +# ============================ working behaviour ============================ + + +class TestCommandSkeleton: + def test_input_output_files_prefixed(self): + cmd = build_command() + assert cmd[0] == TOOL + assert cmd[1:5] == ["-in", "input.mzML", "-out", "output.featureXML"] + # threads pinned to 1 and always appended last + assert cmd[-2:] == ["-threads", "1"] + + def test_collected_files_passed_as_single_list(self): + # A [["a", "b"]] entry is expanded in place after its -key. + cmd = build_command(input_output={"in": [["a.mzML", "b.mzML"]], "out": ["c.featureXML"]}) + assert cmd[1:4] == ["-in", "a.mzML", "b.mzML"] + + +class TestFlagParameters: + """Flags emit a bare ``-key`` when enabled and nothing when disabled.""" + + def test_flag_true_bool_emits_bare_flag(self): + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}, TOOL: {"force": True}} + ) + assert is_bare_flag(cmd, "force") + assert "True" not in cmd + + def test_flag_string_true_emits_bare_flag(self): + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}, TOOL: {"force": "true"}} + ) + assert is_bare_flag(cmd, "force") + assert "true" not in cmd + + def test_flag_string_true_is_case_insensitive(self): + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}, TOOL: {"force": "True"}} + ) + assert is_bare_flag(cmd, "force") + + def test_flag_false_bool_omitted(self): + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}, TOOL: {"force": False}} + ) + assert not has_flag(cmd, "force") + + def test_flag_string_false_omitted(self): + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}, TOOL: {"force": "false"}} + ) + assert not has_flag(cmd, "force") + + +class TestRegularParameters: + """Non-flag merged parameters keep the existing value-appending behaviour.""" + + def test_empty_string_skipped(self): + cmd = build_command({TOOL: {"opt": ""}}) + assert not has_flag(cmd, "opt") + + def test_none_skipped(self): + cmd = build_command({TOOL: {"opt": None}}) + assert not has_flag(cmd, "opt") + + def test_zero_is_preserved(self): + # 0 and 0.0 are valid values, not "empty" — they must be passed through. + cmd = build_command({TOOL: {"min_int": 0, "min_float": 0.0}}) + assert values_after(cmd, "min_int") == ["0"] + assert values_after(cmd, "min_float") == ["0.0"] + + def test_scalar_value_appended(self): + cmd = build_command({TOOL: {"mz_tolerance": 10.5}}) + assert values_after(cmd, "mz_tolerance") == ["10.5"] + + def test_multiline_string_split_into_args(self): + cmd = build_command({TOOL: {"seq": "ALPHA\nBETA\nGAMMA"}}) + assert values_after(cmd, "seq") == ["ALPHA", "BETA", "GAMMA"] + + +class TestCustomParameters: + """custom_params share the flag set and expand non-empty lists.""" + + def test_custom_flag_truthy_bare(self): + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}}, + custom_params={"force": True}, + ) + assert is_bare_flag(cmd, "force") + + def test_custom_flag_false_omitted(self): + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}}, + custom_params={"force": False}, + ) + assert not has_flag(cmd, "force") + + def test_custom_scalar_value(self): + cmd = build_command(custom_params={"extra": 5}) + assert values_after(cmd, "extra") == ["5"] + + def test_custom_nonempty_list_expanded(self): + cmd = build_command(custom_params={"ids": ["a", "b", "c"]}) + assert values_after(cmd, "ids") == ["a", "b", "c"] + + def test_custom_empty_string_skipped(self): + cmd = build_command(custom_params={"opt": ""}) + assert not has_flag(cmd, "opt") + + +class TestFlagSourceContract: + """Where run_topp() reads the flag definitions from.""" + + def test_flag_params_loaded_from_params_json(self): + # Survives a session restart: only params.json carries the flag list. + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}, TOOL: {"force": True}}, + session_state={}, + ) + assert is_bare_flag(cmd, "force") + + def test_fallback_to_session_state_when_json_has_no_flags(self): + # params.json has no _flag_params at all -> live session_state is used. + cmd = build_command( + {TOOL: {"force": True}}, + session_state={"_topp_flag_params": {TOOL: ["force"]}}, + ) + assert is_bare_flag(cmd, "force") + + def test_params_json_takes_priority_over_session_state(self): + # params.json says "force" is a flag; session_state disagrees (empty). + # params.json wins, so force is treated as a flag (bare, no value). + cmd = build_command( + {"_flag_params": {TOOL: ["force"]}, TOOL: {"force": True}}, + session_state={"_topp_flag_params": {TOOL: []}}, + ) + assert is_bare_flag(cmd, "force") + assert "True" not in cmd + + +# ===================== known bugs flagged by CodeRabbit ===================== + + +class TestKnownCodeRabbitBugs: + """ + These assert the CORRECT behaviour for the two scenarios CodeRabbit flagged + on PR #397. They FAIL against the current PR code and are expected to pass + once each bug is fixed. They are intentionally left unmarked (no xfail) so + the failures stay visible. + """ + + def test_flag_fallback_ignored_when_other_tool_has_flags(self): + """ + Finding 1 (per-tool fallback): + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023551 + + params.json._flag_params has an entry for a DIFFERENT tool, so the + global ``if not flag_map`` check is False and the session_state fallback + for the current tool is skipped. The current tool's flag (only in + session_state) is therefore ignored and emitted as ``-force True`` + instead of a bare flag. + """ + cmd = build_command( + {"_flag_params": {"OtherTool": ["some_flag"]}, TOOL: {"force": True}}, + session_state={"_topp_flag_params": {TOOL: ["force"]}}, + ) + assert is_bare_flag(cmd, "force") + assert "True" not in cmd + + def test_merged_list_param_expanded(self): + """ + Finding 2 (merged list values stringified instead of expanded): + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 + + A list-valued merged parameter should expand into separate CLI args, not + be rendered as its Python ``str()`` (e.g. ``"['a', 'b']"``). + """ + cmd = build_command({TOOL: {"ids": ["a", "b"]}}) + assert values_after(cmd, "ids") == ["a", "b"] + + def test_merged_empty_list_param_skipped(self): + """ + Finding 2 (empty list not skipped, merged loop): + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 + + An empty-list parameter must be omitted entirely rather than emitting a + ``-key`` with no usable value. + """ + cmd = build_command({TOOL: {"ids": []}}) + assert not has_flag(cmd, "ids") + + def test_custom_empty_list_param_skipped(self): + """ + Finding 2 (empty list not skipped, custom_params loop): + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 + + An empty-list custom parameter must be omitted rather than emitting a + bare ``-key`` with no value. + """ + cmd = build_command(custom_params={"ids": []}) + assert not has_flag(cmd, "ids") From 2cd515e48d0a34d99b7043a592e2a24227b721a4 Mon Sep 17 00:00:00 2001 From: Tom David Mueller Date: Thu, 16 Jul 2026 13:54:51 +0200 Subject: [PATCH 3/4] ci: run continuous-integration on pull_request The continuous-integration workflow (which runs `pytest test_gui.py tests/`) was triggered on push only, so pull requests from forks never executed the tests/ suite as a check. Add the pull_request trigger so the test job gates PRs as well. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5ee0902b..34b5359d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,6 +1,6 @@ name: continuous-integration -on: [push] +on: [push, pull_request] jobs: test: From 5fd87a27dd44e4efcc98464bacbe6b11e8460db8 Mon Sep 17 00:00:00 2001 From: Tom David Mueller Date: Thu, 16 Jul 2026 14:29:29 +0200 Subject: [PATCH 4/4] Fix flag-parameter command construction and green the test suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address both CodeRabbit findings on run_topp() (CommandExecutor.py): - Per-tool flag fallback: consult session_state per params_key instead of only when the whole _flag_params map is empty, so one tool's saved flags no longer shadow another tool's session flags. - List handling in the non-flag branches: skip empty lists in both the merged and custom loops, and expand non-empty merged lists into separate args instead of stringifying them as "['a', 'b']". Move the four tests that pinned these findings out of TestKnownCodeRabbitBugs into the behaviour classes they now guard (they pass with the fixes applied). Also fix a pre-existing, unrelated CI failure in test_queue_manager_cancel.py: test_stopped_status_is_mapped_in_get_job_info compared JobStatus enum members by identity via a re-import, which mismatches once other test modules pop src.workflow.* from sys.modules. Compare .name instead — reload-proof and semantically equivalent. --- src/workflow/CommandExecutor.py | 15 +++-- tests/test_queue_manager_cancel.py | 4 +- tests/test_topp_flag_parameters.py | 98 +++++++++++------------------- 3 files changed, 47 insertions(+), 70 deletions(-) diff --git a/src/workflow/CommandExecutor.py b/src/workflow/CommandExecutor.py index 042c5e11..6479cb02 100644 --- a/src/workflow/CommandExecutor.py +++ b/src/workflow/CommandExecutor.py @@ -272,9 +272,10 @@ def run_topp(self, tool: str, input_output: dict, custom_params: dict = {}, tool # Load flag parameter names: params.json takes priority (survives session restart), # session_state is the live fallback during the current session. flag_map = self.parameter_manager.get_parameters_from_json().get("_flag_params", {}) - if not flag_map: - flag_map = st.session_state.get("_topp_flag_params", {}) - flag_params: set = set(flag_map.get(params_key, [])) + flag_list = flag_map.get(params_key) + if flag_list is None: + flag_list = st.session_state.get("_topp_flag_params", {}).get(params_key, []) + flag_params: set = set(flag_list) # Construct commands for each process for i in range(n_processes): @@ -305,12 +306,14 @@ def run_topp(self, tool: str, input_output: dict, custom_params: dict = {}, tool if is_enabled: command += [f"-{k}"] continue - # Regular parameter: skip empty/None, append value otherwise - if v == "" or v is None: + # Regular parameter: skip empty/None/empty-list, append value otherwise + if v == "" or v is None or (isinstance(v, list) and not v): continue command += [f"-{k}"] if isinstance(v, str) and "\n" in v: command += v.split("\n") + elif isinstance(v, list): + command += [str(x) for x in v] else: command += [str(v)] # Add custom parameters @@ -323,7 +326,7 @@ def run_topp(self, tool: str, input_output: dict, custom_params: dict = {}, tool if is_enabled: command += [f"-{k}"] continue - if v == "" or v is None: + if v == "" or v is None or (isinstance(v, list) and not v): continue command += [f"-{k}"] if isinstance(v, list): diff --git a/tests/test_queue_manager_cancel.py b/tests/test_queue_manager_cancel.py index 0f877082..c8ef44aa 100644 --- a/tests/test_queue_manager_cancel.py +++ b/tests/test_queue_manager_cancel.py @@ -153,9 +153,7 @@ def test_stopped_status_is_mapped_in_get_job_info(monkeypatch): info = qm.get_job_info("stopped-job") assert info is not None - assert info.status == __import__( - "src.workflow.QueueManager", fromlist=["JobStatus"] - ).JobStatus.CANCELED, ( + assert info.status.name == "CANCELED", ( "RQ 'stopped' status should be reported as CANCELED to the UI; " "otherwise stopped jobs appear stuck in 'queued'." ) diff --git a/tests/test_topp_flag_parameters.py b/tests/test_topp_flag_parameters.py index dfcd1f81..627f50ee 100644 --- a/tests/test_topp_flag_parameters.py +++ b/tests/test_topp_flag_parameters.py @@ -14,18 +14,14 @@ ``_topp_flag_params`` shapes that ``input_TOPP()`` writes), which is where the two halves of the feature meet. -Two groups of tests: - -* Working behaviour — the parts of the feature that function correctly. All of - these pass against the current PR code. -* ``TestKnownCodeRabbitBugs`` — tests that assert the *correct* behaviour for the - two scenarios CodeRabbit flagged as bugs. They FAIL against the current PR - code (that is the point — they document the defects) and turn green only once - the bugs are fixed. See: +The suite covers the working behaviour of the feature and also guards the two +issues CodeRabbit flagged during review, which are now fixed in ``run_topp()``: - Finding 1 (per-tool flag fallback): https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023551 - Finding 2 (list expansion / empty-list skipping): https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 +The tests that pin those findings carry a "CodeRabbit finding N" note in their +docstrings and live alongside the related behaviour they protect. """ import os import sys @@ -246,6 +242,24 @@ def test_multiline_string_split_into_args(self): cmd = build_command({TOOL: {"seq": "ALPHA\nBETA\nGAMMA"}}) assert values_after(cmd, "seq") == ["ALPHA", "BETA", "GAMMA"] + def test_merged_list_param_expanded(self): + """ + CodeRabbit finding 2 (fixed): a list-valued merged parameter expands into + separate CLI args, not its Python ``str()`` (e.g. ``"['a', 'b']"``). + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 + """ + cmd = build_command({TOOL: {"ids": ["a", "b"]}}) + assert values_after(cmd, "ids") == ["a", "b"] + + def test_merged_empty_list_param_skipped(self): + """ + CodeRabbit finding 2 (fixed): an empty-list merged parameter is omitted + entirely rather than emitting a ``-key`` with no usable value. + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 + """ + cmd = build_command({TOOL: {"ids": []}}) + assert not has_flag(cmd, "ids") + class TestCustomParameters: """custom_params share the flag set and expand non-empty lists.""" @@ -276,6 +290,15 @@ def test_custom_empty_string_skipped(self): cmd = build_command(custom_params={"opt": ""}) assert not has_flag(cmd, "opt") + def test_custom_empty_list_param_skipped(self): + """ + CodeRabbit finding 2 (fixed): an empty-list custom parameter is omitted + rather than emitting a bare ``-key`` with no value. + https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 + """ + cmd = build_command(custom_params={"ids": []}) + assert not has_flag(cmd, "ids") + class TestFlagSourceContract: """Where run_topp() reads the flag definitions from.""" @@ -306,28 +329,14 @@ def test_params_json_takes_priority_over_session_state(self): assert is_bare_flag(cmd, "force") assert "True" not in cmd - -# ===================== known bugs flagged by CodeRabbit ===================== - - -class TestKnownCodeRabbitBugs: - """ - These assert the CORRECT behaviour for the two scenarios CodeRabbit flagged - on PR #397. They FAIL against the current PR code and are expected to pass - once each bug is fixed. They are intentionally left unmarked (no xfail) so - the failures stay visible. - """ - - def test_flag_fallback_ignored_when_other_tool_has_flags(self): + def test_flag_fallback_uses_current_tool_when_other_tool_has_flags(self): """ - Finding 1 (per-tool fallback): + CodeRabbit finding 1 (fixed): when params.json._flag_params holds an entry + for a DIFFERENT tool, the current tool's flags must still be read from the + session_state fallback. Previously the global ``if not flag_map`` check + skipped the fallback whenever any tool had flags, so the current tool's + flag was treated as a regular parameter and emitted as ``-force True``. https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023551 - - params.json._flag_params has an entry for a DIFFERENT tool, so the - global ``if not flag_map`` check is False and the session_state fallback - for the current tool is skipped. The current tool's flag (only in - session_state) is therefore ignored and emitted as ``-force True`` - instead of a bare flag. """ cmd = build_command( {"_flag_params": {"OtherTool": ["some_flag"]}, TOOL: {"force": True}}, @@ -335,36 +344,3 @@ def test_flag_fallback_ignored_when_other_tool_has_flags(self): ) assert is_bare_flag(cmd, "force") assert "True" not in cmd - - def test_merged_list_param_expanded(self): - """ - Finding 2 (merged list values stringified instead of expanded): - https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 - - A list-valued merged parameter should expand into separate CLI args, not - be rendered as its Python ``str()`` (e.g. ``"['a', 'b']"``). - """ - cmd = build_command({TOOL: {"ids": ["a", "b"]}}) - assert values_after(cmd, "ids") == ["a", "b"] - - def test_merged_empty_list_param_skipped(self): - """ - Finding 2 (empty list not skipped, merged loop): - https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 - - An empty-list parameter must be omitted entirely rather than emitting a - ``-key`` with no usable value. - """ - cmd = build_command({TOOL: {"ids": []}}) - assert not has_flag(cmd, "ids") - - def test_custom_empty_list_param_skipped(self): - """ - Finding 2 (empty list not skipped, custom_params loop): - https://github.com/OpenMS/streamlit-template/pull/397#discussion_r3585023558 - - An empty-list custom parameter must be omitted rather than emitting a - bare ``-key`` with no value. - """ - cmd = build_command(custom_params={"ids": []}) - assert not has_flag(cmd, "ids")