From 891f4f3b2d1b2fcab1b64e0ea942d186f98b7406 Mon Sep 17 00:00:00 2001
From: Matt Clay <matt@mystile.com>
Date: Wed, 21 Feb 2018 20:09:13 -0800
Subject: [PATCH] Upgrade more code-smell tests. (#36560)

* Enhance no-dict-* code-smell tests.
* Enhance no-basestring code-smell test.
* Enhance no-get-exception code-smell test.
* Enhance empty-init code-smell test.
* Enhance required-and-default-attribute test.
* Remove unused code-smell test.
---
 test/sanity/code-smell/empty-init.json        | 11 +++++
 test/sanity/code-smell/empty-init.py          | 30 +++++++++++++
 test/sanity/code-smell/empty-init.sh          | 20 ---------
 .../code-smell/inappropriately-private.sh     | 18 --------
 test/sanity/code-smell/no-basestring.json     |  6 +++
 test/sanity/code-smell/no-basestring.py       | 28 +++++++++++++
 test/sanity/code-smell/no-basestring.sh       | 18 --------
 test/sanity/code-smell/no-dict-iteritems.json |  6 +++
 test/sanity/code-smell/no-dict-iteritems.py   | 28 +++++++++++++
 test/sanity/code-smell/no-dict-iteritems.sh   | 19 ---------
 test/sanity/code-smell/no-dict-iterkeys.json  |  6 +++
 test/sanity/code-smell/no-dict-iterkeys.py    | 28 +++++++++++++
 test/sanity/code-smell/no-dict-iterkeys.sh    | 22 ----------
 .../sanity/code-smell/no-dict-itervalues.json |  6 +++
 test/sanity/code-smell/no-dict-itervalues.py  | 28 +++++++++++++
 test/sanity/code-smell/no-dict-itervalues.sh  | 19 ---------
 test/sanity/code-smell/no-get-exception.json  |  6 +++
 test/sanity/code-smell/no-get-exception.py    | 42 +++++++++++++++++++
 test/sanity/code-smell/no-get-exception.sh    | 34 ---------------
 .../required-and-default-attributes.json      |  9 ++++
 .../required-and-default-attributes.py        | 27 ++++++++++++
 .../required-and-default-attributes.sh        | 10 -----
 test/sanity/code-smell/skip.txt               |  1 -
 23 files changed, 261 insertions(+), 161 deletions(-)
 create mode 100644 test/sanity/code-smell/empty-init.json
 create mode 100755 test/sanity/code-smell/empty-init.py
 delete mode 100755 test/sanity/code-smell/empty-init.sh
 delete mode 100755 test/sanity/code-smell/inappropriately-private.sh
 create mode 100644 test/sanity/code-smell/no-basestring.json
 create mode 100755 test/sanity/code-smell/no-basestring.py
 delete mode 100755 test/sanity/code-smell/no-basestring.sh
 create mode 100644 test/sanity/code-smell/no-dict-iteritems.json
 create mode 100755 test/sanity/code-smell/no-dict-iteritems.py
 delete mode 100755 test/sanity/code-smell/no-dict-iteritems.sh
 create mode 100644 test/sanity/code-smell/no-dict-iterkeys.json
 create mode 100755 test/sanity/code-smell/no-dict-iterkeys.py
 delete mode 100755 test/sanity/code-smell/no-dict-iterkeys.sh
 create mode 100644 test/sanity/code-smell/no-dict-itervalues.json
 create mode 100755 test/sanity/code-smell/no-dict-itervalues.py
 delete mode 100755 test/sanity/code-smell/no-dict-itervalues.sh
 create mode 100644 test/sanity/code-smell/no-get-exception.json
 create mode 100755 test/sanity/code-smell/no-get-exception.py
 delete mode 100755 test/sanity/code-smell/no-get-exception.sh
 create mode 100644 test/sanity/code-smell/required-and-default-attributes.json
 create mode 100755 test/sanity/code-smell/required-and-default-attributes.py
 delete mode 100755 test/sanity/code-smell/required-and-default-attributes.sh

diff --git a/test/sanity/code-smell/empty-init.json b/test/sanity/code-smell/empty-init.json
new file mode 100644
index 0000000000..43d0bd3501
--- /dev/null
+++ b/test/sanity/code-smell/empty-init.json
@@ -0,0 +1,11 @@
+{
+    "prefixes": [
+        "lib/ansible/modules/",
+        "lib/ansible/module_utils/",
+        "test/units/"
+    ],
+    "extensions": [
+        ".py"
+    ],
+    "output": "path-message"
+}
diff --git a/test/sanity/code-smell/empty-init.py b/test/sanity/code-smell/empty-init.py
new file mode 100755
index 0000000000..f55817dc67
--- /dev/null
+++ b/test/sanity/code-smell/empty-init.py
@@ -0,0 +1,30 @@
+#!/usr/bin/env python
+
+import os
+import re
+import sys
+
+
+def main():
+    skip = set([
+        'test/sanity/code-smell/%s' % os.path.basename(__file__),
+        # facts is grandfathered in but will break namespacing
+        # the only way to fix it is to deprecate and eventually remove it
+        # six will break namespacing but because it is bundled we should not be overriding it
+        'lib/ansible/module_utils/facts/__init__.py',
+        'lib/ansible/module_utils/six/__init__.py',
+    ])
+
+    for path in sys.argv[1:]:
+        if path in skip:
+            continue
+
+        if os.path.basename(path) != '__init__.py':
+            continue
+
+        if os.path.getsize(path) > 0:
+            print('%s: empty __init__.py required' % path)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/sanity/code-smell/empty-init.sh b/test/sanity/code-smell/empty-init.sh
deleted file mode 100755
index 3ae810f5f9..0000000000
--- a/test/sanity/code-smell/empty-init.sh
+++ /dev/null
@@ -1,20 +0,0 @@
-#!/bin/sh
-
-found=''
-
-for path in lib/ansible/modules/ lib/ansible/module_utils test/units/; do
-    # facts is grandfathered in but will break namespacing.  Only way to fix it
-    # is to deprecate and eventually remove.
-    # six will break namespacing but because it is bundled we should not be overriding it
-    files=$(find "${path}" -name __init__.py -size '+0' | sed '\!lib/ansible/module_utils/\(six\|facts\)/__init__.py!d')
-
-    if [ "${files}" ]; then
-        echo "${files}"
-        found=1
-    fi
-done
-
-if [ "${found}" ]; then
-    echo "One or more __init__.py file(s) listed above are non-empty."
-    exit 1
-fi
diff --git a/test/sanity/code-smell/inappropriately-private.sh b/test/sanity/code-smell/inappropriately-private.sh
deleted file mode 100755
index 9ee3342179..0000000000
--- a/test/sanity/code-smell/inappropriately-private.sh
+++ /dev/null
@@ -1,18 +0,0 @@
-#!/bin/sh
-
-#
-# Test that we do not access private attributes of other objects.
-#
-# * private attributes of ourself are okay: self._private.
-# * Private attributes of other objects are not: self.other._private
-#
-
-# Currently the code has many places where we're violating this test so we need
-# to clean up the code before we can enable this.  Maybe we'll need to
-# selectively blacklist modules so that we can work on this a piece at a time.
-#
-# Also need to implement whitelist for certain things like bundled libraries
-# that violate this.
-#
-# 23-10-2015: Count was 508 lines
-grep -Pri '(?<!self)\._(?!_)' "$1" | grep -v modules
diff --git a/test/sanity/code-smell/no-basestring.json b/test/sanity/code-smell/no-basestring.json
new file mode 100644
index 0000000000..776590b74d
--- /dev/null
+++ b/test/sanity/code-smell/no-basestring.json
@@ -0,0 +1,6 @@
+{
+    "extensions": [
+        ".py"
+    ],
+    "output": "path-line-column-message"
+}
diff --git a/test/sanity/code-smell/no-basestring.py b/test/sanity/code-smell/no-basestring.py
new file mode 100755
index 0000000000..e1743807b5
--- /dev/null
+++ b/test/sanity/code-smell/no-basestring.py
@@ -0,0 +1,28 @@
+#!/usr/bin/env python
+
+import os
+import re
+import sys
+
+
+def main():
+    skip = set([
+        'test/sanity/code-smell/%s' % os.path.basename(__file__),
+        'lib/ansible/module_utils/six/__init__.py',
+    ])
+
+    for path in sys.argv[1:]:
+        if path in skip:
+            continue
+
+        with open(path, 'r') as path_fd:
+            for line, text in enumerate(path_fd.readlines()):
+                match = re.search(r'(isinstance.*basestring)', text)
+
+                if match:
+                    print('%s:%d:%d: do not use `isinstance(s, basestring)`' % (
+                        path, line + 1, match.start(1) + 1))
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/sanity/code-smell/no-basestring.sh b/test/sanity/code-smell/no-basestring.sh
deleted file mode 100755
index 1a2ae23613..0000000000
--- a/test/sanity/code-smell/no-basestring.sh
+++ /dev/null
@@ -1,18 +0,0 @@
-#!/bin/sh
-
-BASESTRING_USERS=$(grep -r basestring . \
-    --exclude-dir .git \
-    --exclude-dir .tox \
-    | grep isinstance \
-    | grep -v \
-    -e test/results/ \
-    -e docs/docsite/_build/ \
-    -e docs/docsite/rst/dev_guide/testing/sanity/ \
-    -e lib/ansible/module_utils/six/__init__.py \
-    -e '^[^:]*:#'
-    )
-
-if [ "${BASESTRING_USERS}" ]; then
-    echo "${BASESTRING_USERS}"
-    exit 1
-fi
diff --git a/test/sanity/code-smell/no-dict-iteritems.json b/test/sanity/code-smell/no-dict-iteritems.json
new file mode 100644
index 0000000000..776590b74d
--- /dev/null
+++ b/test/sanity/code-smell/no-dict-iteritems.json
@@ -0,0 +1,6 @@
+{
+    "extensions": [
+        ".py"
+    ],
+    "output": "path-line-column-message"
+}
diff --git a/test/sanity/code-smell/no-dict-iteritems.py b/test/sanity/code-smell/no-dict-iteritems.py
new file mode 100755
index 0000000000..95b3d79b1d
--- /dev/null
+++ b/test/sanity/code-smell/no-dict-iteritems.py
@@ -0,0 +1,28 @@
+#!/usr/bin/env python
+
+import os
+import re
+import sys
+
+
+def main():
+    skip = set([
+        'test/sanity/code-smell/%s' % os.path.basename(__file__),
+        'lib/ansible/module_utils/six/__init__.py',
+    ])
+
+    for path in sys.argv[1:]:
+        if path in skip:
+            continue
+
+        with open(path, 'r') as path_fd:
+            for line, text in enumerate(path_fd.readlines()):
+                match = re.search(r'(?<! six)\.(iteritems)', text)
+
+                if match:
+                    print('%s:%d:%d: use `dict.items` or `ansible.module_utils.six.iteritems` instead of `dict.iteritems`' % (
+                        path, line + 1, match.start(1) + 1))
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/sanity/code-smell/no-dict-iteritems.sh b/test/sanity/code-smell/no-dict-iteritems.sh
deleted file mode 100755
index 1dbf563630..0000000000
--- a/test/sanity/code-smell/no-dict-iteritems.sh
+++ /dev/null
@@ -1,19 +0,0 @@
-#!/bin/sh
-
-ITERITEMS_USERS=$(grep -rI '\.iteritems' . \
-    --exclude-dir .git \
-    --exclude-dir .tox \
-    --exclude-dir docsite \
-    | grep -v \
-    -e 'six\.iteritems' \
-    -e lib/ansible/module_utils/six/__init__.py \
-    -e test/sanity/code-smell/no-dict-iteritems.sh \
-    )
-
-if [ "${ITERITEMS_USERS}" ]; then
-    echo 'iteritems has been removed in python3.  Alternatives:'
-    echo '    for KEY, VALUE in DICT.items():'
-    echo '    from ansible.module_utils.six import iteritems ; for KEY, VALUE in iteritems(DICT):'
-    echo "${ITERITEMS_USERS}"
-    exit 1
-fi
diff --git a/test/sanity/code-smell/no-dict-iterkeys.json b/test/sanity/code-smell/no-dict-iterkeys.json
new file mode 100644
index 0000000000..776590b74d
--- /dev/null
+++ b/test/sanity/code-smell/no-dict-iterkeys.json
@@ -0,0 +1,6 @@
+{
+    "extensions": [
+        ".py"
+    ],
+    "output": "path-line-column-message"
+}
diff --git a/test/sanity/code-smell/no-dict-iterkeys.py b/test/sanity/code-smell/no-dict-iterkeys.py
new file mode 100755
index 0000000000..5309ab7ceb
--- /dev/null
+++ b/test/sanity/code-smell/no-dict-iterkeys.py
@@ -0,0 +1,28 @@
+#!/usr/bin/env python
+
+import os
+import re
+import sys
+
+
+def main():
+    skip = set([
+        'test/sanity/code-smell/%s' % os.path.basename(__file__),
+        'lib/ansible/module_utils/six/__init__.py',
+    ])
+
+    for path in sys.argv[1:]:
+        if path in skip:
+            continue
+
+        with open(path, 'r') as path_fd:
+            for line, text in enumerate(path_fd.readlines()):
+                match = re.search(r'\.(iterkeys)', text)
+
+                if match:
+                    print('%s:%d:%d: use `dict.keys` or `for key in dict:` instead of `dict.iterkeys`' % (
+                        path, line + 1, match.start(1) + 1))
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/sanity/code-smell/no-dict-iterkeys.sh b/test/sanity/code-smell/no-dict-iterkeys.sh
deleted file mode 100755
index 5cd3e8d4e9..0000000000
--- a/test/sanity/code-smell/no-dict-iterkeys.sh
+++ /dev/null
@@ -1,22 +0,0 @@
-#!/bin/sh
-
-ITERKEYS_USERS=$(grep -r -I iterkeys . \
-    --exclude-dir .git \
-    --exclude-dir .tox \
-    --exclude-dir .idea \
-    --exclude-dir docsite \
-    --exclude-dir results \
-    | grep -v \
-    -e 'metadata-.*.json:' \
-    -e lib/ansible.egg-info/ \
-    -e lib/ansible/module_utils/six/__init__.py \
-    -e docs/docsite/rst/dev_guide/testing/sanity/ \
-    -e test/sanity/code-smell/no-dict-iterkeys.sh \
-    -e '^[^:]*:#'
-    )
-
-if [ "${ITERKEYS_USERS}" ]; then
-    echo 'iterkeys has been removed in python3.  Use "for KEY in DICT:" instead'
-    echo "${ITERKEYS_USERS}"
-    exit 1
-fi
diff --git a/test/sanity/code-smell/no-dict-itervalues.json b/test/sanity/code-smell/no-dict-itervalues.json
new file mode 100644
index 0000000000..776590b74d
--- /dev/null
+++ b/test/sanity/code-smell/no-dict-itervalues.json
@@ -0,0 +1,6 @@
+{
+    "extensions": [
+        ".py"
+    ],
+    "output": "path-line-column-message"
+}
diff --git a/test/sanity/code-smell/no-dict-itervalues.py b/test/sanity/code-smell/no-dict-itervalues.py
new file mode 100755
index 0000000000..e503ffb67e
--- /dev/null
+++ b/test/sanity/code-smell/no-dict-itervalues.py
@@ -0,0 +1,28 @@
+#!/usr/bin/env python
+
+import os
+import re
+import sys
+
+
+def main():
+    skip = set([
+        'test/sanity/code-smell/%s' % os.path.basename(__file__),
+        'lib/ansible/module_utils/six/__init__.py',
+    ])
+
+    for path in sys.argv[1:]:
+        if path in skip:
+            continue
+
+        with open(path, 'r') as path_fd:
+            for line, text in enumerate(path_fd.readlines()):
+                match = re.search(r'(?<! six)\.(itervalues)', text)
+
+                if match:
+                    print('%s:%d:%d: use `dict.values` or `ansible.module_utils.six.itervalues` instead of `dict.itervalues`' % (
+                        path, line + 1, match.start(1) + 1))
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/sanity/code-smell/no-dict-itervalues.sh b/test/sanity/code-smell/no-dict-itervalues.sh
deleted file mode 100755
index 0970bb4893..0000000000
--- a/test/sanity/code-smell/no-dict-itervalues.sh
+++ /dev/null
@@ -1,19 +0,0 @@
-#!/bin/sh
-
-ITERVALUES_USERS=$(grep -rI '\.itervalues' . \
-    --exclude-dir .git \
-    --exclude-dir .tox \
-    --exclude-dir docsite \
-    | grep -v \
-    -e 'six\.itervalues' \
-    -e lib/ansible/module_utils/six/__init__.py \
-    -e test/sanity/code-smell/no-dict-itervalues.sh \
-    )
-
-if [ "${ITERVALUES_USERS}" ]; then
-    echo 'itervalues has been removed in python3.  Alternatives:'
-    echo '    for VALUE in DICT.values():'
-    echo '    from ansible.module_utils.six import itervalues ; for VALUE in itervalues(DICT):'
-    echo "${ITERVALUES_USERS}"
-    exit 1
-fi
diff --git a/test/sanity/code-smell/no-get-exception.json b/test/sanity/code-smell/no-get-exception.json
new file mode 100644
index 0000000000..776590b74d
--- /dev/null
+++ b/test/sanity/code-smell/no-get-exception.json
@@ -0,0 +1,6 @@
+{
+    "extensions": [
+        ".py"
+    ],
+    "output": "path-line-column-message"
+}
diff --git a/test/sanity/code-smell/no-get-exception.py b/test/sanity/code-smell/no-get-exception.py
new file mode 100755
index 0000000000..d09fa23b35
--- /dev/null
+++ b/test/sanity/code-smell/no-get-exception.py
@@ -0,0 +1,42 @@
+#!/usr/bin/env python
+
+import os
+import re
+import sys
+
+
+def main():
+    skip = set([
+        'test/sanity/code-smell/%s' % os.path.basename(__file__),
+        'lib/ansible/module_utils/pycompat24.py',
+        'lib/ansible/module_utils/six/__init__.py',
+        # the following files should be fixed and removed from this list
+        'lib/ansible/modules/network/cloudengine/ce_file_copy.py',
+        'lib/ansible/modules/network/panos/panos_dag_tags.py',
+        'lib/ansible/modules/network/panos/panos_match_rule.py',
+        'lib/ansible/modules/network/panos/panos_op.py',
+        'lib/ansible/modules/system/sefcontext.py',
+    ])
+
+    basic_allow_once = True
+
+    for path in sys.argv[1:]:
+        if path in skip:
+            continue
+
+        with open(path, 'r') as path_fd:
+            for line, text in enumerate(path_fd.readlines()):
+                match = re.search(r'(get_exception)', text)
+
+                if match:
+                    if path == 'lib/ansible/module_utils/basic.py' and basic_allow_once:
+                        # basic.py is allowed to import get_exception for backwards compatibility but should not call it anywhere
+                        basic_allow_once = False
+                        continue
+
+                    print('%s:%d:%d: do not use `get_exception`' % (
+                        path, line + 1, match.start(1) + 1))
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/sanity/code-smell/no-get-exception.sh b/test/sanity/code-smell/no-get-exception.sh
deleted file mode 100755
index ba6448ec25..0000000000
--- a/test/sanity/code-smell/no-get-exception.sh
+++ /dev/null
@@ -1,34 +0,0 @@
-#!/bin/sh
-
-# We're getting rid of get_exception in our code so that we can deprecate it.
-# get_exception is no longer needed as we no longer support python-2.4 on the controller.
-
-# We eventually want pycompat24 and basic.py to be the only things on this list
-get_exception=$(find . -path ./test/runner/.tox -prune \
-        -o -path ./lib/ansible/module_utils/pycompat24.py -prune \
-        -o -path ./lib/ansible/module_utils/basic.py -prune \
-        -o -path ./lib/ansible/modules/network/panos -prune \
-        -o -path ./lib/ansible/modules/network/junos/junos_facts.py -prune \
-        -o -path ./lib/ansible/modules/network/junos/junos_package.py -prune \
-        -o -path ./lib/ansible/modules/network/cloudengine/ce_file_copy.py -prune \
-        -o -path ./lib/ansible/modules/system -prune \
-        -o -name '*.py' -type f -exec grep -H 'get_exception' '{}' '+')
-
-basic_failed=0
-
-if test "$(grep -c get_exception ./lib/ansible/module_utils/basic.py)" -gt 1 ; then
-  printf "\n== basic.py contains get_exception calls ==\n\n"
-  printf "  basic.py is allowed to import get_exception for backwards compat but\n"
-  printf "  should not call it anywhere\n"
-  basic_failed=1
-fi
-
-failures=0
-if test -n "$get_exception" ; then
-  printf "\n== Contains get_exception() calls ==\n"
-  printf "  %s\n" "$get_exception"
-  failures=$(printf "%s" "$get_exception"| wc -l)
-  failures=$((failures + 2))
-fi
-
-exit $((basic_failed + failures))
diff --git a/test/sanity/code-smell/required-and-default-attributes.json b/test/sanity/code-smell/required-and-default-attributes.json
new file mode 100644
index 0000000000..dd9ac7b1f9
--- /dev/null
+++ b/test/sanity/code-smell/required-and-default-attributes.json
@@ -0,0 +1,9 @@
+{
+    "prefixes": [
+        "lib/ansible/"
+    ],
+    "extensions": [
+        ".py"
+    ],
+    "output": "path-line-column-message"
+}
diff --git a/test/sanity/code-smell/required-and-default-attributes.py b/test/sanity/code-smell/required-and-default-attributes.py
new file mode 100755
index 0000000000..5c6f86c5b9
--- /dev/null
+++ b/test/sanity/code-smell/required-and-default-attributes.py
@@ -0,0 +1,27 @@
+#!/usr/bin/env python
+
+import os
+import re
+import sys
+
+
+def main():
+    skip = set([
+        'test/sanity/code-smell/%s' % os.path.basename(__file__),
+    ])
+
+    for path in sys.argv[1:]:
+        if path in skip:
+            continue
+
+        with open(path, 'r') as path_fd:
+            for line, text in enumerate(path_fd.readlines()):
+                match = re.search(r'(FieldAttribute.*(default|required).*(default|required))', text)
+
+                if match:
+                    print('%s:%d:%d: use only one of `default` or `required` with `FieldAttribute`' % (
+                          path, line + 1, match.start(1) + 1))
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/sanity/code-smell/required-and-default-attributes.sh b/test/sanity/code-smell/required-and-default-attributes.sh
deleted file mode 100755
index 9822a15597..0000000000
--- a/test/sanity/code-smell/required-and-default-attributes.sh
+++ /dev/null
@@ -1,10 +0,0 @@
-#!/bin/sh
-
-BASEDIR=${1-"lib/ansible"}
-cd "$BASEDIR"
-grep -r FieldAttribute . |grep 'default' | grep 'required'
-if test $? -eq 0 ; then
-    exit 1
-fi
-exit 0
-
diff --git a/test/sanity/code-smell/skip.txt b/test/sanity/code-smell/skip.txt
index bfb8b181a2..e69de29bb2 100644
--- a/test/sanity/code-smell/skip.txt
+++ b/test/sanity/code-smell/skip.txt
@@ -1 +0,0 @@
-inappropriately-private.sh