Merge lp:~gz/bzr/tests_for_bug_205636 into lp:bzr

[Martin Packman]

The problem of adding files in a directory that used to be a file described in bug 205636 was found to have been fixed by the bug 192859 symlink related changes. However, no tests specifically for that case were added. While making this branch, I discovered only one on my two ways of reproducing the problem has actually been fixed, but these tests can land at least.

I followed the per_workingtree placement in Martin Pool's symlink branch tests. However, I'm not sure the test_add test that fails with the older working tree formats is really valid, from the command line I only get failures down the smart_add path so perhaps add isn't expected to deal with this kind of oddness.

The remaining issue that will need fixing can be reproduced with:

    >bzr init test
    >cd test
    >echo Test>dir
    >bzr add dir
    >del dir
    >mkdir dir\dir
    >bzr add dir

(What, "bash: del: command not found"? I'm not crazy about shell tests either.)

[Martin Packman (gz)]

Looks like bug 251864 sort of covers the smart_add case that still fails.

[Vincent Ladeuil (vila)]

>>>>> Martin [gz] <email address hidden> writes:

<snip/>
    > The remaining issue that will need fixing can be reproduced with:

    >> bzr init test
    >> cd test
    >> echo Test>dir
    >> bzr add dir
    >> del dir
    >> mkdir dir\dir
    >> bzr add dir

    > (What, "bash: del: command not found"? I'm not crazy about shell tests either.)

Try shell-like tests then :-p

[John A Meinel (jameinel)]

I don't understand the need for the "reraise as Assertion" code. Why not just call "self.knonwFailure()". ?

[Martin Packman (gz)]

I was writing these tests to find out what triggered the bug, and when it was fixed. Using self.knownFailure means the problem code is never actually run. With self.expectFailure instead, the code *is* run, and if it starts passing (after the merge in the second rev for instance) then you get an unexpected success (but see bug 654474) instead. An expectError method could also fail if the error type changes.

With lp:~gz/bzr/smart_add_former_file_dir_251864 merged as well, this rather ugly assertion adapter code is removed as the test starts passing and it's not needed any more.

[Martin Packman (gz)]

> Try shell-like tests then :-p

I did, and found a bug in the error handling both there and in my own code testtools...

[John A Meinel (jameinel)]

63 + try:
64 + tree.smart_add(["dir"])
65 + except AttributeError, e:
66 + self.expectFailure("Adding dir raises AttributeError",
67 + self._reraise_current_error_as_assertion)

^- Looking at this, I don't see any reason why expectFailure would "run the code".

If you did:

self.expectFailure("Adding dir raises AttributeError",
  self._reraise_error_as_assertion, tree.smart_add, ['dir'])

Or something along those lines, then I could see your point. The try/except code means that you will *only* call self._reraise_current... if an exception has already triggered, and as such you always know that the code is executing, etc.

try:
 ...
except AttributeError:
  self.knownFailure(...)

is much more straightforward.

[Martin Packman (gz)]

Yeah, your two alternative spellings there are much clearer. I'll leave this branch unchanged for the moment though as that was a temporary hack that's going away.

Having a real expectError method would both keep the exc_info unmolested and avoid the need for try/except, but was out of scope here.

[Martin Packman (gz)]

I would really like some input on:

* If test_add is correct, and whether the old working tree formats should be fixed.
* If anything else should be asserted in the tests, and if so what.

[Martin Pool (mbp)]

> > (What, "bash: del: command not found"? I'm not crazy about shell tests
> either.)
>
> Try shell-like tests then :-p

The point being, of course, that they should rarely or never actually depend on local OS shell behaviour. rm in a shell-like test just directly deletes it.

[Martin Pool (mbp)]

> I would really like some input on:
>
> * If test_add is correct, and whether the old working tree formats should be
> fixed.
> * If anything else should be asserted in the tests, and if so what.

The tests look reasonable.

I would not bother fixing the old formats.

I would think about adding some additional assertions that the files within the directories are now versioned.

review: Approve

[Vincent Ladeuil (vila)]

Summarizing the comments:
* add assertions that 'dir' and 'dir/file' are versioned
* get rid of the re-raise if it's not needed anymore

And I'll add: avoid importing os and use the osutils equivalents. While not strictly necessary yet, we plan to make working trees fully testable in-memory via transports and 'os' calls won't work there. Limiting such calls to the minimum needed will help.

review: Approve

[Martin Packman (gz)]

Added some state of tree assertions, again modelled on what Martin Pool used in the bug 192859 branch. This showed the test_add test was really not doing the right thing. As it isn't what `bzr add` ends up using and no one outright said it was valid, I just removed it.

I don't get what you mean about os.remove Vincent. I'd need a transport method to avoid filesystem specificity not just osutils, and my understanding was this is not yet done as per bug 606249 and so on.

=== modified file 'bzrlib/tests/per_workingtree/test_smart_add.py'
--- bzrlib/tests/per_workingtree/test_smart_add.py	2010-02-23 07:43:11 +0000
+++ bzrlib/tests/per_workingtree/test_smart_add.py	2010-11-02 22:11:44 +0000
@@ -17,6 +17,7 @@
 """Test that we can use smart_add on all Tree implementations."""
 
 from cStringIO import StringIO
+import os
 import sys
 
 from bzrlib import (
@@ -202,6 +203,48 @@
         self.assertEqual(['', 'dir', 'dir/subdir', 'dir/subdir/foo'],
             [path for path, ie in tree.iter_entries_by_dir()])
 
+    @staticmethod
+    def _reraise_current_error_as_assertion():
+        _, e, tb = sys.exc_info()
+        try:
+            raise AssertionError, AssertionError(str(e)), tb
+        finally:
+            del tb
+
+    def test_add_dir_bug_205636(self):
+        """Added file turning into a dir should be detected on add dir"""
+        tree = self.make_branch_and_tree(".")
+        self.build_tree(["dir"]) # whoops, make a file called dir
+        tree.smart_add(["dir"])
+        os.remove("dir")
+        self.build_tree(["dir/", "dir/file"])
+        # GZ 2010-10-17: Need an expectError method like expectFailure but for
+        #                execptions that are not assertions
+        try:
+            tree.smart_add(["dir"])
+        except AttributeError, e:
+            self.expectFailure("Adding dir raises AttributeError",
+                self._reraise_current_error_as_assertion)
+        tree.commit("Add dir contents")
+        self.addCleanup(tree.lock_read().unlock)
+        self.assertEqual([(u"dir", "directory"), (u"dir/file", "file")],
+            [(t[0], t[2]) for t in tree.list_files()])
+        self.assertFalse(list(tree.iter_changes(tree.basis_tree())))
+
+    def test_add_subdir_file_bug_205636(self):
+        """Added file turning into a dir should be detected on add dir/file"""
+        tree = self.make_branch_and_tree(".")
+        self.build_tree(["dir"]) # whoops, make a file called dir
+        tree.smart_add(["dir"])
+        os.remove("dir")
+        self.build_tree(["dir/", "dir/file"])
+        tree.smart_add(["dir/file"])
+        tree.commit("Add file in dir")
+        self.addCleanup(tree.lock_read().unlock)
+        self.assertEqual([(u"dir", "directory"), (u"dir/file", "file")],
+            [(t[0], t[2]) for t in tree.list_files()])
+        self.assertFalse(list(tree.iter_changes(tree.basis_tree())))
+
     def test_custom_ids(self):
         sio = StringIO()
         action = test_smart_add.AddCustomIDAction(to_file=sio,