Merge lp:~freyes/charm-helpers/fix-test-ipv6 into lp:charm-helpers

[Felipe Reyes]

Fix test_is_ipv6_disabled()

When calling subprocess.check_output() in is_ipv6_disabled() pass
universal_newlines=True so a string is returned in py2 and py3

The test that fails is:

======================================================================
ERROR: test_is_ipv6_disabled (tests.contrib.network.test_ip.IPTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/freyes/Projects/charms/devel/charm-helpers/fix-tests/.venv3/lib/python3.5/site-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/home/freyes/Projects/charms/devel/charm-helpers/fix-tests/tests/contrib/network/test_ip.py", line 404, in test_is_ipv6_disabled
    self.assertFalse(net_ip.is_ipv6_disabled())
  File "/home/freyes/Projects/charms/devel/charm-helpers/fix-tests/charmhelpers/contrib/network/ip.py", line 250, in is_ipv6_disabled
    result = result.decode('UTF-8')
AttributeError: 'str' object has no attribute 'decode'

[Alex Kavanagh (ajkavanagh)]

universal_newlines=True is to normalise newlines between mac, windows and *nix; we're only dealing with *nix machines here, and it indirectly papers over the cracks between Py2 and Py using a IOTextWrapper with default settings.

The 'problem' is that check_output() returns a bytestring in Py3 vs a str on Py2 which is what the decode is for, and the bug is in the test.

I'd prefer to change the test to pass a bytestring if it's Py3 as the code currently works OR (perhaps even better) just make Py2 a bit looser by instead doing:

   return b"net.ipv6.conf.all.disable_ipv6 = 1" in result

as Python2 will paper over the cracks by doing an implicit conversion: e.g. in ipython (py2):

In [4]: s = "hello there\n\n123 and som"

In [5]: b"123" in s
Out[5]: True

Obviously, the test would also need to pass a b"""...""" and b"" (the two mock outputs), and the .decode('UTF-8') would be removed in the original function.

This would work for both py2 and py3. Thoughts?

[Felipe Reyes (freyes)]

Alex, I'm OK with changing the patch to the approach you suggest, but universal_newlines=True is a widespread approach to deal with py2 vs py3, if you think that it's worth to take a different method to deal with this, I will do the change.

$ find charmhelpers/ -name "*.py" -exec grep universal_newlines {} \; | wc -l
13

[Alex Kavanagh (ajkavanagh)]

Felipe, that's interesting. I don't particularly want to create work, so on this basis, I'm happy to go with the universal_newlines as its prevalent in charmhelpers. I guess a one line comment to indicate why it is being used 'might' be useful for the future? Thanks for sorting this out. The 'bug' really shouldn't have got merged, but I won't go looking for the culprit :)

review: Approve

[Jorge Niedbalski (niedbalski)]

LGTM

review: Approve

=== modified file 'charmhelpers/contrib/network/ip.py'
--- charmhelpers/contrib/network/ip.py	2017-06-01 15:52:12 +0000
+++ charmhelpers/contrib/network/ip.py	2017-06-15 22:11:25 +0000
@@ -243,11 +243,11 @@
     try:
         result = subprocess.check_output(
             ['sysctl', 'net.ipv6.conf.all.disable_ipv6'],
-            stderr=subprocess.STDOUT)
+            stderr=subprocess.STDOUT,
+            universal_newlines=True)
     except subprocess.CalledProcessError:
         return True
-    if six.PY3:
-        result = result.decode('UTF-8')
+
     return "net.ipv6.conf.all.disable_ipv6 = 1" in result
 
 

=== modified file 'tests/contrib/network/test_ip.py'
--- tests/contrib/network/test_ip.py	2017-04-25 17:13:19 +0000
+++ tests/contrib/network/test_ip.py	2017-06-15 22:11:25 +0000
@@ -391,7 +391,7 @@
         self.assertTrue(net_ip.is_ipv6_disabled())
         mock_check_output.assert_called_once_with(
             ['sysctl', 'net.ipv6.conf.all.disable_ipv6'],
-            stderr=subprocess.STDOUT)
+            stderr=subprocess.STDOUT, universal_newlines=True)
         # if it isn't there, it must return false
         mock_check_output.return_value = ""
         self.assertFalse(net_ip.is_ipv6_disabled())