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

On 21 June 2010 07:25, Robert Collins <email address hidden> wrote:
> This:
>
> 195     -        self.check_output(revs[1], 'cat-revision a@r-0-1')
> 196     -        self.check_output(revs[2], 'cat-revision a@r-0-2')
> 197     -        self.check_output(revs[3], 'cat-revision a@r-0-3')
> 198     -
> 199     -        self.check_output(revs[1], 'cat-revision -r 1')
> 200     -        self.check_output(revs[2], 'cat-revision -r 2')
> 201     -        self.check_output(revs[3], 'cat-revision -r 3')
> 202     -
> 203     -        self.check_output(revs[1], 'cat-revision -r revid:a@r-0-1')
> 204     -        self.check_output(revs[2], 'cat-revision -r revid:a@r-0-2')
> 205     -        self.check_output(revs[3], 'cat-revision -r revid:a@r-0-3')
> 206     +        for i in [1, 2, 3]:
> 207     +            self.assertEqual(revs[i],
> 208     +                self.run_bzr('cat-revision -r revid:a@r-0-%d' % i)[0])
> 209     +            self.assertEqual(revs[i],
> 210     +                self.run_bzr('cat-revision a@r-0-%d' % i)[0])
> 211     +            self.assertEqual(revs[i],
> 212     +                self.run_bzr('cat-revision -r %d' % i)[0])
>
> Results in harder to evaluate exceptions. I realise its very unlikely to fail but if it does you'll get something like
> 'serialised_revision' != 'other serialized revision'
>
> and no evidence of *what* revision id we were actually asking for.
>
> I'm going to land this as this code is pretty unlikely to change, but I think we should generally avoid such for loops vigorously: use parameterisation instead, or explicit lists as the test had before.

I think the exception from run_bzr tells you what command it's
running, so that should make it quite clear what failed. If your test
framework tells you "manually unroll loops by copy&paste" then the
framework is buggy. :-)

--
Martin