compare: fix mutually-exclusive false-positive on --baseline-tag + --comparison-branch (#535)

* compare: fix mutually-exclusive false-positive on --baseline-tag + --comparison-branch

Why:
- The baseline-tag conflict check in get_by_strings was reading
  comparison_covered, so any combination of --baseline-tag with
  --comparison-branch errored out as "--baseline-branch and
  --baseline-tag are mutually exclusive" — even though no baseline
  branch was provided. This blocked tag-vs-branch release verification
  comparisons that would otherwise be valid.
- Separately, when defaults.yml sets baseline-branch, the fallback
  applied even when the user passed --baseline-tag, re-triggering the
  same spurious error.

How:
- get_by_strings: check baseline_covered (not comparison_covered) for
  the --baseline-tag conflict.
- Extract resolve_baseline_branch() helper. When --baseline-tag is
  provided, skip the defaults-file branch fallback so the tag-only
  path stays valid through get_by_strings.
- Add tests/test_compare_validation.py with 12 unit tests covering
  every valid combination, every invalid combination (still errors),
  and the defaults-fallback skip logic.

* Bumping version from 0.12.29 to 0.12.30

Author: fcostaoliveira

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "redisbench-admin"
-version = "0.12.29"
+version = "0.12.30"
 description = "Redis benchmark run helper. A wrapper around Redis and Redis Modules benchmark tools ( ftsb_redisearch, memtier_benchmark, redis-benchmark, aibench, etc... )."
 authors = ["filipecosta90 <filipecosta.90@gmail.com>","Redis Performance Group <performance@redis.com>"]
 readme = "README.md"

redisbench_admin/compare/compare.py

@@ -380,14 +380,19 @@ def compare_command_logic(args, project_name, project_version):
     to_ts_ms = args.to_timestamp
     from_date = args.from_date
     to_date = args.to_date
-    baseline_branch = args.baseline_branch
-    if baseline_branch is None and default_baseline_branch is not None:
+    baseline_branch = resolve_baseline_branch(
+        args.baseline_branch, args.baseline_tag, default_baseline_branch
+    )
+    if (
+        baseline_branch is not None
+        and args.baseline_branch is None
+        and args.baseline_tag is None
+    ):
         logging.info(
             "Given --baseline-branch was null using the default baseline branch {}".format(
-                default_baseline_branch
+                baseline_branch
             )
         )
-        baseline_branch = default_baseline_branch
     comparison_branch = args.comparison_branch
     simplify_table = args.simple_table
     print_regressions_only = args.print_regressions_only
@@ -1239,6 +1244,23 @@ def get_percentage_value(row):
     )
 
 
+def resolve_baseline_branch(
+    arg_baseline_branch, arg_baseline_tag, default_baseline_branch
+):
+    """Resolve the effective baseline branch from CLI args + defaults-file fallback.
+
+    The defaults-file branch is only used when the user provided neither
+    --baseline-branch nor --baseline-tag. If --baseline-tag is set the
+    fallback is skipped, otherwise get_by_strings would raise a spurious
+    "mutually exclusive" error.
+    """
+    if arg_baseline_branch is not None:
+        return arg_baseline_branch
+    if arg_baseline_tag is not None:
+        return None
+    return default_baseline_branch
+
+
 def get_by_strings(
     baseline_branch,
     comparison_branch,
@@ -1261,7 +1283,7 @@ def get_by_strings(
         comparison_str = comparison_branch
 
     if baseline_tag is not None:
-        if comparison_covered:
+        if baseline_covered:
             logging.error(
                 "--baseline-branch and --baseline-tag are mutually exclusive. Pick one..."
             )

tests/test_compare_validation.py

@@ -0,0 +1,173 @@
+import logging
+
+import pytest
+
+from redisbench_admin.compare.compare import (
+    get_by_strings,
+    resolve_baseline_branch,
+)
+
+
+# ---------------------------------------------------------------------------
+# get_by_strings — valid combinations
+# ---------------------------------------------------------------------------
+
+
+def test_get_by_strings_branch_vs_branch():
+    baseline_str, by_baseline, comparison_str, by_comparison = get_by_strings(
+        baseline_branch="master",
+        comparison_branch="feature",
+        baseline_tag=None,
+        comparison_tag=None,
+    )
+    assert (baseline_str, by_baseline) == ("master", "branch")
+    assert (comparison_str, by_comparison) == ("feature", "branch")
+
+
+def test_get_by_strings_tag_baseline_branch_comparison():
+    """Regression: this combo previously errored as mutually exclusive
+    because the baseline-tag check was reading comparison_covered."""
+    baseline_str, by_baseline, comparison_str, by_comparison = get_by_strings(
+        baseline_branch=None,
+        comparison_branch="feature",
+        baseline_tag="v1.0",
+        comparison_tag=None,
+    )
+    assert (baseline_str, by_baseline) == ("v1.0", "version")
+    assert (comparison_str, by_comparison) == ("feature", "branch")
+
+
+def test_get_by_strings_branch_baseline_tag_comparison():
+    baseline_str, by_baseline, comparison_str, by_comparison = get_by_strings(
+        baseline_branch="master",
+        comparison_branch=None,
+        baseline_tag=None,
+        comparison_tag="v2.0",
+    )
+    assert (baseline_str, by_baseline) == ("master", "branch")
+    assert (comparison_str, by_comparison) == ("v2.0", "version")
+
+
+def test_get_by_strings_tag_vs_tag():
+    baseline_str, by_baseline, comparison_str, by_comparison = get_by_strings(
+        baseline_branch=None,
+        comparison_branch=None,
+        baseline_tag="v1.0",
+        comparison_tag="v2.0",
+    )
+    assert (baseline_str, by_baseline) == ("v1.0", "version")
+    assert (comparison_str, by_comparison) == ("v2.0", "version")
+
+
+# ---------------------------------------------------------------------------
+# get_by_strings — invalid combinations should still raise SystemExit
+# ---------------------------------------------------------------------------
+
+
+def test_get_by_strings_baseline_branch_and_tag_rejected(caplog):
+    with caplog.at_level(logging.ERROR), pytest.raises(SystemExit) as excinfo:
+        get_by_strings(
+            baseline_branch="master",
+            comparison_branch="feature",
+            baseline_tag="v1.0",
+            comparison_tag=None,
+        )
+    assert excinfo.value.code == 1
+    assert any(
+        "baseline-branch and --baseline-tag are mutually exclusive" in r.message
+        for r in caplog.records
+    )
+
+
+def test_get_by_strings_comparison_branch_and_tag_rejected(caplog):
+    with caplog.at_level(logging.ERROR), pytest.raises(SystemExit) as excinfo:
+        get_by_strings(
+            baseline_branch="master",
+            comparison_branch="feature",
+            baseline_tag=None,
+            comparison_tag="v2.0",
+        )
+    assert excinfo.value.code == 1
+    assert any(
+        "comparison-branch and --comparison-tag are mutually exclusive" in r.message
+        for r in caplog.records
+    )
+
+
+def test_get_by_strings_no_baseline_rejected(caplog):
+    with caplog.at_level(logging.ERROR), pytest.raises(SystemExit) as excinfo:
+        get_by_strings(
+            baseline_branch=None,
+            comparison_branch="feature",
+            baseline_tag=None,
+            comparison_tag=None,
+        )
+    assert excinfo.value.code == 1
+    assert any(
+        "--baseline-branch or --baseline-tag" in r.message for r in caplog.records
+    )
+
+
+def test_get_by_strings_no_comparison_rejected(caplog):
+    with caplog.at_level(logging.ERROR), pytest.raises(SystemExit) as excinfo:
+        get_by_strings(
+            baseline_branch="master",
+            comparison_branch=None,
+            baseline_tag=None,
+            comparison_tag=None,
+        )
+    assert excinfo.value.code == 1
+    assert any(
+        "--comparison-branch or --comparison-tag" in r.message for r in caplog.records
+    )
+
+
+# ---------------------------------------------------------------------------
+# resolve_baseline_branch — defaults-file fallback
+# ---------------------------------------------------------------------------
+
+
+def test_resolve_baseline_branch_explicit_branch_wins():
+    assert (
+        resolve_baseline_branch(
+            arg_baseline_branch="explicit",
+            arg_baseline_tag=None,
+            default_baseline_branch="master",
+        )
+        == "explicit"
+    )
+
+
+def test_resolve_baseline_branch_falls_back_to_default():
+    assert (
+        resolve_baseline_branch(
+            arg_baseline_branch=None,
+            arg_baseline_tag=None,
+            default_baseline_branch="master",
+        )
+        == "master"
+    )
+
+
+def test_resolve_baseline_branch_skips_fallback_when_tag_set():
+    """Regression: if --baseline-tag is given, the defaults-file branch must
+    not be auto-applied — otherwise get_by_strings raises mutually exclusive."""
+    assert (
+        resolve_baseline_branch(
+            arg_baseline_branch=None,
+            arg_baseline_tag="v1.0",
+            default_baseline_branch="master",
+        )
+        is None
+    )
+
+
+def test_resolve_baseline_branch_no_default_no_fallback():
+    assert (
+        resolve_baseline_branch(
+            arg_baseline_branch=None,
+            arg_baseline_tag=None,
+            default_baseline_branch=None,
+        )
+        is None
+    )