[merge] 'bzr version-info'

[John Arbash Meinel]

Robert Collins wrote:
> 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). 
> 

Thanks for looking over this thoroughly. A lot of this stems from a
project that has been around a while, and I'm happy to clean it up.

> +1 with some tweaks please:
> 
> + def _parse_version_info_format
> 
> could do with a docstring

Done.


> 
> +        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.)

It didn't used to be noise (pre 0.9 that didn't translate to urls). But
yes, it is noise now.

> 
> 
> +    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.

done.

> 
> 
> +    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.
> 

sure.

> 
> +        open('branch/c', 'wb').write('now unclean\n')
> 
> This should be a self.build_tree_contents call.

done.

> 
> 
> +        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).
> 

removed.

> 
> +            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.

Done.

> 
> 
> +# TODO: jam 20051228 When part of bzrlib, this should become
> +#       from bzrlib.generate_version_info import foo
> 
> Perhaps this TODO should be done ? :)
> 

It actually was done, and I just forgot to remove the comment. (The old
code did:
from bzrlib.plugins.version_info.version_info_formats.format_rio ...)


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

One of them is necessary, because the api expects a file-like object.
But the other has been fixed. (And that one was updated with a try/finally).

> 
> +        # 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.
> 

done.

> 
> 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

I refined the blackbox tests. So now I'm basically just testing that
specific command-line flags have the right effect. And a basic test that
both --format=rio and --format=python is available.

Are you happy enough now? I'm attaching just the blackbox test, since
the rest was just the same cleanup that you mentioned.

When you say +2, are you asking for a second reviewer? Or just for you
to be +1, on top of myself?

John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_version_info.py
Type: text/x-python
Size: 4125 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060921/78c209c1/attachment.py 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060921/78c209c1/attachment.pgp