Code review comment for lp:~james-w/launchpad/fix-publishing-getbyid

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

> Hi James,
>
> Thanks for the cleanups.
>
> Was changing
> lib/lp/soyuz/scripts/tests/upload_test_files/drdsl_1.2.0.orig.tar.gz
> intentional ?

No, some make target or something seems to modify it here.

> The class definition of PublishingSetTests needs to be preceded by two empty
> lines.

Changed, thanks.

> There seems to be quite a bit of duplication in the various methods in
> PublishingSetTests, perhaps some of that code could be moved into a setUp()
> method.

Changed, thanks.

> I'm bothered by the size of the test factory and its lack of tests. Seeing it
> extended further worries me a bit, though I don't have any easy alternatives.

It's possible, but tedious to test the factory itself. We could start doing that?

Thanks,

James

« Back to merge proposal