[MERGE/RFC] Better unicode symlinks handling (was Re: [MERGE][1.14] Fix bug #355454 by casting to unicode.)

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
+    dir_reader_scenarios = test_osutils.dir_reader_scenarios()
+
+    ue_scenarios = [('dirstate_Python',
+                     {'update_entry': dirstate.py_update_entry})]
+    if has_dirstate_helpers_c:
+        c_scenario = ('dirstate_C',
+                     {'update_entry': 
_dirstate_helpers_c.update_entry})
+        ue_scenarios.append(c_scenario)
+    process_entry_tests, remaining_tests = 
tests.split_suite_by_condition(
+        remaining_tests, tests.condition_isinstance(TestUpdateEntry))
+    tests.multiply_tests(process_entry_tests,
+                         tests.multiply_scenarios(dir_reader_scenarios,
+                                                  ue_scenarios),
+                         suite)
+
+    pe_scenarios = [('dirstate_Python',
+                     {'_process_entry': dirstate.ProcessEntryPython})]
+    if has_dirstate_helpers_c:
+        c_scenario = ('dirstate_C',
+                     {'_process_entry': 
_dirstate_helpers_c.ProcessEntryC})
+        pe_scenarios.append(c_scenario)
+    process_entry_tests, remaining_tests = 
tests.split_suite_by_condition(
+        remaining_tests, tests.condition_isinstance(TestProcessEntry))
+    tests.multiply_tests(process_entry_tests,
+                         tests.multiply_scenarios(dir_reader_scenarios,
+                                                  pe_scenarios),
+                         suite)
+
+    dir_reader_tests, remaining_tests = tests.split_suite_by_condition(
+        remaining_tests, tests.condition_isinstance(
+            test_dirstate.TestCaseWithDirState))
+    tests.multiply_tests(dir_reader_tests, dir_reader_scenarios, suite)
+    suite.addTest(remaining_tests)
+

^- I would move the line:
+    dir_reader_scenarios = test_osutils.dir_reader_scenarios()

to be close to where it is actually used (here at the end).

...

+def load_tests(basic_tests, module, loader):
+    suite = loader.suiteClass()
+    dir_reader_tests, remaining_tests = tests.split_suite_by_condition(
+        basic_tests, tests.condition_isinstance(TestCaseWithDirState))
+    tests.multiply_tests(dir_reader_tests,
+                         test_osutils.dir_reader_scenarios(), suite)
+    suite.addTest(remaining_tests)
+    return suite
+
+

^- this sure looks like a second test multiplication over the same 
tests.
(everything subclassing TestCaseWithDirState). Perhaps not, though.

...

  class TestCaseWithDirState(tests.TestCaseWithTransport):
      """Helper functions for creating DirState objects with various 
content."""

+    # Set by load_tests
+    _dir_reader_class = None
+    _native_to_unicode = None # Not used yet
+    _fs_enc = osutils._fs_enc # Not parametrized yet
+

^- I would avoid adding parameterization that we aren't using yet. It 
adds some
confusion as to what is being tested. If you feel strongly you can leave 
it,
but I think just deleting the variables would be reasonable.

...

+def _already_unicode(s):
+    return s
+
+def _fs_enc_to_unicode(s):
+    return s.decode(osutils._fs_enc)
+
+def _utf8_to_unicode(s):
+    return s.decode('UTF-8')
+


^- two blank lines for top-level functions.

...


+def dir_reader_scenarios():
+    # For each dir reader we define:
...
+    dir_readers = [(osutils.UnicodeDirReader, _already_unicode)]

^- Why do you build up a list of 'dir_readers' that you then iterate 
over to
cast into a dict of dir reader scenarios? Is there a reason why you 
didn't just
create the scenarios directly?



For details, see: 
http://bundlebuggy.aaronbentley.com/project/bzr/request/%3Cm2vdp3mf85.fsf%40free.fr%3E
Project: Bazaar