[MERGE/RFC] Better unicode symlinks handling

[Vincent Ladeuil]

>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:

    jam> John Arbash Meinel has voted tweak.
    jam> Status is now: Conditionally approved
    jam> Comment:
    jam> +    dir_reader_scenarios = test_osutils.dir_reader_scenarios()
    jam> +

<snip/>

    jam> +    tests.multiply_tests(process_entry_tests,
    jam> +                         tests.multiply_scenarios(dir_reader_scenarios,

One.

    jam> +                                                  ue_scenarios),
    jam> +                         suite)

<snip/>

    jam> +    tests.multiply_tests(process_entry_tests,
    jam> +                         tests.multiply_scenarios(dir_reader_scenarios,

Two.

    jam> +                                                  pe_scenarios),
    jam> +                         suite)

<snip/>

    jam> +    tests.multiply_tests(dir_reader_tests, dir_reader_scenarios, suite)

Three.

    jam> +    suite.addTest(remaining_tests)
    jam> +

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

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

Nope, it's used thrice.

    jam> ...

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

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

I don't understand the 'second' here :-/

- multiply_tests parametrize TestCaseWithDirState tests against the dir readers.
- the remaining tests are added as is 

    jam> ...

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

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

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

Done.

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

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

Not really...

Fixed. Ironically fixing it revealed that it's easy to mistype
attribute names in the dicts :)

           Vincent