Review: [MERGE] Convert test.blackbox.test_c* to use internals where appropriate

[Andrew Bennetts]

BB:tweak

bzrlib/tests/blackbox/test_cat.py
---------------------------------

>      def test_cat(self):
> -
> -        def bzr(*args, **kwargs):
> -            return self.run_bzr_subprocess(*args, **kwargs)[0]
[...]
> -        bzr('cat', 'a', retcode=3)
> -        bzr('cat', 'a', '-r', 'revno:1:branch-that-does-not-exist', retcode=3)
> -        
> +        self.run_bzr('cat', 'a', retcode=3)
> +        self.run_bzr('cat', 'a', '-r', 'revno:1:branch-that-does-not-exist', retcode=3)

You replace run_bzr_subprocess with run_bzr.  As explained by Alexander, this
test should still use run_bzr_subprocess:

    “I'm sorry, but avoiding subprocess is wrong thing.
    'bzr cat' should be tested in subprocess on win32, otherwise
    it's hard to test mangling of line-endings on windows.
    
    Either make it conditional depending on platform,
    or make other call for bzr commands without invoking subprocess.”

(quoted from a review rejecting an earlier proposed change to test_cat, see
http://bundlebuggy.aaronbentley.com/request/%3C20070718080617.GK27285%40steerpike.home.puzzling.org%3E)

So, the other changes to use internals are ok, but when you run bzr here we
still need run_bzr_subprocess to ensure coverage of line-ending mangling on
windows.

I was planning on submitting a patch to add a comment to this test to explain
this, but hadn't gotten around to it.  Could you do it for me? :)

I think a comment like this would do:

    # We use run_bzr_subprocess rather than run_bzr here so that we can test
    # mangling of line-endings on Windows.

Really we don't need to run every single one of these "cat" invocations in a
subprocess (e.g. the case where it fails because the file isn't versioned
presumably is irrelevant to testing line-mangling), so ideally we'd be a bit
more selective than using run_bzr_subprocess for every cat in this test.  If you
want to tidy that up, that'd be great, but otherwise keep using
run_bzr_subprocess so that we don't lose coverage.  If you do tidy it up, make
sure you get Alexander to take a look so he's still comfortable with it.

[...]
> +        self.assertEquals(self.run_bzr('cat', 'a')[0], 'foo\n')
> +
> +        tree.commit(message='2')
> +        self.assertEquals(self.run_bzr('cat', 'a')[0], 'baz\n')
> +        self.assertEquals(self.run_bzr('cat', 'a', '-r', '1')[0], 'foo\n')
> +        self.assertEquals(self.run_bzr('cat', 'a', '-r', '-1')[0], 'baz\n')

In addition to fixing these to use run_bzr_subprocess, it's tempting to add a
custom assertion method like:

    def assertCatOutput(self, output, revision_arg=None):
        args = ['cat', 'a']
        if revision_arg is not None:
            args += ['-r', revision_arg]
        self.assertEquals(self.run_bzr_subprocess(*args)[0], output)

So those lines would be:

        self.assertCatOutput('baz\n')
        self.assertCatOutput('foo\n', revision_arg='1')
        self.assertCatOutput('baz\n', revision_arg='-1')

But that's possibly overcomplicating things.

> +        rev_id = tree.branch.revision_history()[-1]

As Aaron says, use last_revision() instead.


bzrlib/tests/blackbox/test_commit.py
------------------------------------

> +++ bzrlib/tests/blackbox/test_commit.py	2007-08-03 13:48:29 +0000
> @@ -26,6 +26,7 @@
>      )
>  from bzrlib.branch import Branch
>  from bzrlib.bzrdir import BzrDir
> +from bzrlib.conflicts import resolve

As Aaron says, you don't use this.

(Incidentally, pyflakes says that 're', 'sys', 'Branch', 'BzrCommandError' and
'WorkingTree' are imported but unused too.  Feel free to tidy that while you're
there...)


bzrlib/tests/blackbox/test_conflicts.py
---------------------------------------

> +        a_tree = self.make_branch_and_tree('a')
> +        self.build_tree_contents([
> +            ('a/myfile',        'contentsa\n'),
> +            ('a/my_other_file', 'contentsa\n'),
> +            ('a/mydir/',                     ),
> +            ])

PEP 8 says not to use whitespace to align things.

-Andrew.