[merge] 'bzr version-info'

[Robert Collins]

On Wed, 2006-09-20 at 10:16 -0500, John Arbash Meinel wrote:
> 
> Robert indicated that because this is a new command it has a chance to
> make it into 0.11 (since it can't create regressions in other
> commands). 

+1 with some tweaks please:

+ def _parse_version_info_format

could do with a docstring

+        if location is None:
+            location = u'.'

Isn't the u noise? (open_containing wants a url, so transforming
location to a url first would be a good thing.)


+    def test_invalid_format(self):
+        bzr = self.run_bzr
+
+        bzr('version-info', '--format', 'quijibo', retcode=3)
+

I think vertical whitespace here is cruel and unsual punishment.

Also this should just use self.run_bzr - the temporary variable does not
buy anything.


+    def test_default(self):
+        # smoketest that not supplying a --format still works
+        self.create_branch()
+
+        info = self.run_bzr('version-info', 'branch')[0]
+

And here too - for two lines of code, a space between them makes the
separation of functions harder to 'get' visually.


+        open('branch/c', 'wb').write('now unclean\n')

This should be a self.build_tree_contents call.


+        def bzr(*args, **kwargs):
+            return self.run_bzr(*args, **kwargs)[0]

I'm not sure this helper really helps - but I dont insist it be removed.
But as is it should get a docstring (even one that says 'return stdout
from running a bzr command') (And see self.capture() if you want that
from the test suite).


+            outf = open('test_version_information.py', 'wb')
+            outf.write(txt)
+            outf.close()

Another build_tree_contents case. (as all the cases of open() in this
test script.


+# TODO: jam 20051228 When part of bzrlib, this should become
+#       from bzrlib.generate_version_info import foo

Perhaps this TODO should be done ? :)


Theres more open() abuse in the main test file too.

+        # We have both a working tree, and we are checking
+        # COMPATIBILITY:
+        # bzr < 0.9 did not have Tree.changes_from
+        try:
+            delta = self._working_tree.changes_from(basis_tree)
+        except AttributeError:
+            import bzrlib.delta
+            delta = bzrlib.delta.compare_trees(basis_tree,
self._working_tree)
+

Thats not needed: you're in bzr core, theres no room for skew.


Finally some general thoughts: I think your blackbox tests are testing
too much : rather than testing that the magic smoke is in, they are
testing specific functionality - like the validity of the python file. I
think we dont really need that in the blackbox test : those tests aim to
test that the glue code in bzrlib/builtins.py /
bzrlib/cmd_version_info.py is being invoked correctly.

So I think that the blackbox ui tests should be scaled back sharply
before more.

With the details changes addressed, and the blackbox tests not focused
more sharply, I'm at about +0.8.

As far as 0.11 goes, I'm happy for it to go in now, even though we're
frozen, as long as we get to a total +2 for this.

-Rob



-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060921/ac42ed24/attachment.pgp