From 558fe74eb7dcc7772f86b8da8cf5a035342d2603 Mon Sep 17 00:00:00 2001 From: Vinicius Queiroz Date: Wed, 1 Jul 2026 14:21:49 -0300 Subject: [PATCH] [issue-89] fix: precision follow-up C5/C7/C9/C3 (#89) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Três achados de auditoria fora do escopo da #88: 1. C5/C7 (P1): Should Be Equal As Integers/Numbers/Strings escapavam do self-compare (C7) e literais iguais (C5) porque o check casava exato "should be equal". As variantes tipadas são igualmente tautológicas. Novo conjunto SHOULD_BE_EQUAL_KEYWORDS, com strip de prefixo de library (BuiltIn.Should Be Equal As Integers também casa). 2. C9 (P2): STARTS: / EQUALS: com prefixo vazio é catch-all (toda mensagem começa com ""), aceitando qualquer erro. Novo padrão _CATCH_ALL_PREFIX_RE ^(STARTS|EQUALS):\s*$ no OR de catch-all. STARTS: connection continua limpo. 3. C3 (P3): swallow (Run Keyword And Return Status) aninhado sob Run Keyword And Continue On Failure reportava C2b (LOW) em vez de C3 (HIGH). _swallow_status_unused passa a desembrulhar uma camada do wrapper soft-assert via _effective_swallow. Warn On Failure fica de fora de propósito (já força o teste verde). A atribuição ${status}= fica no call externo, então a detecção de status não-usado segue correta. Regressões para os três itens, com os casos de aceite que devem continuar limpos. Closes #89 --- src/falsegreen_robot/scanner.py | 42 +++++++++++++-- tests/test_scanner.py | 92 +++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/src/falsegreen_robot/scanner.py b/src/falsegreen_robot/scanner.py index 475e36a..0532971 100644 --- a/src/falsegreen_robot/scanner.py +++ b/src/falsegreen_robot/scanner.py @@ -164,6 +164,15 @@ def detect_pyramid_level(model): # expected_status values that DISABLE the check (no oracle): the request accepts any outcome. _EXPECTED_STATUS_OFF = {"any", "anything"} SWALLOW_KEYWORDS = {"run keyword and ignore error", "run keyword and return status"} +# BuiltIn equality assertions that are tautological when both sides are identical +# (C7 with a ${var}, C5 with a literal). The typed "As Integers/Numbers/Strings" +# variants are idiomatic Robot and just as vacuous as plain Should Be Equal (#89). +SHOULD_BE_EQUAL_KEYWORDS = { + "should be equal", + "should be equal as integers", + "should be equal as numbers", + "should be equal as strings", +} # Keywords that UNCONDITIONALLY end the current block: nothing after them in the # same body runs. A test case cannot use [Return], so the test-level terminators # are the keywords that always stop execution (Fail aborts, Pass Execution @@ -190,6 +199,11 @@ def detect_pyramid_level(model): # e.g. `REGEXP:.*` or `REGEXP:^.*$`. A bare `.*` is NOT this: without REGEXP: it is # glob, where `.` is literal, so it only matches messages starting with a dot. _CATCH_ALL_REGEXP_RE = re.compile(r"^REGEXP:\s*\^?\(?\.[*+]\??\)?\$?$", re.IGNORECASE) +# STARTS: / EQUALS: with an EMPTY pattern is a catch-all: every message starts with +# "" (STARTS: matches any error) and EQUALS: on empty matches messages that are +# themselves empty - both vacuous oracles for `Expect Error` (#89). A non-empty +# prefix (STARTS:Boom) is specific and stays clean. +_CATCH_ALL_PREFIX_RE = re.compile(r"^(?:STARTS|EQUALS):\s*$", re.IGNORECASE) def _norm(name): @@ -321,6 +335,27 @@ def is_swallow(keyword): return _norm(keyword) in SWALLOW_KEYWORDS +# Soft-assert wrappers that take ` *inner_args` and still let the test +# fail: a swallow nested under one is the same dropped-status C3, so peel one layer +# to find the real keyword (#89 P3). Warn On Failure is excluded on purpose - it +# already forces the test green, so nesting adds nothing to detect there. +_SOFT_ASSERT_WRAPPERS = {"run keyword and continue on failure"} + + +def _effective_swallow(call): + """Return `(keyword, [args])` of the swallow this call really runs, peeling one + soft-assert wrapper. `Run Keyword And Continue On Failure Run Keyword And Return + Status ...` -> the inner Return Status call. Returns None when it is not a + swallow (directly or wrapped).""" + kw = getattr(call, "keyword", None) + args = list(getattr(call, "args", None) or ()) + if is_swallow(kw): + return kw, args + if _norm(kw) in _SOFT_ASSERT_WRAPPERS and args and is_swallow(args[0]): + return args[0], args[1:] + return None + + _VAR_NAME_RE = re.compile(r"[\$@&]\{([^{}]+)\}") @@ -378,7 +413,7 @@ def _swallow_status_unused(calls, node=None): condition_names = _control_condition_names(node) if node is not None else set() swallows = [] for idx, c in enumerate(calls): - if type(c).__name__ != "KeywordCall" or not is_swallow(c.keyword): + if type(c).__name__ != "KeywordCall" or _effective_swallow(c) is None: continue assigned = list(getattr(c, "assign", None) or ()) if not assigned: @@ -938,7 +973,8 @@ def _call_level_smells(file, owner, calls, findings, local_keywords=None, extra_ continue if _norm(kw) == "run keyword and expect error" and args \ and (_CATCH_ALL_ERROR_RE.match((args[0] or "").strip()) - or _CATCH_ALL_REGEXP_RE.match((args[0] or "").strip())): + or _CATCH_ALL_REGEXP_RE.match((args[0] or "").strip()) + or _CATCH_ALL_PREFIX_RE.match((args[0] or "").strip())): findings.append(Finding(file, ln, owner, "C9", "expects any error (catch-all pattern)")) has_verification = True continue @@ -956,7 +992,7 @@ def _call_level_smells(file, owner, calls, findings, local_keywords=None, extra_ "expected_status=%s accepts any HTTP status" % off)) has_verification = True continue - if _norm(kw) == "should be equal" and len(args) >= 2: + if _norm(_strip_library_prefix(kw, local_keywords)) in SHOULD_BE_EQUAL_KEYWORDS and len(args) >= 2: if args[0] == args[1] and ln not in dead: code = "C7" if args[0].startswith("${") else "C5" findings.append(Finding(file, ln, owner, code, "both sides are identical")) diff --git a/tests/test_scanner.py b/tests/test_scanner.py index a2fc52d..0df8ed7 100644 --- a/tests/test_scanner.py +++ b/tests/test_scanner.py @@ -247,6 +247,52 @@ def test_c7_self_compare(tmp_path): assert "C7" in codes(tmp_path, body) +# #89 item 1: the typed Should Be Equal As variants are as tautological as the +# plain form when both sides are identical - C7 with a ${var}, C5 with a literal. +@pytest.mark.parametrize("kw", [ + "Should Be Equal As Integers", + "Should Be Equal As Numbers", + "Should Be Equal As Strings", + "BuiltIn.Should Be Equal As Integers", +]) +def test_c7_typed_should_be_equal_self_compare(tmp_path, kw): + body = """\ +*** Test Cases *** +Self + %s ${x} ${x} +""" % kw + assert "C7" in codes(tmp_path, body) + + +@pytest.mark.parametrize("kw,val", [ + ("Should Be Equal As Integers", "5"), + ("Should Be Equal As Numbers", "5"), + ("Should Be Equal As Strings", "ok"), +]) +def test_c5_typed_should_be_equal_identical_literals(tmp_path, kw, val): + body = """\ +*** Test Cases *** +Const + %s %s %s +""" % (kw, val, val) + assert "C5" in codes(tmp_path, body) + + +@pytest.mark.parametrize("step", [ + "Should Be Equal As Integers ${a} ${b}", + "Should Be Equal As Numbers 5 6", + "Should Be Equal As Strings foo bar", +]) +def test_no_c5_c7_for_distinct_typed_should_be_equal(tmp_path, step): + body = """\ +*** Test Cases *** +Distinct + %s +""" % step + found = codes(tmp_path, body) + assert "C5" not in found and "C7" not in found + + def test_c16_sleep(tmp_path): body = """\ *** Test Cases *** @@ -1065,6 +1111,30 @@ def test_c3_return_status_assigned_but_never_used(tmp_path): assert "C3" in codes(tmp_path, body) +# #89 item 3: a Return Status swallow nested under a Continue On Failure wrapper is +# the same dropped-status C3 (HIGH), not the generic C2b (LOW) it used to fall to. +def test_c3_nested_swallow_under_continue_on_failure(tmp_path): + body = """\ +*** Test Cases *** +Nested Swallow + ${status}= Run Keyword And Continue On Failure Run Keyword And Return Status Should Be Equal ${1} ${2} + Log done +""" + found = codes(tmp_path, body) + assert "C3" in found and "C2b" not in found + + +def test_no_c3_nested_swallow_status_read_later(tmp_path): + # The wrapped status IS consumed afterwards - not swallowed, so no C3. + body = """\ +*** Test Cases *** +Nested Swallow Used + ${status}= Run Keyword And Continue On Failure Run Keyword And Return Status Should Be Equal ${1} ${2} + Should Be True ${status} +""" + assert "C3" not in codes(tmp_path, body) + + def test_c3_status_form_in_keyword(tmp_path): body = """\ *** Keywords *** @@ -1195,6 +1265,28 @@ def test_no_c9_when_expect_error_equals_a_literal_star(tmp_path): assert "C9" not in codes(tmp_path, body) +# #89 item 2: STARTS: / EQUALS: with an EMPTY pattern is a catch-all. Every message +# starts with "", so STARTS: accepts any error - a vacuous oracle (C9). +@pytest.mark.parametrize("pat", ["STARTS:", "STARTS: ", "EQUALS:", "starts:"]) +def test_c9_expect_error_empty_prefix_is_catch_all(tmp_path, pat): + body = """\ +*** Test Cases *** +Expects Any Error Via Empty Prefix + Run Keyword And Expect Error %s Do Risky Thing +""" % pat + assert "C9" in codes(tmp_path, body), pat + + +def test_no_c9_when_expect_error_starts_prefix_is_nonempty(tmp_path): + # STARTS: with a real prefix stays a specific matcher - not a catch-all. + body = """\ +*** Test Cases *** +Expects A Real Prefix + Run Keyword And Expect Error STARTS: connection Do Risky Thing +""" + assert "C9" not in codes(tmp_path, body) + + def test_c9_expect_error_regexp_catch_all(tmp_path): # A REGEXP catch-all (.* / .+ / ^.*$) matches any message, so the oracle is # vacuous just like the glob star - C9.