launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

On Monday 02 August 2010 16:01:42 Jonathan Lange wrote:
> On Mon, Aug 2, 2010 at 3:28 PM, Julian Edwards
> 
> <julian.edwards@xxxxxxxxxxxxx> wrote:
> > On Monday 02 August 2010 14:15:54 you wrote:
> >> >> The comment makes me think there should be a method called
> >> >> IBuilder.getActiveBuild().
> >> > 
> >> > I've made IBuildQueueSet.getByActiveBuilder()
> >> 
> >> Thanks. Why that and not IBuilder.getActiveBuild()?
> > 
> > Because it's returning BuildQueue objects, not builders.
> 
> By that reasoning we should have an IBooleanSet that holds all of our
> predicate functions.
> 
> Don't feel obliged to change it though.

GRAR.  Ignore me, I've been consistently mis-reading that as IBuilder*Set*

:(

I'll move it.

> There's another way of doing it, but I don't think it's better.
> 
> Instead of having an optional parameter, you can have something like:
>  self._clock = getUtility(IClock)
> 
> And then have two different sets of ZCML: one for unit tests and one
> for real code. Of course, all this does is hide the "untested line of
> code" problem, since now you have untested ZCML.
> 
> These reflections might help alleviate your concerns:
>  * If you have integration tests, then that line of code does get
> exercised, even though it lacks unit tests.
>  * It's really the least of your test coverage problems :)

Yeah, it was more of a general thing than worrying about it here :)

> The normal way of testing Twisted code looks like this:
> 
> def test_foo(self):
>   d = self.xmlrpc_service.callRemote("countBranches")
>   # Equivalent to d.addCallback(lambda count: self.assertEqual(count, 5))
>   d.addCallback(self.assertEqual, 5)
>   return d
> 
> Trial will wait for the returned Deferred to fire using horrible evil
> crap that you really don't want to know about. If the Deferred
> errbacks, it'll fail the test. Otherwise, it's all good.
> 
> For Twisted code that involves time, you almost always want to pass a
> clock in and advance it manually. Often tests like this don't actually
> need to have a Deferred object. Your NewBuildersScanner tests are good
> examples of this.
> 
> When you do want to mix things up, you just combine the two. Say for
> example you want to write a function that returns a Deferred that will
> fire in a given number of seconds from now, and you want to test it:
> 
> def test_run_later_success(self):
>     clock = Clock()
>     d = run_later(clock, 5, math.sqrt, 49)
>     d.addCallback(self.assertEqual, 7)
>     clock.advance(7)
>     return d
> 
> (There's also another trick you can do here, but you're probably bored by
> now).

Not at all.  I am enjoying the useful advice.

> Good to go (even gooderer if getByActiveBuilder moves to
> IBuilder.getActiveBuild()).

I'll move it.  Thanks a million for the patient review.

J

-- 
https://code.launchpad.net/~julian-edwards/launchpad/buildd-manager-parallel-scan/+merge/30672
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/buildd-manager-parallel-scan into lp:launchpad/devel.