From 57575d1cfa4cbde13cf2ace688365ad602050044 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Thu, 21 Dec 2017 13:42:53 -0600 Subject: [PATCH] Fix tests as filters #4 (#33930) * Resolve newly added tests as filters * Add code smell to test for ansible provided jinja tests as filters syntax * Add docs for no-tests-as-filters code smell test * Address tests as filters in new integration tests * Address feedback * Address feedback 2 --- .../testing/sanity/no-tests-as-filters.rst | 10 +++ .../tests/common/sanity.yaml | 2 +- .../targets/vmware_host/tasks/main.yml | 12 +-- .../targets/win_disk_facts/tasks/main.yml | 2 +- .../tasks/floating_ip.yml | 26 +++--- test/sanity/code-smell/no-tests-as-filters.py | 90 +++++++++++++++++++ 6 files changed, 121 insertions(+), 21 deletions(-) create mode 100644 docs/docsite/rst/dev_guide/testing/sanity/no-tests-as-filters.rst create mode 100644 test/sanity/code-smell/no-tests-as-filters.py diff --git a/docs/docsite/rst/dev_guide/testing/sanity/no-tests-as-filters.rst b/docs/docsite/rst/dev_guide/testing/sanity/no-tests-as-filters.rst new file mode 100644 index 00000000000..a61c0fac02f --- /dev/null +++ b/docs/docsite/rst/dev_guide/testing/sanity/no-tests-as-filters.rst @@ -0,0 +1,10 @@ +Sanity Tests ยป no-tests-as-filters +================================== + +Using Ansible provided Jinja2 tests as filters will be removed in Ansible 2.9. + +Prior to Ansible 2.5, Jinja2 tests included within Ansible were most often used as filters. The large difference in use is that filters are referenced as ``variable | filter_name`` where as Jinja2 tests are refereced as ``variable is test_name``. + +Jinja2 tests are used for comparisons, whereas filters are used for data manipulation, and have different applications in Jinja2. This change is to help differentiate the concepts for a better understanding of Jinja2, and where each can be appropriately used. + +As of Ansible 2.5 using an Ansible provided Jinja2 test with filter syntax will display a deprecation error. diff --git a/test/integration/targets/nxos_gir_profile_management/tests/common/sanity.yaml b/test/integration/targets/nxos_gir_profile_management/tests/common/sanity.yaml index 7f322107f7d..fd13578d668 100644 --- a/test/integration/targets/nxos_gir_profile_management/tests/common/sanity.yaml +++ b/test/integration/targets/nxos_gir_profile_management/tests/common/sanity.yaml @@ -87,7 +87,7 @@ - assert: *false - when: not (platform | match('N35')) and not titanium + when: not ( platform is match('N35')) and not titanium rescue: diff --git a/test/integration/targets/vmware_host/tasks/main.yml b/test/integration/targets/vmware_host/tasks/main.yml index 714f6deb805..89473159733 100644 --- a/test/integration/targets/vmware_host/tasks/main.yml +++ b/test/integration/targets/vmware_host/tasks/main.yml @@ -83,7 +83,7 @@ - name: ensure host system is present assert: that: - - add_host_result | changed + - add_host_result is changed - "{% for host in host_list.json if ((host | basename) == 'test_host_system_0001') -%} True {%- else -%} False {%- endfor %}" # Testcase: Add Host again @@ -104,7 +104,7 @@ - name: ensure precend task didn't changed anything assert: that: - - not (readd_host_result|changed) + - not ( readd_host_result is changed) # Testcase: Add Host via add_or_reconnect state - name: add host via add_or_reconnect @@ -129,7 +129,7 @@ - name: ensure host system is present assert: that: - - add_or_reconnect_host_result | changed + - add_or_reconnect_host_result is changed - "{% for host in host_list.json if ((host | basename) == 'test_host_system_0002') -%} True {%- else -%} False {%- endfor %}" ## Testcase: Reconnect Host @@ -152,7 +152,7 @@ #- name: ensure host system has been reconnected # assert: # that: -# - reconnect_host_result | changed +# - reconnect_host_result is changed # # it would be a good idea to check the events on the host to see the reconnect # # https://github.com/vmware/govmomi/blob/master/govc/USAGE.md#events # # "govc events ..." need to be callable from @@ -183,7 +183,7 @@ #- name: ensure host system is absent # assert: # that: -# - remove_host_result | changed +# - remove_host_result is changed # - "{% for host in host_list.json if ((host | basename) == 'test_host_system_0001') -%} False {%- else -%} True {%- endfor %}" ## Testcase: Remove Host again @@ -206,4 +206,4 @@ #- name: ensure precend task didn't changed anything # assert: # that: -# - not (reremove_host_result|changed) +# - not ( reremove_host_result is changed) diff --git a/test/integration/targets/win_disk_facts/tasks/main.yml b/test/integration/targets/win_disk_facts/tasks/main.yml index 1033aea916c..70a96a88f7d 100644 --- a/test/integration/targets/win_disk_facts/tasks/main.yml +++ b/test/integration/targets/win_disk_facts/tasks/main.yml @@ -6,7 +6,7 @@ ignore_errors: true - name: Only run tests when Windows is capable - when: (win_feature_has_storage_module|success) and (ansible_powershell_version is defined) and (ansible_powershell_version >= 3) + when: ( win_feature_has_storage_module is successful) and (ansible_powershell_version is defined) and (ansible_powershell_version >= 3) block: - name: Test in normal mode diff --git a/test/legacy/roles/cloudscale_floating_ip/tasks/floating_ip.yml b/test/legacy/roles/cloudscale_floating_ip/tasks/floating_ip.yml index 64872d5148c..61a7362a009 100644 --- a/test/legacy/roles/cloudscale_floating_ip/tasks/floating_ip.yml +++ b/test/legacy/roles/cloudscale_floating_ip/tasks/floating_ip.yml @@ -8,8 +8,8 @@ - name: Verify request floating IP assert: that: - - floating_ip | success - - floating_ip | changed + - floating_ip is successful + - floating_ip is changed - (item.ip_version == 4 and floating_ip.ip | ipv4) or (item.ip_version == 6 and floating_ip.ip | ipv6) - floating_ip.server == test01.uuid @@ -21,8 +21,8 @@ - name: Verify floating IP indempotence assert: that: - - floating_ip_indempotence | success - - not floating_ip_indempotence | changed + - floating_ip_indempotence is successful + - floating_ip_indempotence is not changed - floating_ip_indempotence.server == test01.uuid - name: Check network parameter alias @@ -33,7 +33,7 @@ - name: Verify network parameter alias assert: that: - - floating_ip_network | success + - floating_ip_network is successful - name: Move floating IP to second server cloudscale_floating_ip: @@ -43,8 +43,8 @@ - name: Verify move floating IPv4 to second server assert: that: - - move_ip | success - - move_ip | changed + - move_ip is successful + - move_ip is changed - move_ip.server == test02.uuid - name: Fail if server is missing on update @@ -55,7 +55,7 @@ - name: Verify fail if server is missing on update assert: that: - - update_failed | failed + - update_failed is failed - "'Missing required parameter' in update_failed.msg" - name: Release floating IP @@ -66,8 +66,8 @@ - name: Verify release floating IPs assert: that: - - release_ip | success - - release_ip | changed + - release_ip is successful + - release_ip is changed - release_ip.state == 'absent' - name: Release floating IP indempotence @@ -78,8 +78,8 @@ - name: Verify release floating IPs indempotence assert: that: - - release_ip | success - - not release_ip | changed + - release_ip is successful + - release_ip is not changed - release_ip.state == 'absent' - name: Fail if server is missing on request @@ -90,5 +90,5 @@ - name: Verify fail if server is missing on request assert: that: - - request_failed | failed + - request_failed is failed - "'Missing required parameter' in request_failed.msg" diff --git a/test/sanity/code-smell/no-tests-as-filters.py b/test/sanity/code-smell/no-tests-as-filters.py new file mode 100644 index 00000000000..101beee384f --- /dev/null +++ b/test/sanity/code-smell/no-tests-as-filters.py @@ -0,0 +1,90 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- +# (c) 2017, Matt Martz +# +# This file is part of Ansible +# +# Ansible is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# Ansible is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Ansible. If not, see . + +from __future__ import print_function + +import os +import re +import sys + +from collections import defaultdict + +from ansible.plugins.test import core, files, mathstuff + +TESTS = list(core.TestModule().tests().keys()) + list(files.TestModule().tests().keys()) + list(mathstuff.TestModule().tests().keys()) + + +TEST_MAP = { + 'version_compare': 'version', + 'is_dir': 'directory', + 'is_file': 'file', + 'is_link': 'link', + 'is_abs': 'abs', + 'is_same_file': 'same_file', + 'is_mount': 'mount', + 'issubset': 'subset', + 'issuperset': 'superset', + 'isnan': 'nan', + 'succeeded': 'successful', + 'success': 'successful', + 'change': 'changed', + 'skip': 'skipped', +} + + +FILTER_RE = re.compile(r'((.+?)\s*([\w \.\'"]+)(\s*)\|(\s*)(\w+))') + + +def main(): + all_matches = defaultdict(list) + + for root, dirs, filenames in os.walk('.'): + for name in filenames: + if os.path.splitext(name)[1] not in ('.yml', '.yaml'): + continue + path = os.path.join(root, name) + + with open(path) as f: + text = f.read() + + for match in FILTER_RE.findall(text): + filter_name = match[5] + + try: + test_name = TEST_MAP[filter_name] + except KeyError: + test_name = filter_name + + if test_name not in TESTS: + continue + + all_matches[path].append(match[0]) + + if all_matches: + print('Use of Ansible provided Jinja2 tests as filters is deprecated.') + print('Please update to use `is` syntax such as `result is failed`.') + + for path, matches in all_matches.items(): + for match in matches: + print('%s: %s' % (path, match,)) + sys.exit(1) + + +if __name__ == '__main__': + main()