Add unit tests for unsafe_proxy #47887

sivel · 2018-10-31T17:39:34Z

SUMMARY

Add unit tests for unsafe_proxy

Name                                Stmts   Miss Branch BrPart  Cover   Missing
-------------------------------------------------------------------------------
lib/ansible/utils/unsafe_proxy.py      38      0     20      0   100%

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

test/units/utils/test_unsafe_proxy.py

ANSIBLE VERSION

2.8

ADDITIONAL INFORMATION

ansibot · 2018-10-31T17:44:53Z

Hi @sivel, thank you for submitting this pull-request!

click here for bot help

mattclay

I recommend changing all is not assertions to assert what is expected. I've used the new Suggested Changes feature to mark all of those, along with removing an unused import.

I also recommend changing all assertions of AnsibleUnsafe to the more specific AnsibleUnsafeText, except for the last one in test_Wrap_var_unsafe. I didn't call those out individually though.

mattclay · 2018-10-31T18:09:27Z

test/units/utils/test_unsafe_proxy.py

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


Suggested change

import pytest

mattclay · 2018-10-31T18:31:23Z

test/units/utils/test_unsafe_proxy.py

+    assert isinstance(wrap_var('foo'), AnsibleUnsafe)
+    assert isinstance(wrap_var(u'foo'), AnsibleUnsafe)
+    if PY3:
+        assert not isinstance(wrap_var(b'foo'), AnsibleUnsafe)


Suggested change

assert not isinstance(wrap_var(b'foo'), AnsibleUnsafe)

assert isinstance(wrap_var(b'foo'), type(b''))

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.

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?

Feedback should be addressed.

test/units/utils/test_unsafe_proxy.py

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


test/units/utils/test_unsafe_proxy.py

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


test/units/utils/test_unsafe_proxy.py

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


test/units/utils/test_unsafe_proxy.py

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


test/units/utils/test_unsafe_proxy.py

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


test/units/utils/test_unsafe_proxy.py

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


test/units/utils/test_unsafe_proxy.py

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


test/units/utils/test_unsafe_proxy.py

+
+def test_wrap_var_tuple():
+    assert not isinstance(wrap_var(('foo',)), AnsibleUnsafe)
+    assert not isinstance(wrap_var(('foo',))[0], AnsibleUnsafe)


mattclay

Looks good, although I wonder if there should be a test to assert that AnsibleUnsafeText inherits AnsibleUnsafe?

* Add unit tests for unsafe_proxy * Remove unused import * Address comments * Add inheritance test

Add unit tests for unsafe_proxy

9aaa9f1

sivel requested a review from mattclay October 31, 2018 17:39

mattclay requested changes Oct 31, 2018

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Oct 31, 2018

sivel added 2 commits October 31, 2018 14:23

Remove unused import

5ef6ca8

Address comments

91a8988

mattclay approved these changes Oct 31, 2018

View reviewed changes

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 31, 2018

jborean93 removed the needs_triage Needs a first human triage before being processed. label Nov 1, 2018

Add inheritance test

d5baa90

sivel merged commit 042a0cf into ansible:devel Nov 1, 2018

Tomorrow9 pushed a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018

Add unit tests for unsafe_proxy (ansible#47887)

ae6d4d3

* Add unit tests for unsafe_proxy * Remove unused import * Address comments * Add inheritance test

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for unsafe_proxy #47887

Add unit tests for unsafe_proxy #47887

sivel commented Oct 31, 2018

ansibot commented Oct 31, 2018

mattclay left a comment

mattclay Oct 31, 2018

mattclay Oct 31, 2018

sivel Oct 31, 2018

mattclay Oct 31, 2018

sivel Oct 31, 2018

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mattclay left a comment

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



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



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



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



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



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



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

Add unit tests for unsafe_proxy #47887

Add unit tests for unsafe_proxy #47887

Conversation

sivel commented Oct 31, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Oct 31, 2018

mattclay left a comment

Choose a reason for hiding this comment

mattclay Oct 31, 2018

Choose a reason for hiding this comment

mattclay Oct 31, 2018

Choose a reason for hiding this comment

sivel Oct 31, 2018

Choose a reason for hiding this comment

mattclay Oct 31, 2018

Choose a reason for hiding this comment

sivel Oct 31, 2018

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mattclay left a comment

Choose a reason for hiding this comment