Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests as filters #4 #33930

Merged
merged 6 commits into from
Dec 21, 2017
Merged

Fix tests as filters #4 #33930

merged 6 commits into from
Dec 21, 2017

Conversation

sivel
Copy link
Member

@sivel sivel commented Dec 14, 2017

SUMMARY

Address recent changes to tests to fix jinja tests as filters

Following on #32361 this PR fixes tests as filters syntax to use proper is test syntax in newly modified files.

Additionally, the PR adds a code smell test to check for tests as filters syntax.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

tests

ANSIBLE VERSION
devel
ADDITIONAL INFORMATION

@sivel sivel requested a review from mattclay December 14, 2017 22:43
@sivel
Copy link
Member Author

sivel commented Dec 14, 2017

I forgot, I'll need to add docs for the code smell test

@ansibot
Copy link
Contributor

ansibot commented Dec 14, 2017

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request cloud needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. vmware VMware community labels Dec 14, 2017
@Akasurde Akasurde removed the needs_triage Needs a first human triage before being processed. label Dec 15, 2017
@ansibot ansibot added networking Network category nxos Cisco NXOS community support:network This issue/PR relates to code supported by the Ansible Network Team. windows Windows community labels Dec 20, 2017
@tchernomax
Copy link
Contributor

shipit


Jinja tests are used for comparisons, whereas filters are used for data manipulation, and have different applications in jinja. This change is to help differentiate the concepts for a better understanding of jinja, and where each can be appropriately used.

As of Ansible 2.5 using an Ansible provided jinja test with filter syntax, will display a deprecation error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary comma.

Sanity Tests » no-tests-as-filters
==================================

Using Ansible provided jinja tests as filters will be removed in Ansible 2.9.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Jinja2 instead of jinja here and in following sentences.


FILTER_RE = re.compile(r'((.+?)\s*([\w \.\'"]+)(\s*)\|(\s*)(\w+))')

all_matches = defaultdict(list)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the test body in a main function called with the standard if __name__ == '__main__' conditional.

It's not really an issue now, but if we start running import tests on code smell tests we'll want that.


if all_matches:
print('Use of Ansible provided Jinja tests as filters is deprecated.')
print('Please update to use `is` syntax such as `result is failed`')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should probably be at the end of the sentence.

all_matches[path].append(match[0])

if all_matches:
print('Use of Ansible provided Jinja tests as filters is deprecated.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Jinja2 instead of Jinja.

@sivel
Copy link
Member Author

sivel commented Dec 21, 2017

@mattclay feedback addressed


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``.

Jinja 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more Jinja to Jinja2 fix here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah. Thanks.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 21, 2017
@sivel
Copy link
Member Author

sivel commented Dec 21, 2017

@mattclay addressed

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 21, 2017
@sivel sivel merged commit 57575d1 into ansible:devel Dec 21, 2017
@ansibot ansibot added docs This issue/PR relates to or includes documentation. bug This issue/PR relates to a bug. and removed docs_pull_request labels Mar 4, 2018
@dagwieers dagwieers added the cisco Cisco technologies label Feb 23, 2019
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. cisco Cisco technologies cloud docs This issue/PR relates to or includes documentation. networking Network category nxos Cisco NXOS community support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. support:network This issue/PR relates to code supported by the Ansible Network Team. test This PR relates to tests. vmware VMware community windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants