[viff-devel] Broken unit tests

Marcel Keller mkeller at cs.au.dk
Fri Jul 17 02:56:28 PDT 2009


Hi Martin,

Martin Geisler wrote:
> All the tests in the buildbot are currently broken:
> 
>   http://buildbot.viff.dk/waterfall
> 
> It is because of this change of yours:
> 
> @@ -164,6 +166,18 @@
>          # the Runtime, since we want everybody to wait until all
>          # runtimes are ready.
>          self.runtimes.append(result)
> +        self.real_runtimes.append(runtime)
> +        self.i = 0
> +
> +        # This loop call should ensure the queues of the parties are
> +        # processed in a more or less fair manner. This is necessary
> +        # because we have only one reactor for all parties here.
> +        def loop_call():
> +            for runtime in self.real_runtimes[self.i:] + self.real_runtimes[:self.i]:
> +                runtime.process_deferred_queue()
> +                self.i = (self.i + 1) % len(self.real_runtimes)
> +
> +        reactor.setLoopCall(loop_call)

I think the problem is that after this change the unit tests need the 
reactor to be a ViffReactor, but it seems that the reactor is a 
SelectReactor. And this seems to be the case since your changeset "Make 
the VIFF reactor available to trial".

> I'm very confused about what exactly
> 
>   for x in xs[i:] + xs[:i]:
>     ...
>     i = (i + 1) % len(xs)
> 
> is supposed to do? After x has run through all xs (rotated i steps),
> then i will have been incremented by len(xs). But you do it mod len(xs)
> and so i comes out of the loop unchanged.

This solution is indeed not very elegant. The problem that has to be 
solved here is the following: In a normal run every player (i.e. every 
runtime) has its own reactor and this reactor runs 
process_deferred_queue() of one runtime. In the unit tests however, 
there is one reactor for n runtimes. Therefore the reactor has to call 
process_deferred_queue() of every runtime. Now, if we just would iterate 
as usual over the runtimes this would lead to very unbalanced and 
therefore slower execution, because the loop call is called recursively. 
So this construct is to ensure that every time loop_call() is called, 
another runtime has first priority.

> The xs[i:] + xs[:i] construct is at best uncommon and at worst wasted
> work. Instead of rotating the list, it should be enough to start at an
> offset (i) and work your way through the list with proper wrapping at
> the end.

You're right.

> Also, the fact that you have both self.runtimes and self.real_runtimes
> smells funny :-) It's as if you're circumventing the natural scopes of
> the Deferreds by storing a reference to the runtimes "on the side".
> 
> The list of Deferreds in self.runtimes ought to be enough -- can you not
> add callbacks to that list if you need something to happen when the
> runtimes are ready?

Yes, it just was too lazy.

Best reagards,
Marcel



More information about the viff-devel mailing list