Code review comment for lp:~mandel/ubuntuone-client/fix-os-helper

> > > * If you plan to use _get_windows_valid_path from other modules, do not
> name
> > > it with an initial underscore. Things named _something are not meant to be
> > > used outside the module that defines them.
> >
> > I was hoping to be allowed to use _ so that it was explicit that you should
> > not be using it unless you really really know what you are doing, and I
> > seriously hope I know what I'm doing in this code ;)
>
> We are not allowed to use methods starting with _ outside a module, only
> exception is when writing tests about that module.
> If you need people to be careful using a method, add a huge docstring to it
> stating why is dangerous and what precautions we need to have when using it.

No worries, in this new version the name has changed.

>
> > > * If I understand this test correctly, it makes no sense:
> > >
> > > def test_restore_illegal_chars(self):
> > > """Test that the path does not have utf chars."""
> > > path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> > > path = restore_illegal_chars(path)
> > > self.assertFalse(any(c in path for c in LINUX_ILLEGAL_CHARS_MAP))
> >
> > What is wrong with the test? it is making sure that the method gets a path
> > with all the utf8 chars in it and returns a path with only illegal chars.
> The
> > assert ensures that once there are no utf8 chars in the returned path.
>
> What is wrong is that you're not testing the key part of restore_illegal_chars
> which is the following: not only some chars are removed, but also the proper
> replacements is being made, which is needed to be able to ensure that we can
> obtain back the original path. See below for a trivial implementation of
> restore_illegal_chars that will make this test pass and break all the code
> horribly.
>
> > > it should be something like (please note the docstring was corrected):
> > >
> > > def test_restore_illegal_chars(self):
> > > """The illegal unicode chars are replaced by characters, valid in
> > > linux."""
> > > original_path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> > > path = remove_illegal_chars(original_path)
> > > assert not any(c in path for c in LINUX_ILLEGAL_CHARS_MAP)
> > >
> > > path = restore_illegal_chars(path)
> > > self.assertEqual(original_path, path)
> > >
> >
> > I do see the confusion is possible because the name of the test is wrong, it
> > should be test_remove_illegla_chars, looks like an error when doing the
> > search/replace when renaming methods. On the other hand I do not like your
> > test because it is testing more than one thing, here you are taking a path
>
> Is not testing more that one thing. Please note that using the builtin
> "assert" is not testing something, but being explicit about an assumption that
> is being made while preparing the variables to perform the actual test, which
> is the call to assertEqual.
>
> > original. If this test fails we would need to look at two different methods,
>
> We will have either an Error or Failure, depending if the assert failed or the
> assertEqual, respectively.
> If we get an Error, we know some of our assumptions were wrong, and we need to
> either reconsider our test, or see what thing we trusted that is not behaving
> as expected.
>
> This is a common strategy when writing tests, you need to assume the rest of
> your code correct, and you can use that to build the unit test for a specific
> feature. So, in one test, we assume restore_foo correct in order to test
> remove_foo, and viceversa.
>
> > From my point of view tests should have a single concern, in this case would
>
> Agreed, see how each test has a single assertEqual.

Ok, if you looked at it from that point... but then I'd change the order, that is, assert that the paths are equals as in 'assert original == path' and use assertFalse(any(blah...) since what we want to test is the non-existence of the chars in that path rather than the fact that we can perform a round trip.

>
> > > and you need to add an analogous test for remove_illegal_chars:
> > >
> > > def test_remove_illegal_chars(self):
> > > """The illegal chars are replaced by unicode objects, valid on
> > > windows."""
> > > original_path = self.testfile + ''.join(WINDOWS_ILLEGAL_CHARS_MAP)
> > > path = remove_illegal_chars(original_path)
> > > assert not any(c in path for c in WINDOWS_ILLEGAL_CHARS_MAP)
> > >
> > > path = restore_illegal_chars(path)
> > > self.assertEqual(original_path, path)
> > >
> >
> > Same as the previous comment, I would add a test for restore_illegal_chars
> > that simply asserts that no illegal chars are present.
>
> You need to ensure more than that, you need to ensure that no character gets
> lost in the process. Otherwise we could provide an implementation like this
> that would make the test you suggest, pass:
>
> def remove_illegal_chars(self, path):
> return ''
>
> > > * as per out chat this morning, get_syncdaemon_valid_path was supposed to
> > > disappear. Why is still being used in filesystem_notifications.py?
> > >
> > > 275 + 'path': head.replace(get_syncdaemon_valid_path(self.path),
> > > 297 + full_filename = get_syncdaemon_valid_path(file_name)
> > >
> >
> > Because the Event class that is passed to syncdaemon with the event
> > information should be returning paths that do not have \\?\, illegal chars
> and
> > that are bytes, otherwise we would be leaking this details to sd through the
> > events.
>
> But how come you told me yesterday morning that the method could go away?
> Agreeing one thing in IRC and then seeing something different in the MP is
> very confusing, and a bit frustrating I must confess.

Indeed, we chatted and then I wrote the code and realized that we should return bytes and not literal paths, I much refer to leave the method and ignore such details in that part of the code which is complicated.

>
> > > * are you sure we need the decorator "@windowspath(paths_indexes=[1])" for
> > > filesystem_notifications.py:get_wd?
>
> Maybe you missed this question. Can you please tell me a little more about
> this? ^

get_wd gets a path and looks at the stored paths in the WatchManager dictionary _wdm. _wdm contains a dictionary of wd (watch id) as keys and Watches as values. If you look at line 409 in filesystem_notifications the path that is passed to the creation of a Watch is one that was mangled with to get the windows correct paths.

This means that ig get_wd does not get a windows valid path, when looking for it in the Watches it won't find the correct wd because u'test%interrobang%' != 'test?'. Pay also careful attention to the fact that get_wd does not care if the path is abs or not but the Wathcer does need it to be absolute.

Does it make sense?