From f4d7b9a59658a12fe44a5e463007cd9d9e334d76 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Tue, 1 Aug 2017 16:18:27 -0700 Subject: [PATCH] code-smell test changes * Create get_exception and wildcard import code-smell tests * Add more detail to boilerplate and no-basestring descriptions * Remove the no-list-cmp test as the pylint undefined-variable test covers it --- .../dev_guide/testing/sanity/boilerplate.rst | 3 +- .../testing/sanity/no-basestring.rst | 9 +++- .../testing/sanity/no-get-exception.rst | 28 +++++++++++++ .../dev_guide/testing/sanity/no-list-cmp.rst | 8 ---- .../testing/sanity/no-wildcard-import.rst | 29 +++++++++++++ test/sanity/code-smell/boilerplate.sh | 32 +++----------- test/sanity/code-smell/no-get-exception.sh | 36 ++++++++++++++++ test/sanity/code-smell/no-list-cmp.sh | 18 -------- test/sanity/code-smell/no-wildcard-import.sh | 42 +++++++++++++++++++ 9 files changed, 149 insertions(+), 56 deletions(-) create mode 100644 docs/docsite/rst/dev_guide/testing/sanity/no-get-exception.rst delete mode 100644 docs/docsite/rst/dev_guide/testing/sanity/no-list-cmp.rst create mode 100644 docs/docsite/rst/dev_guide/testing/sanity/no-wildcard-import.rst create mode 100755 test/sanity/code-smell/no-get-exception.sh delete mode 100755 test/sanity/code-smell/no-list-cmp.sh create mode 100755 test/sanity/code-smell/no-wildcard-import.sh diff --git a/docs/docsite/rst/dev_guide/testing/sanity/boilerplate.rst b/docs/docsite/rst/dev_guide/testing/sanity/boilerplate.rst index a6fba7c4064..e07d84654cc 100644 --- a/docs/docsite/rst/dev_guide/testing/sanity/boilerplate.rst +++ b/docs/docsite/rst/dev_guide/testing/sanity/boilerplate.rst @@ -1,11 +1,10 @@ Sanity Tests » boilerplate ========================== -Most Python files other than those under ``lib/ansible/modules/`` should include the following boilerplate: +Most Python files should include the following boilerplate: .. code-block:: python from __future__ import (absolute_import, division, print_function) - __metaclass__ = type diff --git a/docs/docsite/rst/dev_guide/testing/sanity/no-basestring.rst b/docs/docsite/rst/dev_guide/testing/sanity/no-basestring.rst index c4508ce42b0..1c44da6bd5f 100644 --- a/docs/docsite/rst/dev_guide/testing/sanity/no-basestring.rst +++ b/docs/docsite/rst/dev_guide/testing/sanity/no-basestring.rst @@ -1,4 +1,11 @@ Sanity Tests » no-basestring ============================ -Do not use ``isinstance(s, basestring)``. +Do not use ``isinstance(s, basestring)`` as basestring has been removed in +Python3. You can import ``string_types``, ``binary_type``, or ``text_type`` +from ``ansible.module_utils.six`` and then use ``isinstance(s, string_types)`` +or ``isinstance(s, (binary_type, text_type))`` instead. + +If this is part of code to convert a string to a particular type, +``ansible.module_utils._text`` contains several functions that may be even +better for you: ``to_text``, ``to_bytes``, and ``to_native``. diff --git a/docs/docsite/rst/dev_guide/testing/sanity/no-get-exception.rst b/docs/docsite/rst/dev_guide/testing/sanity/no-get-exception.rst new file mode 100644 index 00000000000..e9b419fffe4 --- /dev/null +++ b/docs/docsite/rst/dev_guide/testing/sanity/no-get-exception.rst @@ -0,0 +1,28 @@ +Sanity Tests » no-get-exception +=============================== + +We created a function, ``ansible.module_utils.pycompat24.get_exception`` to +help retrieve exceptions in a manner compatible with Python-2.4 through +Python-3.6. We no longer support Python-2.4 and Python-2.5 so this is +extraneous and we want to deprecate the function. Porting code should look +something like this: + +.. code-block:: python + + # Unfixed code: + try: + raise IOError('test') + except IOError: + e = get_excetion() + do_something(e) + except: + e = get_exception() + do_something_else(e) + + # After fixing: + try: + raise IOError('test') + except IOErrors as e: + do_something(e) + except Exception as e: + do_something_else(e) diff --git a/docs/docsite/rst/dev_guide/testing/sanity/no-list-cmp.rst b/docs/docsite/rst/dev_guide/testing/sanity/no-list-cmp.rst deleted file mode 100644 index 6678ecb2278..00000000000 --- a/docs/docsite/rst/dev_guide/testing/sanity/no-list-cmp.rst +++ /dev/null @@ -1,8 +0,0 @@ -Sanity Tests » no-list-cmp -========================== - -The ``cmp`` function has been removed in Python 3. - -See `When sorting, use key instead of cmp`_. - -.. _When sorting, use key instead of cmp: http://python3porting.com/preparing.html#when-sorting-use-key-instead-of-cmp diff --git a/docs/docsite/rst/dev_guide/testing/sanity/no-wildcard-import.rst b/docs/docsite/rst/dev_guide/testing/sanity/no-wildcard-import.rst new file mode 100644 index 00000000000..1cda220ebc9 --- /dev/null +++ b/docs/docsite/rst/dev_guide/testing/sanity/no-wildcard-import.rst @@ -0,0 +1,29 @@ +Sanity Tests » no-wildcard-import +================================= + +Using :code:`import *` is a bad habit which pollutes your namespace, hinders +debugging, and interferes with static analysis of code. For those reasons, we +do want to limit the use of :code:`import *` in the ansible code. Change our +code to import the specific names that you need instead. + +Examples of unfixed code: + +.. code-block:: python + + from ansible.module_utils.six import * + if isinstance(variable, string_types): + do_something(variable) + + from ansible.module_utils.basic import * + module = AnsibleModule([...]) + +Examples of fixed code: + +.. code-block:: python + + from ansible.module_utils import six + if isinstance(variable, six.string_types): + do_something(variable) + + from ansible.module_utils.basic import AnsibleModule + module = AnsibleModule([...]) diff --git a/test/sanity/code-smell/boilerplate.sh b/test/sanity/code-smell/boilerplate.sh index dd3c3462c32..b73ae71a708 100755 --- a/test/sanity/code-smell/boilerplate.sh +++ b/test/sanity/code-smell/boilerplate.sh @@ -3,6 +3,7 @@ metaclass1=$(find ./bin -type f -exec grep -HL '__metaclass__ = type' '{}' '+') future1=$(find ./bin -type f -exec grep -HL 'from __future__ import (absolute_import, division, print_function)' '{}' '+') +# We eventually want to remove the module_utils and modules pruning from metaclass2 and future2 metaclass2=$(find ./lib/ansible -path ./lib/ansible/modules -prune \ -o -path ./lib/ansible/module_utils -prune \ -o -path ./lib/ansible/module_utils/six/_six.py -prune \ @@ -60,33 +61,10 @@ future3=$(find ./lib/ansible/modules -path ./lib/ansible/modules/windows -prune -o -path ./lib/ansible/modules/network/vyos -prune \ -o -name '*.py' -type f -size +0c -exec egrep -HL 'from __future__ import (?absolute_import, division, print_function)?' '{}' '+') -# Ordered by approximate work, lowest to highest -# Key: -# [*]: import * fixes -# [!]: many get_exception fixes -# [i]: a few get_exception fixes -# (everything below needs boilerplate added) -# Priorities: import*, get_exception, then boilerplate-only -# -# network/ios -# network/eos [i] -# network/netvisor -# network/aos [!] -# network/vyos [i] -# network/lenovo -# network/panos [!] -# network/junos [i] -# files [!] -# network/avi -# network/f5 [*][i] -# monitoring [*][!] -# packaging/os [*][i] -# cloud/cloudstack [*] -# cloud/openstack [*] -# cloud/ovirt -# network/cloudengine [i] -# network/nxos [*][i] -# cloud/amazon [*] +### +### Important: If you're looking for a list of files to cleanup for boilerplate +### Look at this wikipage instead: https://github.com/ansible/community/wiki/Testing:-boilerplate,-wildcard-imports,-and-get_exception +### ### TODO: ### - module_utils <=== these are important but not well organized so we'd diff --git a/test/sanity/code-smell/no-get-exception.sh b/test/sanity/code-smell/no-get-exception.sh new file mode 100755 index 00000000000..6283dea3cfb --- /dev/null +++ b/test/sanity/code-smell/no-get-exception.sh @@ -0,0 +1,36 @@ +#!/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 this list to be empty +get_exception=$(find . -path ./test/runner/.tox -prune \ + -o -path ./lib/ansible/module_utils -prune \ + -o -path ./lib/ansible/modules/storage/netapp -prune \ + -o -path ./lib/ansible/modules/packaging/os -prune \ + -o -path ./lib/ansible/modules/monitoring -prune \ + -o -path ./lib/ansible/modules/network/panos -prune \ + -o -path ./lib/ansible/modules/network/nxos -prune \ + -o -path ./lib/ansible/modules/network/junos -prune \ + -o -path ./lib/ansible/modules/network/vyos -prune \ + -o -path ./lib/ansible/modules/network/fortios -prune \ + -o -path ./lib/ansible/modules/network/f5 -prune \ + -o -path ./lib/ansible/modules/network/cloudengine -prune \ + -o -path ./lib/ansible/modules/network/aos -prune \ + -o -path ./lib/ansible/modules/network/eos -prune \ + -o -path ./lib/ansible/modules/files -prune \ + -o -path ./lib/ansible/modules/system -prune \ + -o -path ./lib/ansible/modules/web_infrastructure -prune \ + -o -path ./lib/ansible/plugins/action/wait_for_connection.py -prune \ + -o -name '*.py' -type f -exec grep -H 'get_exception' '{}' '+') + + +if test -n "$get_exception" ; then + printf "\n== Contains get_exception() calls ==\n" + printf "%s" "$get_exception" + failures=$(printf "%s" "$get_exception"| wc -l) + failures=$((failures + 2)) + exit "$failures" +fi + +exit 0 diff --git a/test/sanity/code-smell/no-list-cmp.sh b/test/sanity/code-smell/no-list-cmp.sh deleted file mode 100755 index c71c693d62d..00000000000 --- a/test/sanity/code-smell/no-list-cmp.sh +++ /dev/null @@ -1,18 +0,0 @@ -#!/bin/sh - -CMP_USERS=$(grep -rI ' cmp[^a-zA-Z0-9_:=]' . \ - --exclude-dir .tox \ - | grep -v \ - -e lib/ansible/module_utils/six/_six.py \ - -e .git \ - -e docs/docsite/_build/ \ - -e docs/docsite/rst/dev_guide/testing/sanity/ \ - -e test/sanity/code-smell/no-list-cmp.sh - ) - -if [ "${CMP_USERS}" ]; then - echo 'cmp has been removed in python3. Alternatives:' - echo ' http://python3porting.com/preparing.html#when-sorting-use-key-instead-of-cmp' - echo "${CMP_USERS}" - exit 1 -fi diff --git a/test/sanity/code-smell/no-wildcard-import.sh b/test/sanity/code-smell/no-wildcard-import.sh new file mode 100755 index 00000000000..fc5d8daac25 --- /dev/null +++ b/test/sanity/code-smell/no-wildcard-import.sh @@ -0,0 +1,42 @@ +#!/bin/sh + +# Only needed until we enable pylint test for wildcard imports + + +# The first three paths here are valid uses of wildcard imports +# unsafe_proxy is backwards compat (pylint disabled added) +# module_common.py is picked up from static strings, not from actual imports (pylint won't detect) +# test_action.py is picked up from static strings, not from actual imports (pylint won't detect) +# mock.py is importing code for an installed library for compat (pylint disabled added) +# unittest.py is importing code for an installed library for compat (pylint disabled added) +# +# Everything else needs to be fixed +wildcard_imports=$(find . -path ./test/runner/.tox -prune \ + -o -path ./lib/ansible/vars/unsafe_proxy.py -prune \ + -o -path ./lib/ansible/executor/module_common.py -prune \ + -o -path ./test/units/plugins/action/test_action.py \ + -o -path ./lib/ansible/compat/tests/mock.py -prune \ + -o -path ./lib/ansible/compat/tests/unittest.py \ + -o -path ./lib/ansible/module_utils/api.py -prune \ + -o -path ./lib/ansible/module_utils/cloud.py -prune \ + -o -path ./lib/ansible/module_utils/aos.py -prune \ + -o -path ./test/units/modules/network/cumulus/test_nclu.py -prune \ + -o -path ./lib/ansible/modules/cloud/amazon -prune \ + -o -path ./lib/ansible/modules/cloud/openstack -prune \ + -o -path ./lib/ansible/modules/cloud/cloudstack -prune \ + -o -path ./lib/ansible/modules/monitoring -prune \ + -o -path ./lib/ansible/modules/network/f5 -prune \ + -o -path ./lib/ansible/modules/network/nxos -prune \ + -o -path ./lib/ansible/modules/packaging/os -prune \ + -o -name '*.py' -type f -exec grep -H 'import \*' '{}' '+') + + +if test -n "$wildcard_imports" ; then + printf "\n== Wildcard imports detected ==\n" + printf "%s" "$wildcard_imports" + failures=$(printf "%s" "$wildcard_imports"| wc -l) + failures=$((failures + 2)) + exit "$failures" +fi + +exit 0