launchpad-dev team mailing list archive

[James Westby <jw+debian@xxxxxxxxxxxxxxx>]

On Wed, 04 Aug 2010 16:25:31 -0400, Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx> wrote:
> I landed a few changes because of this. I think everyone should be aware
> of two discoveries that concerned me.
> 
> 1. The while looking at one report on a naked object I saw a test
> description disagreed with a test result. I was really puzzled. Really,
> because I wrote all the code and I knew re-reading it that it was wrong.
> There was a typo in the attr assignment. Login with the wrapped object
> showed that that attr did not exist. Using the real attribute did make
> the test work as I expected: items orders from 'hot' to 'cold'
> 
> 2. While wrapping the milestone I saw factory.makeMilestone() rarely
> made a valid milestone. A milestone must have a product or distro
> series. No test with a distro milestone could render a milestone,
> release, or distribution view. We did not have a test that exercised
> these conditions with the factory made milestone, but lp engineers have
> tried to help users be creating milestones using SQL, and these same
> milestones definitely cause exceptions last year.

3. While adding tests to avoid regressions for the ones I was fixing I
found 5 classes which did not correctly implement the interfaces they
claimed to. I assumed it was standard practice to verifyObject() an
instance of each new class you wrote, but there seems to be a hole
here. The cases I found were minor, and I'm not sure how a major bug
would squeeze in here, but it still seems like a hole.

> It certainly stole a day from me. I think the registry team have
> branches that fixed or fix all the issues reported running registry
> tests.

Soyuz also has the SoyuzTestPublisher which was creating some objects
that weren't proxied. Fixing that is proving to be more time consuming
than any I have fixed in the factory.

I am fixing these each time I see a warning in the tests I run (when I
run a subset), so taking the warning away would mean that I would stop
doing that.

>From a complete test run earlier today we still have:

test_publishing.py complaints about removeSecurityProxy (now
ShouldThisBeUsingRemoveSecurityProxy). This can just be replaced with
removeSecurityProxy, and I have that in an in-progress branch here, but
it will be a while before that lands. Someone with a branch ready to
submit could fold that in. This is by far and away the most number of
lines in the test output.

makeSeries
makeBranchMergeProposal
makeCodeReviewVoteReference
makeSourcePackageRecipeBuildJob
makeTranslationMessage
makePOTMsgSet
makeTranslationImportQueueEntry

So there are not too many left to fix.

An alternative to turning it off would be to only produce one warning
per method, per run. That would still provide the warning but also be
much less output.

If we are going to ditch the proxy, or turn it off in tests, or
something, then could we please do it soon? Otherwise I will waste even
more hours of my time.

Thanks,

James