hi,<br><br><div><span class="gmail_quote">On 6/25/07, <b class="gmail_sendername">Robert Collins</b> &lt;<a href="mailto:robertc@robertcollins.net">robertc@robertcollins.net</a>&gt; wrote:</span><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
On Mon, 2007-06-25 at 19:40 +0200, Marius Kruger wrote:<br>&gt; hopefully this bundle address all the comments on the list.<br>&gt; The changes I made since the last patch are:<br><br>+1 Conditional on one further change:
</blockquote><div>... <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">assertRemoveErrorContainsRe can now away completely I think. I propose
<br>the following -<br>delete the asertRemoveErrorContainsRe method<br>change<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;e = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;TestRemove.files, keep_files=False)<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;self._assertRemoveErrorContainsRe(e, &#39;unknown:.*b/c.*b.*a.*d&#39;)
<br><br>into<br><br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;err = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;TestRemove.files, keep_files=False)<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;self._assertContainsRe(err.changes_as_text, &#39;unknown:.*b/c.*b.*a.*d&#39;)
</blockquote><div><br>done <br>(never mind my previous e-mail, <br>the pattern was a little wrong: the &#39;d&#39; should have been sorted to the front, <br>but because the was a &#39;d&#39; in the rest of the error message, this test used to pass.
<br>It seems its a good thing to make this change away from str(err), <br>and it seems a, b, c, d are not such a good choice for file names if the testing<br>is done by simple regexes... but hopefully there are not other such nasty surprises.)
<br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">The reason for this is:<br>The exception *formatting* should be tested in bzrlib/tests/test_errors.py
<br>The exception *throwing* should be testing what is supplied to the<br>BzrRemoveChangedFilesError constructor, not the current formatting which<br>could change, get internationalised etc.<br><br>This reduces the number of layers involved in your tests, and thus the
<br>number of noise changes you may encounter.</blockquote><div>This little piece of wisdom should probably go into the HACKING doc if it isn&#39;t there<br>already (I must admit, I haven&#39;t read it in a while)<br>&nbsp;<br>
<br></div></div>BTW if I run all the tests (on bzr.dev and my branch), this test fails:<br><blockquote>[1292/6951 in 80s, 43 skipped] test_permissions.TestSftpPermissions.test_new_files&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; /var/lib/python-support/python2.5/paramiko/message.py:226: DeprecationWarning: integer argument expected, got float
<br>&nbsp; self.packet.write(struct.pack(&#39;&gt;I&#39;, n))<br>FAIL: blackbox.test_status.TestStatus.test_status_versioned<br>&nbsp;&nbsp;&nbsp; Unexpected return code<br>not equal:<br>a = 0<br>b = 3<br></blockquote><br>Anyways, here are the changes since the previous patch
<br><br>=== modified file &#39;bzrlib/tests/workingtree_implementations/test_remove.py&#39;<br>--- bzrlib/tests/workingtree_implementations/test_remove.py&nbsp;&nbsp;&nbsp;&nbsp; 2007-06-23 19:45:17 +0000<br>+++ bzrlib/tests/workingtree_implementations/test_remove.py&nbsp;&nbsp;&nbsp;&nbsp; 2007-06-28 05:42:19 +0000
<br>@@ -16,7 +16,6 @@<br><br>&nbsp;&quot;&quot;&quot;Tests for interface conformance of &#39;WorkingTree.remove&#39;&quot;&quot;&quot;<br><br>-import re<br>&nbsp;from bzrlib.tests.workingtree_implementations import TestCaseWithWorkingTree
<br>&nbsp;from bzrlib import errors, osutils<br><br>@@ -34,11 +33,6 @@<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.build_tree(TestRemove.files)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; return tree<br><br>-&nbsp;&nbsp;&nbsp; def _assertRemoveErrorContainsRe(self, exception, file_detail_re):<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
self.assertContainsRe(str(exception),<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &#39;(?s)Can\&#39;t remove changed or unknown&#39;<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &#39; files:.*&#39; + file_detail_re)<br>-<br>&nbsp;&nbsp;&nbsp;&nbsp; def test_remove_keep(self):<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &quot;&quot;&quot;Check that files are unversioned but not deleted.&quot;&quot;&quot;
<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; tree = self.getTree()<br>@@ -66,9 +60,10 @@<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; tree = self.getTree()<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; tree.add(TestRemove.files)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(TestRemove.files)<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; e = self.assertRaises(errors.BzrRemoveChangedFilesError
, tree.remove,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; err = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; TestRemove.files, keep_files=False)<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self._assertRemoveErrorContainsRe(e, &#39;added:.*a.*b.*b/c.*d&#39;)
<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertContainsRe(err.changes_as_text,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &#39;(?s)added:.*a.*b/.*b/c.*d/&#39;)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(TestRemove.files)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.failUnlessExists(TestRemove.files)<br><br>
@@ -79,9 +74,9 @@<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; tree.commit(&quot;make sure a is versioned&quot;)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.build_tree_contents([(&#39;a&#39;, &quot;some other new content!&quot;)])<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(TestRemove.a)
<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; e = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; err = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; TestRemove.a, keep_files=False)<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self._assertRemoveErrorContainsRe(e, &#39;modified:.*a&#39;)
<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertContainsRe(err.changes_as_text, &#39;(?s)modified:.*a&#39;)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(TestRemove.a)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.failUnlessExists(TestRemove.a)<br><br>@@ -131,9 +126,10 @@<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
self.assertInWorkingTree(rfilesx)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.failUnlessExists(rfilesx)<br><br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; e = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; err = self.assertRaises(errors.BzrRemoveChangedFilesError
, tree.remove,<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; rfilesx, keep_files=False)<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self._assertRemoveErrorContainsRe(e, &#39;modified:.*ax.*bx/cx&#39;)<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertContainsRe(err.changes_as_text,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &#39;(?s)modified:.*ax.*bx/cx&#39;)
<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(rfilesx)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.failUnlessExists(rfilesx)<br><br>@@ -151,9 +147,10 @@<br>&nbsp;&nbsp;&nbsp;&nbsp; def test_remove_unknown_files(self):<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &quot;&quot;&quot;Try to delete unknown files.&quot;&quot;&quot;
<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; tree = self.getTree()<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; e = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; err = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; TestRemove.files
, keep_files=False)<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self._assertRemoveErrorContainsRe(e, &#39;unknown:.*b/c.*b.*a.*d&#39;)<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertContainsRe(err.changes_as_text,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &#39;(?s)unknown:.*d/.*b/c.*b/.*a.*&#39;)<br><br>
&nbsp;&nbsp;&nbsp;&nbsp; def test_remove_nonexisting_files(self):<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &quot;&quot;&quot;Try to delete non-existing files.&quot;&quot;&quot;<br>@@ -180,9 +177,10 @@<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(TestRemove.files)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.failUnlessExists
(TestRemove.files)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.build_tree([&#39;b/my_unknown_file&#39;])<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; e = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; err = self.assertRaises(errors.BzrRemoveChangedFilesError
, tree.remove,<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; TestRemove.b, keep_files=False)<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self._assertRemoveErrorContainsRe(e, &#39;unknown:.*b/my_unknown_file&#39;)<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertContainsRe(err.changes_as_text,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &#39;(?s)unknown:.*b/my_unknown_file&#39;)
<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(TestRemove.b)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.failUnlessExists(TestRemove.b)<br><br>@@ -203,9 +201,9 @@<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; tree.commit(&quot;make sure b and c are versioned&quot;)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.build_tree_contents
([(&#39;b/c&#39;, &quot;some other new content!&quot;)])<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(TestRemove.b_c)<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; e = self.assertRaises(errors.BzrRemoveChangedFilesError, tree.remove,<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; err = self.assertRaises
(errors.BzrRemoveChangedFilesError, tree.remove,<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; TestRemove.b, keep_files=False)<br>-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self._assertRemoveErrorContainsRe(e, &#39;modified:.*b/c&#39;)<br>+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertContainsRe(err.changes_as_text
, &#39;(?s)modified:.*b/c&#39;)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.assertInWorkingTree(TestRemove.b_c)<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; self.failUnlessExists(TestRemove.b_c)<br><br><br><br><br>regards<br>marius<br><br>-- <br><a href="http://bazaar-vcs.org">bazaar-vcs.org
</a><br>Because I don&#39;t trust Version Control Systems with less than 6350 unit tests.