From 333b4e0b6bcc6597495a251831faab2ad7fb6177 Mon Sep 17 00:00:00 2001
From: Sorin Sbarnea <ssbarnea@redhat.com>
Date: Thu, 18 Mar 2021 18:43:47 +0000
Subject: [PATCH] Improve empty-string-compare rule (#1480)

- rewrite matching to avoid line processing and trigger only on when
  blocks, highly reducing the chance of false-positives
- add additional pass test to avoid future regression
- refactor rule to use embedded tests

Fixes: #1232
---
 .../rules/ComparisonToEmptyStringRule.py      | 67 ++++++++++++++++++-
 test/TestComparisonToEmptyString.py           | 39 -----------
 2 files changed, 64 insertions(+), 42 deletions(-)
 delete mode 100644 test/TestComparisonToEmptyString.py

diff --git a/src/ansiblelint/rules/ComparisonToEmptyStringRule.py b/src/ansiblelint/rules/ComparisonToEmptyStringRule.py
index 1b6faa9759..c16813cf39 100644
--- a/src/ansiblelint/rules/ComparisonToEmptyStringRule.py
+++ b/src/ansiblelint/rules/ComparisonToEmptyStringRule.py
@@ -2,9 +2,11 @@
 # Copyright (c) 2018, Ansible Project
 
 import re
-from typing import Union
+import sys
+from typing import Any, Dict, Union
 
 from ansiblelint.rules import AnsibleLintRule
+from ansiblelint.utils import nested_items
 
 
 class ComparisonToEmptyStringRule(AnsibleLintRule):
@@ -20,5 +22,64 @@ class ComparisonToEmptyStringRule(AnsibleLintRule):
 
     empty_string_compare = re.compile("[=!]= ?(\"{2}|'{2})")
 
-    def match(self, line: str) -> Union[bool, str]:
-        return bool(self.empty_string_compare.search(line))
+    def matchtask(self, task: Dict[str, Any]) -> Union[bool, str]:
+        for k, v, _ in nested_items(task):
+            if k == 'when':
+                if isinstance(v, str):
+                    if self.empty_string_compare.search(v):
+                        return True
+                elif isinstance(v, bool):
+                    pass
+                else:
+                    for item in v:
+                        if isinstance(item, str) and self.empty_string_compare.search(
+                            item
+                        ):
+                            return True
+
+        return False
+
+
+# testing code to be loaded only with pytest or when executed the rule file
+if "pytest" in sys.modules:
+
+    import pytest
+
+    SUCCESS_PLAY = '''
+- hosts: all
+  tasks:
+    - name: shut down
+      shell: |
+        /sbin/shutdown -t now
+        echo $var == ""
+      when: ansible_os_family
+'''
+
+    FAIL_PLAY = '''
+- hosts: all
+  tasks:
+  - name: shut down
+    command: /sbin/shutdown -t now
+    when: ansible_os_family == ""
+  - name: shut down
+    command: /sbin/shutdown -t now
+    when: ansible_os_family !=""
+'''
+
+    @pytest.mark.parametrize(
+        'rule_runner', (ComparisonToEmptyStringRule,), indirect=['rule_runner']
+    )
+    def test_rule_empty_string_compare_fail(rule_runner: Any) -> None:
+        """Test rule matches."""
+        results = rule_runner.run_playbook(FAIL_PLAY)
+        assert len(results) == 2
+        for result in results:
+            assert result.message == ComparisonToEmptyStringRule.shortdesc
+
+    @pytest.mark.parametrize(
+        'rule_runner', (ComparisonToEmptyStringRule,), indirect=['rule_runner']
+    )
+    def test_rule_empty_string_compare_pass(rule_runner: Any) -> None:
+        """Test rule matches."""
+        results = rule_runner.run_playbook(SUCCESS_PLAY)
+        assert len(results) == 0, results
diff --git a/test/TestComparisonToEmptyString.py b/test/TestComparisonToEmptyString.py
deleted file mode 100644
index cdbd0faac3..0000000000
--- a/test/TestComparisonToEmptyString.py
+++ /dev/null
@@ -1,39 +0,0 @@
-# pylint: disable=preferred-module  # FIXME: remove once migrated per GH-725
-import unittest
-
-from ansiblelint.rules import RulesCollection
-from ansiblelint.rules.ComparisonToEmptyStringRule import ComparisonToEmptyStringRule
-from ansiblelint.testing import RunFromText
-
-SUCCESS_TASKS = '''
-- name: shut down
-  command: /sbin/shutdown -t now
-  when: ansible_os_family
-'''
-
-FAIL_TASKS = '''
-- hosts: all
-  tasks:
-  - name: shut down
-    command: /sbin/shutdown -t now
-    when: ansible_os_family == ""
-  - name: shut down
-    command: /sbin/shutdown -t now
-    when: ansible_os_family !=""
-'''
-
-
-class TestComparisonToEmptyStringRule(unittest.TestCase):
-    collection = RulesCollection()
-    collection.register(ComparisonToEmptyStringRule())
-
-    def setUp(self):
-        self.runner = RunFromText(self.collection)
-
-    def test_success(self):
-        results = self.runner.run_role_tasks_main(SUCCESS_TASKS)
-        self.assertEqual(0, len(results))
-
-    def test_fail(self):
-        results = self.runner.run_playbook(FAIL_TASKS)
-        self.assertEqual(2, len(results))