launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

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.

>> > As we agreed on the call, it's renamed to slaveDialogueEnded.
>>
>> Thanks, although in code we use American spelling, which would make it
>> slaveDialogEnded.
>
> To avoid frustration on both sides of the Atlantic it's now
> slaveConversationEnded().
>

Thanks.

>> In the diff, you've made NewBuildersScanner inherit from TimeoutMixin.
>> AFAICT, it isn't necessary, and only confuses things since there's no
>> actual timeout involved.
>>
>> Instead of mixing in, you could do the following in the constructor:
>>
>>     def __init__(self, manager, clock=None):
>>         self.manager = manager
>>         if clock is None:
>>             clock = reactor
>>         self._clock = clock
>>
>> And then change scheduleScan to just do:
>>         return self._clock.callLater(self.SCAN_INTERVAL, self.scan)
>
> Gar, thanks.  I got too distracted while looking at the RunProcessWithTimeout
> example!
>
> My only concern with all of this, regardless of how it's done, is that there's
> always an untested line of code with this way of testing.  I can't think of a
> better way of doing it though.
>

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 :)

>> >> What you should be doing instead is always, always, always returning
>> >> Deferreds from things that make Deferreds or call something that
>> >> returns a Deferred.
>> >
>> > There's no need to tell *me* that now :)
>>
>> Heh heh.
>
> BTW, how does that pattern usually apply with a callLater() invocation?  Or
> doesn't it, and we pass the clock?
>

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).

>> Thanks. I've referred to it while replying.
>>
>> In summary:
>>   * I don't understand why getByActiveBuilder is on IBuildQueueSet
>> rather than being IBuilder.getActiveBuild().
>>   * slaveDialogueEnded => slaveDialogEnded
>>   * Don't inherit from TimeoutMixin in NewBuildersScanner.
>>
>> Almost there!
>>
>> jml
>
> http://pastebin.ubuntu.com/472219/
>
> How are we looking?
>

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

Thanks Julian.

jml
-- 
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.