Merge lp:~mandel/ubuntuone-client/add-method-name into lp:ubuntuone-client

[Manuel de la Peña]

Add extra logging information for the assert messages when used in a decorator.

Add extra logging information for the assert messages when used in a decorator.

[Natalia Bidart (nataliabidart)]

* Can you please remove all the trailing spaces you added to the blank lines?

* Can you please restore all the ending dots to the messages?

review: Needs Fixing

[lp:~mandel/ubuntuone-client/add-method-name]

1119. By Manuel de la Peña on 2011-08-31

Fixed spaces.

[Natalia Bidart (nataliabidart)]

The ending dots should not be added in _add_method_info but in the strings itself. The code style recommends adding an ending dot to every comment/message/log message, so usually a dev will write that in the same string, and we'll end up with 2 dots (is a bit unexpected to have methods adding ending dots).

review: Needs Fixing

[lp:~mandel/ubuntuone-client/add-method-name]

1120. By Manuel de la Peña on 2011-09-01

Re-added the dots and changed the sed message.

[Diego Sarmentero (diegosarmentero)]

Instead of:

if method_name is not None:

Can not you just say?:

if not method_name:

(so you well check for empty strings too, maybe i'm wrong, but i think that is better)

TESTS??

review: Needs Information

[lp:~mandel/ubuntuone-client/add-method-name]

1121. By Manuel de la Peña on 2011-09-01

Change the way in which the if statement works so that we do not accept ''

1122. By Manuel de la Peña on 2011-09-01

Added tests.

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[Natalia Bidart (nataliabidart)]

* You're adding a lot of duplication for the tests. What about creating a new assert method and reuse that in every test?

def assert_error_raised(self, path, method_name=None):

     if method_name is None:
         self.assertRaises(AssertionError, assert_windows_path, path)
     else:
         exc = self.assertRaises(AssertionError, assert_windows_path, path,
                                 method_name)
         self.assertTrue(method_name in exc.message)

* There are lint issues:

== Python Lint Notices ==

./tests/platform/windows/test_os_helper.py:
    321: redefinition of function 'test_assert_windows_path_with_illegal_chars' from line 316

review: Needs Fixing

[lp:~mandel/ubuntuone-client/add-method-name]

1123. By Manuel de la Peña on 2011-09-02

Simplified tests code and fixed a redefinition of a fucntion.

1124. By Manuel de la Peña on 2011-09-02

Fixed stupid brain fucks.

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

=== modified file 'tests/platform/windows/test_os_helper.py'
--- tests/platform/windows/test_os_helper.py	2011-08-15 20:29:25 +0000
+++ tests/platform/windows/test_os_helper.py	2011-09-02 11:37:28 +0000
@@ -261,30 +261,68 @@
 class DecoratorsTestCase(TestCase):
     """Test case for all the validators and transformers."""
 
+    def assert_error_raised(self, path, method_name=None):
+        if method_name is None:
+            self.assertRaises(AssertionError, assert_windows_path, path)
+        else:
+            exc = self.assertRaises(AssertionError, assert_windows_path, path,
+                                     method_name)
+            self.assertTrue(method_name in exc.message)
+
     def test_assert_windows_path_slash(self):
         """A path with a / is invalid."""
         path = LONG_PATH_PREFIX + u'/a/b/'
-        self.assertRaises(AssertionError, assert_windows_path, path)
+        self.assert_error_raised(path)
+
+    def test_assert_windows_method_name_path_slash(self):
+        """A path with a / is invalid."""
+        path = LONG_PATH_PREFIX + u'/a/b/'
+        method_name = 'method_name'
+        self.assert_error_raised(path, method_name)
 
     def test_assert_windows_path_non_unicode(self):
         """A non-unicode path is invalid."""
         path = (LONG_PATH_PREFIX + u'C:\\Yadda').encode('utf8')
-        self.assertRaises(AssertionError, assert_windows_path, path)
+        self.assert_error_raised(path)
+
+    def test_assert_windows_method_name_path_non_unicode(self):
+        """A non-unicode path is invalid."""
+        path = (LONG_PATH_PREFIX + u'C:\\Yadda').encode('utf8')
+        method_name = 'method_name'
+        self.assert_error_raised(path, method_name)
 
     def test_assert_windows_path_non_literal(self):
         """A non-literal path is invalid."""
         path = u'C:\\Yadda'
-        self.assertRaises(AssertionError, assert_windows_path, path)
+        self.assert_error_raised(path)
+
+    def test_assert_windows_method_name_path_non_literal(self):
+        """A non-literal path is invalid."""
+        path = u'C:\\Yadda'
+        method_name = 'method_name'
+        self.assert_error_raised(path, method_name)
 
     def test_assert_windows_path_non_absolute(self):
         """A non-absolute path is invalid."""
         path = u'./yadda'
-        self.assertRaises(AssertionError, assert_windows_path, path)
+        self.assert_error_raised(path)
+
+    def test_assert_windows_method_name_path_non_absolute(self):
+        """A non-absolute path is invalid."""
+        path = u'./yadda'
+        method_name = 'method_name'
+        self.assert_error_raised(path, method_name)
 
     def test_assert_windows_path_with_illegal_chars(self):
         """A path with illegal chars is invalid."""
         path = u'./yadda' + u''.join(WINDOWS_ILLEGAL_CHARS_MAP)
-        self.assertRaises(AssertionError, assert_windows_path, path)
+        self.assert_error_raised(path)
+
+    def test_assert_windows_method_name_path_with_illegal_chars(self):
+        """A path with illegal chars is invalid."""
+        path = u'./yadda' + u''.join(WINDOWS_ILLEGAL_CHARS_MAP)
+        method_name = 'method_name'
+        self.assert_error_raised(path, method_name)
 
 
 class TestIllegalPathsWalk(WalkTests):

=== modified file 'ubuntuone/platform/windows/os_helper.py'
--- ubuntuone/platform/windows/os_helper.py	2011-08-15 18:54:45 +0000
+++ ubuntuone/platform/windows/os_helper.py	2011-09-02 11:37:28 +0000
@@ -100,8 +100,18 @@
 
 # Functions to be used for path validation
 
-
-def assert_windows_path(path):
+def _add_method_info(messages, method_name=None):
+    """Loop through the messages and add the extra info."""
+    updated_messages = messages.copy()
+    if method_name:
+        # lets update the messages to contain the method info.
+        extra_info = 'Asserted in method "%s".' % method_name
+        for message_name in messages:
+            updated_messages[message_name] += extra_info
+    return updated_messages
+
+
+def assert_windows_path(path, method_name=None):
     """Check whether 'path' is a valid windows path.
 
     A 'valid windows path' should meet the following requirements:
@@ -111,19 +121,28 @@
     * is a literal path (it starts with the LONG_PATH_PREFIX prefix)
     * do not contain any invalid character (see WINDOWS_ILLEGAL_CHARS_MAP)
 
+    Opcionally the name of the method that called the assertion can be passed
+    to improve the assertion message.
     """
-    assert isinstance(path, unicode), 'Path %r should be unicode.' % path
-    assert path.startswith(LONG_PATH_PREFIX), \
-           'Path %r should start with the LONG_PATH_PREFIX.' % path
+    messages = {
+        'unicode_path': 'Path %r should be unicode.',
+        'long_path': 'Path %r should start with the LONG_PATH_PREFIX.',
+        'illegal_path': '%r should not contain any character from' +
+                         ' WINDOWS_ILLEGAL_CHARS_MAP.',
+    }
+    messages = _add_method_info(messages, method_name)
+
+    assert isinstance(path, unicode), messages['unicode_path'] % path
+    assert path.startswith(LONG_PATH_PREFIX), messages['long_path'] % path
     assert os.path.isabs(path.replace(LONG_PATH_PREFIX, u''))
 
     path = path.replace(LONG_PATH_PREFIX, u'')
     drive, path = os.path.splitdrive(path)
     assert not any(c in WINDOWS_ILLEGAL_CHARS_MAP for c in path), \
-        '%r should not contain any character from WINDOWS_ILLEGAL_CHARS_MAP.' % path
-
-
-def assert_syncdaemon_path(path):
+                   messages['illegal_path'] % path
+
+
+def assert_syncdaemon_path(path, method_name=None):
     """Check whether 'path' is a valid syncdaemon path.
 
     A 'valid syncdaemon path' should meet the following requirements:
@@ -133,17 +152,23 @@
     * do not contain the LONG_PATH_PREFIX
 
     """
-    assert isinstance(path, str), 'Path %r should be a bytes sequence.' % path
+    messages = {
+        'byte_path': 'Path %r should be a bytes sequence.',
+        'utf8_path': 'Path %r should be encoded with utf8.',
+        'long_path': '%r should not start with the LONG_PATH_PREFIX.',
+        'unicode_chars': '%r should not contain any character from LINUX_ILLEGAL_CHARS_MAP.',
+    }
+    messages = _add_method_info(messages, method_name)
+
+    assert isinstance(path, str), messages['byte_path'] % path
     try:
         path = path.decode('utf8')
     except UnicodeDecodeError:
-        msg = 'Path %r should be encoded with utf8.'
-        raise AssertionError(msg % path)
+        raise AssertionError(messages['utf8_path'] % path)
     # path is now a unicode, we can compare against other unicodes
-    assert not path.startswith(LONG_PATH_PREFIX), \
-        '%r should not start with the LONG_PATH_PREFIX.'
+    assert not path.startswith(LONG_PATH_PREFIX), messages['long_path']
     assert not any(c in LINUX_ILLEGAL_CHARS_MAP for c in path), \
-        '%r should not contain any character from LINUX_ILLEGAL_CHARS_MAP.' % path
+        messages['unicode_chars'] % path
 
 
 # Functions to be used for path transformation
@@ -249,7 +274,7 @@
         def inner(*args, **kwargs):
             """Do the validation."""
             for i in path_indexes:
-                validate(args[i])
+                validate(args[i], method_name=function.__name__)
             return function(*args, **kwargs)
 
         return inner