Merge lp:~james-w/launchpad/no-more-sampledata-0 into lp:launchpad
| Status: | Merged |
|---|---|
| Approved by: | Jonathan Lange on 2010-08-03 |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 11293 |
| Proposed branch: | lp:~james-w/launchpad/no-more-sampledata-0 |
| Merge into: | lp:launchpad |
| Diff against target: |
1358 lines (+769/-248) 10 files modified
lib/lp/soyuz/tests/ppa.py (+13/-7) lib/lp/soyuz/tests/soyuz.py (+15/-10) lib/lp/soyuz/tests/soyuzbuilddhelpers.py (+4/-3) lib/lp/soyuz/tests/test_archive.py (+135/-155) lib/lp/testing/__init__.py (+3/-8) lib/lp/testing/factory.py (+84/-57) lib/lp/testing/matchers.py (+135/-0) lib/lp/testing/sampledata.py (+38/-0) lib/lp/testing/tests/test_factory.py (+171/-8) lib/lp/testing/tests/test_matchers.py (+171/-0) |
| To merge this branch: | bzr merge lp:~james-w/launchpad/no-more-sampledata-0 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jonathan Lange (community) | 2010-07-31 | Approve on 2010-08-03 | |
|
Review via email:
|
|||
Description of the Change
Hi,
This changes a bunch of soyuz tests to not use the sampledata.
It goes a bit further than that in that it restructures them to
have less setup and to create the things they need using the
factory rather than SoyuzTestPublisher.
In addition I moved some other code that is too deeply buried
right now in to lp.testing.
with some new methods to satisfy the needs of the changed code.
Thanks,
James
| Jonathan Lange (jml) wrote : | # |
| James Westby (james-w) wrote : | # |
> The review below mostly picks up on some naming standard glitches and
> points out some things that could be improved in the soyuz tests
> (rather than in your changes). However, I've also got a couple of
> questions about your changes and would very much appreciate hearing
> back from you.
>
> I also feel a little guilty about making so many petty comments when
> you've done such a helpful thing. Please do not feel obliged to act on
> all of my suggestions.
Thanks for the review. The intent is to improve the understandability
of the tests, so your suggestions are helpful.
> > === modified file 'lib/lp/
> > --- lib/lp/
> > +++ lib/lp/
> > @@ -25,6 +25,10 @@
> > from lp.soyuz.
> > from lp.soyuz.
> > PackagePublishi
> > +from lp.testing.
> > + HOARY_
> > + MOZILLA_
> MOZILLA_
> > + NAME16_
> >
>
> I'm very glad you've started putting things in the sampledata module.
>
> Would it be possible to change these names to clarify the intent
> behind their use?
>
> For example, NAME16_PERSON_NAME tells me nothing about the person
> object, and when I see it in a test, I don't know why name16 was
> chosen instead of name15. UBUNTU_
> Likewise, it would be good to have intent-revealing names for the
> FIREFOX constants and the HOARY constants.
I can do that, but it will require some research. It's one of the
problems with sampledata that I don't know what the intent is.
> My comment about the other error message applies here. I think it can
> be dropped. (If archives have unhelpful __repr__ implementations, we
> should fix that rather than working around it all the time in tests).
They have the default repr, should we change that?
> Hmm. These assertions look very much like the ones previous. Is there
> a domain-specific assertion method or Matcher to be had here?
Probably, I'll take a look. I think there's only two uses, so it hadn't
hit my refactoring threshold yet.
> I think this one needs a comment. What's an LFA?
LibraryFileAlias, I thought it was standard parlance, should I expand the
variable name instead?
Thanks,
James
| Jonathan Lange (jml) wrote : | # |
On Mon, Aug 2, 2010 at 2:25 PM, James Westby <email address hidden> wrote:
...
>> > === modified file 'lib/lp/
>> > --- lib/lp/
>> > +++ lib/lp/
>> > @@ -25,6 +25,10 @@
>> > from lp.soyuz.
>> > from lp.soyuz.
>> > PackagePublishi
>> > +from lp.testing.
>> > + HOARY_
>> > + MOZILLA_
>> MOZILLA_
>> > + NAME16_
>> >
>>
>> I'm very glad you've started putting things in the sampledata module.
>>
>> Would it be possible to change these names to clarify the intent
>> behind their use?
>>
>> For example, NAME16_PERSON_NAME tells me nothing about the person
>> object, and when I see it in a test, I don't know why name16 was
>> chosen instead of name15. UBUNTU_
>> Likewise, it would be good to have intent-revealing names for the
>> FIREFOX constants and the HOARY constants.
>
> I can do that, but it will require some research. It's one of the
> problems with sampledata that I don't know what the intent is.
>
At the very least make the UBUNTU_
I don't want to make you spend a whole heap of time figuring out why
all of this stuff gets used. However, I'm also a little worried that
the trend will continue. Perhaps we could work a little to avoid the
risk by adding some comments or something. I don't know.
>> My comment about the other error message applies here. I think it can
>> be dropped. (If archives have unhelpful __repr__ implementations, we
>> should fix that rather than working around it all the time in tests).
>
> They have the default repr, should we change that?
>
Don't bother. Someone else can when it comes time to actually debug.
>> Hmm. These assertions look very much like the ones previous. Is there
>> a domain-specific assertion method or Matcher to be had here?
>
> Probably, I'll take a look. I think there's only two uses, so it hadn't
> hit my refactoring threshold yet.
>
Yeah, maybe it's not needed.
>> I think this one needs a comment. What's an LFA?
>
> LibraryFileAlias, I thought it was standard parlance, should I expand the
> variable name instead?
>
Hmm, maybe it is. I don't do much with the librarian.
I have no strong opinion about the rename. Follow your heart.
cheers,
jml
| James Westby (james-w) wrote : | # |
Hi,
I have made all the requested changes.
I went a little further than necessary and created some matchers
rather than new assertion methods.
Thanks,
James
| James Westby (james-w) wrote : | # |
I tried to rename the sampledata variables as best I could, but for things
like "hoary", it just means "one bag of stuff as opposed to 'warty' which
is a different bag of stuff."
Thanks,
James
| Robert Collins (lifeless) wrote : | # |
Something that stood out to me, and perhaps jml's review mentions this
is that your new factory methods don't seem to have tests?
-Rob
| James Westby (james-w) wrote : | # |
On Mon, 02 Aug 2010 20:01:40 -0000, Robert Collins <email address hidden> wrote:
> Something that stood out to me, and perhaps jml's review mentions this
> is that your new factory methods don't seem to have tests?
It did, and they do now.
Thanks,
James

On Sun, Aug 1, 2010 at 12:06 AM, James Westby <email address hidden> wrote: reviewers)
> James Westby has proposed merging lp:~james-w/launchpad/no-more-sampledata-0 into lp:launchpad/devel.
>
> Requested reviews:
> Launchpad code reviewers (launchpad-
>
>
> Hi,
>
> This changes a bunch of soyuz tests to not use the sampledata.
>
Woot.
> It goes a bit further than that in that it restructures them to sampledata, and extended the factory
> have less setup and to create the things they need using the
> factory rather than SoyuzTestPublisher.
>
> In addition I moved some other code that is too deeply buried
> right now in to lp.testing.
> with some new methods to satisfy the needs of the changed code.
>
> Thanks,
>
Thank you.
The review below mostly picks up on some naming standard glitches and
points out some things that could be improved in the soyuz tests
(rather than in your changes). However, I've also got a couple of
questions about your changes and would very much appreciate hearing
back from you.
I also feel a little guilty about making so many petty comments when
you've done such a helpful thing. Please do not feel obliged to act on
all of my suggestions.
jml
> === modified file 'lib/lp/ soyuz/tests/ ppa.py' soyuz/tests/ ppa.py 2010-03-11 16:48:37 +0000 soyuz/tests/ ppa.py 2010-07-31 23:06:48 +0000 interfaces. binarypackagena me import IBinaryPackageN ameSet interfaces. publishing import ( ngPriority, PackagePublishi ngStatus) sampledata import ( DISTROSERIES_ NAME, I386_ARCHITECTU RE_NAME, MAIN_COMPONENT_ NAME, FIREFOX_ SOURCEPACKAGENA ME, MOZILLA_ FIREFOX_ SOURCEPACKAGEVE RSION, PERSON_ NAME, UBUNTU_TEAM_NAME)
> --- lib/lp/
> +++ lib/lp/
> @@ -25,6 +25,10 @@
> from lp.soyuz.
> from lp.soyuz.
> PackagePublishi
> +from lp.testing.
> + HOARY_
> + MOZILLA_
> + NAME16_
>
I'm very glad you've started putting things in the sampledata module.
Would it be possible to change these names to clarify the intent
behind their use?
For example, NAME16_PERSON_NAME tells me nothing about the person TEAM_MEMBER_ NAME might be better.
object, and when I see it in a test, I don't know why name16 was
chosen instead of name15. UBUNTU_
Likewise, it would be good to have intent-revealing names for the
FIREFOX constants and the HOARY constants.
> === modified file 'lib/lp/ soyuz/tests/ soyuz.py' soyuz/tests/ soyuz.py 2010-02-08 11:40:06 +0000 soyuz/tests/ soyuz.py 2010-07-31 23:06:48 +0000 model.publishin g import ( blishingHistory , blishingHistory ) sampledata import ( RE_NAME, LAUNCHPAD_ DBUSER_ NAME, DISTRIBUTION_ NAME, WARTY_DISTROSER IES_NAME, UPDATES_ SUITE_NAME)
> --- lib/lp/
> +++ lib/lp/
> @@ -27,14 +27,19 @@
> from lp.soyuz.
> SourcePackagePu
> BinaryPackagePu
> +from lp.testing.
> + CHROOT_LFA, CPROV_NAME, I386_ARCHITECTU
> + UBUNTU_
> + WARTY_
>
Ditto CPROV, LAUNCHPAD_ DBUSER_ NAME and WARTY_*
> === modified file 'lib/lp/ soyuz/tests/ test_archive. py' soyuz/tests/ test_archive. py 2010-07-21 07:45:50 +0000 soyuz/tests/ test_archive. py 2010-07-31 23:06:48 +0000 ionsInArchive( TestCaseWithFac tory): ssLayer
> --- lib/lp/
> +++ lib/lp/
...
> @@ -42,118 +41,110 @@
>
> class TestGetPublicat
>
> - layer = LaunchpadZopele
> -
> - def setUp(self):
> - ...