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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Is it still needed ?

Overall, nice demo of script usefulness... thanks :)

My vote is 'Tweak' make sure you land https://code.edge.launchpad.net/~mbp/bzr/scripts/+merge/27581 first or get rid of it.

review: Needs Fixing

« Back to merge proposal