Code review comment for lp:~mbp/bzr/externalbase

On 15 June 2010 18:13, Vincent Ladeuil <email address hidden> wrote:
> Review: Needs Fixing
> Yeah ! I was stealth-ly working toward the same goal :)
>
> Since you're there, how about replacing 'from bzrlib.tests import TestCaseWithTransport ... (TestCaseWithTransport)' by 'from bzrlib import tests.... (tests.TestCaseWithTransport)' ? :-p
>
> 953     - def check_error(self, output, *args):
> 954     - """Verify that the expected error matches what bzr says.
> 955     + def check_output(self, output, *args):
> 956     + """Verify that the expected output matches what bzr says.
> 957
> 958     The output is supplied first, so that you can supply a variable
> 959     number of arguments to bzr.
> 960     """
> 961     - self.assertContainsRe(self.run_bzr(args, retcode=3)[1], output)
> 962     + self.assertEquals(self.run_bzr(*args)[0], output)
>

> Really ? I'm surprised that changing a method name doesn't come with corresponding changes in the tests
themselves... This sounds like an intermediate step that you left
behind when switching to run_script().

Actually this is just a strange diff hunk. test_revision_info
declared a check_error but didn't use it. They did use check_output
from ExternalBase, and weren't particularly suitable for changing to
scripts. So for now I just gave them a check_output of their own.
This is a bit of a sideways step for code cleanliness, but I think
this patch is worthwhile overall, and we could clean
test_revision_info more later.
--
Martin