Code review comment for lp:~eric97/bzr/dump-btree-api

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

> On Thu, 2011-02-17 at 09:46 +0000, Vincent Ladeuil wrote:
> > Review: Approve
> > Nicely done !
>
> Thanks.
>
> > You can just use:
> >
> > test_raw_output_3nodes = '''Root node:
> > B+Tree Graph Index 2
> > ...
> > '''
>
> That won't match, so it requires a change -- either:
> test_raw_output_0nodes = re.sub("(?m)^ *", "",
> '''Root node:
> B+Tree Graph Index 2
> ...
> ''')

I meant: use a triple quoting string and don't indent the lines in it so the string matches the output of the command without adding all the quotes and the '\n's.

> or:
> test_raw_output_0nodes = \
> '''Root node:
> B+Tree Graph Index 2
> ...
> '''
>

Same as my second example, don't worry, it's just a matter of taste, *I* dislike the '= \' and prefer '="""\" instead, no big deal, anyway I misread it as being defined at the top level where no leading spaces won't break the reading. poolie disliked embedding such strings in classes anyway, so forget about it.

> I don't understand: why is one easier to copy than the other?

Both of my examples are as easy to copy (the second one slightly more) and your second example is easy as well. The point is to avoid the additional quotes and '\n's.

>
> As for whether copying is bad, well, those data definitions, and most of
> the method that reads them, are already duplicated between the two test
> files. I didn't like doing that, but liked even less to reach across
> from one to the other:
> self.assertEquals(some_other_module.SomeClass.expected_value,
> actual_value)
> Plus, for the module that does that, it would put the test data far from
> the tests themselves, thus awkward to look up.

Right, the counter-argument is that you'll need to update both files if the output format change. There is no perfect answer to this problem when the tests can't be in the same file. In the end, it's a matter of taste and pragmatism.

« Back to merge proposal