On Wed, 04 Aug 2010 16:25:31 -0400, Curtis Hovey <curtis.ho...@canonical.com> 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 _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
