[rfc] add a TestFactory class or concept

[Martin Pool]

2009/8/7 Andrew Bennetts <andrew.bennetts at canonical.com>:
> Robert Collins wrote:
>> On Thu, 2009-08-06 at 18:12 +1000, Martin Pool wrote:
>> > Lots of tests are run on some specific values; for instance they make
>> > a branch and then do some operations on it.
>> >
>> > I was talking to jml the other day and he said that Launchpad have a
>> > cleaner separation of all of this setup into a TestFactory class
>> > (iirc) - if you want a Person to test, you would always get that from
>> > the factory rather than putting arbitrary example data into the class.
>>
>> I don't really like the sound of this. I was looking at a test just the
>> other day that has its specific data elsewhere, and its really hard to
>> read. I'd rather focus on making it very pithy to put specific data
>> in-line.

OK, so one thing we clearly want is that if you get a traceback about
a particular object, you should be able to map it back to the code
that caused it.  (For me the totemic example here is errors in gc
where the path or object mentioned in the error may be all you have to
go on.)

In some ways, I think by making our tests more declarative -- that's a
good word for it -- we might make it easier to track down some
failures.  At the moment there's a fair amount of unnecessary
inconsistency between tests, and that only makes them slower to debug.
 Case in point: Aaron doesn't like BranchBuilder so if he does need to
fix a test that uses it, it will be that much more puzzling.  On the
other hand Aaron (iirc) uses the idiom of tests returning objects to
chain into other tests and that may confuse other people.

I don't expect we can make them all consistent overnight but if we
could agree on some standard helpers we could gradually converge.

> My understanding of the test factory idea is that this is exactly what it's
> intended to do.  So you might say inline, via calling the test factory, “give me
> a branch with property X” (e.g. X == has one commit, and that commit has a
> lefthand ghost).  So in the makePerson example from Launchpad you can optionally
> specify details like the display name or team memberships if a test is
> exercising something involving them.  I'd expect that a test would never assert
> something about a property it didn't explicitly request.
>
> The trick of course is finding a succint way to express the relevant details,
> and none of the irrelevant ones.  BranchBuilder partially succeeds at this; it
> lets you specify some of the dimensions of a branch's revision history fairly
> tersely, but you still have to deal with details like adding a root entry in the
> first rev when most tests don't care what the root looks like.
>
> I do like it when our test helpers allow the setup phase of our tests be
> declarative (and short).  As an example, stacking tests generally aren't (lots
> of creating multiple branches and run some fetches/sprouts/clones to setup the
> situation to test), and are harder to follow.

That's a good way to put it.

As far as what you're allowed to assert: I think a test should treat
anything it didn't specifically set as being a kind of opaque
variable.  It can assert that it has the same value it observed
earlier, as we already do that kind of test all the time, but it
shouldn't be assuming it has any particular value.

-- 
Martin <http://launchpad.net/~mbp/>