Code review comment for lp:~james-w/launchpad/no-more-sampledata-2

Revision history for this message
James Westby (james-w) wrote :

Thanks for the review.

> Most of the review comments are about dropping calls to sync()
> entirely and adding comments to certain calls to removeSecurityProxy –
> it's good to know why a test feels the need to fiddle with internals.

wgrant said pretty much the same things too, I'm going to take a bit
more time to try and do things more cleanly.

> Also, I feel obliged to include this snippet of a conversation I had
> with Julian, in case they are relevant to this patch or trigger new
> thinking about the no-more-sampledata work you've been doing.
>
> #launchpad-code, 2010-08-03, 16:13-16:19:
> <bigjools> jml: I'm a bit uncomfortable with the factory changes to skip STP
> <jml> bigjools, why so?
> <bigjools> jml: a few reasons. One, because it's not differentiating
> between binary and source publications. Two, someone will want to make
> it do binaries at some point and there will be pain because they're an
> order of magnitiude more complicated to set up, and three, the STP
> relies on sample data because there's a load of publisher config in it
> which is not being set up in the factory methods so you can end up
> with data
> <bigjools> that can't exist in the real world.
> <bigjools> and probably more problems if I look harder
> <bigjools> not insurmountable of course, but nonetheless the problems exist

I saw this conversation. I'm keen to solve these problems such that the
factory is sufficient, but I don't know of many concrete cases where the
sampledata is different, one of the problems of sampledata.

One thing I know is that lucilleconfig isn't set on factory created
distributions and distroseries, and I am working on a way to deal
with that. That's an issue where your tests won't run, rather than
testing the wrong thing though.

Thanks,

James

« Back to merge proposal