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

Add unit tests for unsafe_proxy #47887

Merged
merged 4 commits into from
Nov 1, 2018
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 77 additions & 0 deletions test/units/utils/test_unsafe_proxy.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# -*- coding: utf-8 -*-
# (c) 2018 Matt Martz <matt@sivel.net>
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

from __future__ import absolute_import, division, print_function
__metaclass__ = type

from ansible.module_utils.six import PY3
from ansible.utils.unsafe_proxy import AnsibleUnsafe, AnsibleUnsafeText, UnsafeProxy, wrap_var


def test_UnsafeProxy():
assert isinstance(UnsafeProxy({}), dict)
assert not isinstance(UnsafeProxy({}), AnsibleUnsafe)

This comment was marked as resolved.


assert isinstance(UnsafeProxy('foo'), AnsibleUnsafeText)


def test_wrap_var_string():
assert isinstance(wrap_var('foo'), AnsibleUnsafeText)
assert isinstance(wrap_var(u'foo'), AnsibleUnsafeText)
if PY3:
assert isinstance(wrap_var(b'foo'), type(b''))
assert not isinstance(wrap_var(b'foo'), AnsibleUnsafe)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert not isinstance(wrap_var(b'foo'), AnsibleUnsafe)
assert isinstance(wrap_var(b'foo'), type(b''))

Copy link
Member Author

Choose a reason for hiding this comment

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

I used the not isinstance syntax, so that if someone were to implement something like:

class AnsibleUnsafeBytes(binary_type, AnsibleUnsafe):
    pass

The tests would have to be knowingly updated, because they would fail.

The change you recommend, would silently pass, due to making an assumption about the data.

The current method would fail, as it causes a change in behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense. It would still be good to add the assertions while keeping the existing ones. In line with that, should most of the isinstance assertions which aren't negated check for AnsibleUnsafeText instead of just AnsibleUnsafe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Feedback should be addressed.

else:
assert isinstance(wrap_var(b'foo'), AnsibleUnsafeText)


def test_wrap_var_dict():
assert isinstance(wrap_var(dict(foo='bar')), dict)
assert not isinstance(wrap_var(dict(foo='bar')), AnsibleUnsafe)

This comment was marked as resolved.

assert isinstance(wrap_var(dict(foo='bar'))['foo'], AnsibleUnsafeText)


def test_wrap_var_dict_None():
assert wrap_var(dict(foo=None))['foo'] is None
assert not isinstance(wrap_var(dict(foo=None))['foo'], AnsibleUnsafe)

This comment was marked as resolved.



def test_wrap_var_list():
assert isinstance(wrap_var(['foo']), list)
assert not isinstance(wrap_var(['foo']), AnsibleUnsafe)

This comment was marked as resolved.

assert isinstance(wrap_var(['foo'])[0], AnsibleUnsafeText)


def test_wrap_var_list_None():
assert wrap_var([None])[0] is None
assert not isinstance(wrap_var([None])[0], AnsibleUnsafe)

This comment was marked as resolved.



def test_wrap_var_set():
assert isinstance(wrap_var(set(['foo'])), set)
assert not isinstance(wrap_var(set(['foo'])), AnsibleUnsafe)

This comment was marked as resolved.

for item in wrap_var(set(['foo'])):
assert isinstance(item, AnsibleUnsafeText)


def test_wrap_var_set_None():
for item in wrap_var(set([None])):
assert item is None
assert not isinstance(item, AnsibleUnsafe)

This comment was marked as resolved.



def test_wrap_var_tuple():
assert isinstance(wrap_var(('foo',)), tuple)
assert not isinstance(wrap_var(('foo',)), AnsibleUnsafe)

This comment was marked as resolved.

assert isinstance(wrap_var(('foo',))[0], type(''))
assert not isinstance(wrap_var(('foo',))[0], AnsibleUnsafe)

This comment was marked as resolved.



def test_wrap_var_None():
assert wrap_var(None) is None
assert not isinstance(wrap_var(None), AnsibleUnsafe)

This comment was marked as resolved.



def test_Wrap_var_unsafe():
assert isinstance(wrap_var(AnsibleUnsafeText(u'foo')), AnsibleUnsafeText)