[PATCH] [Bug 209281] Re: Windows diff apps don't understand symlinks created by Cygwin bzr diff --using

[Matt McClure]

I'll have a new patch as soon as I have a chance to change it to use
UnavailableFeature.

I realized that 'windows' isn't the value of sys.platform I want to
test.  I changed it to 'win32' everywhere applicable.

On Wed, Apr 16, 2008 at 9:18 PM, Aaron Bentley <aaron at aaronbentley.com> wrote:
> Matt McClure wrote:
>  > +    def test_windows_tool_reads_both_files(self):
>
> > +        if (sys.platform != 'windows'
>  > +            and sys.platform != 'cygwin'):
>  > +            raise tests.TestSkipped('Platform does not have Windows tools.')
>
> > +        output = StringIO()
>  > +        tree = self.make_branch_and_tree('tree')
>  > +        self.build_tree_contents([('tree/file', 'content')])
>  > +        tree.add('file', 'file-id')
>  > +        tree.commit('old tree')
>
> > +        tree.lock_read()
>  > +        self.addCleanup(tree.unlock)
>  > +        diff_obj = DiffFromTool(['python', '-c',
>  > +                                 """
>  > +import subprocess
>  > +proc = subprocess.Popen(["attrib", "%(old_path)s"],
>  > +                        stdout=subprocess.PIPE)
>  > +proc.wait()
>  > +print proc.stdout.read()
>  > +proc = subprocess.Popen(["attrib", "%(new_path)s"],
>  > +                        stdout=subprocess.PIPE)
>  > +proc.wait()
>  > +print proc.stdout.read()
>  > +"""
>
>  This is pretty involved.  I think using attrib directly was clearer.

Really?  In the new implementation the diff tool is a program that
uses a Windows tool to verify it can read both files.  In other words,
the fixture and exercise portions of the test are clearly separated
from the verification portion.  In the old implementation, the diff
tool was just a dummy, and calling attrib was part of the verification
rather than the fixture that is exercised.

>  > +        self.assertNotContainsRe(lines, 'Path not found')
>
>  Checking the text of an error is a fairly dirty thing to do.  It would
>  be better to check the status code.  Error messages may not be
>  consistent.  Especially, they may vary by language.

I can remove that verification.  The test should be just as valid
using only the assertContainsRe() calls on each file path string.
Unfortunately, attrib exits with 0 regardless of its success or
failure to find the argument file.

>  > @@ -1281,6 +1313,8 @@
>
> >          tree.commit('old tree', timestamp=0)
>  >          tree.rename_one('oldname', 'newname')
>  >          self.build_tree_contents([('tree/newname', 'newcontent')])
>  > +        self.build_tree_contents([('tree/newname2', 'newcontent2')])
>  > +        tree.add('newname2', 'file-id2')
>  >          old_tree = tree.basis_tree()
>  >          old_tree.lock_read()
>  >          self.addCleanup(old_tree.unlock)
>
>  ^^^ I don't understand the purpose of adding newname2.

You're right.  It's not necessary.  I removed it.

Matt