On Fri, Sep 20, 2013 at 9:54 AM, Klaus Aehlig <[email protected]> wrote:
> > Only advancing time if _wait_fn is called is an oversimplification. > > It's not an oversimplification, it's beeing strict on the specification, > that the only way to guarantee that time passes is by calling the wait > function. This will catch all forms of busy wait, and it will catch > all forms of busy waiting, and it will catch the typical programming > error of assuming that just because we called some function, the > new value of the time must be different. > > > Actually, it's a corner case that two consecutive invocations of _time_fn > > return the same value, > > It is a corner case that does happen in practise. Over and over again. > > http://buildbot.ganeti.org/builders/tests-wheezy64/builds/594/steps/distcheck/logs/stdio > In fact, it's precisely this corner cases that made our tests flaky > and triggered the investigation that eventually lead to the discovery > of the bug fixed in the first patch. > > > but here that's the default. Calling _time_fn should > > also increase self.time, and there should be a test introduced which > tests > > for the special case that the time resolution between two calls of > _time_fn > > is too small to return a different value. > > If you really insist, I can add an additional test for the trivial case > that time passes when calling _time_fn. However, bugs usually are in the > corner cases. > A mocked _time_fn which does not advance time by it's own decreases coverage, so I really insist on restoring at least the old coverage. As I said, I'd add one or a few tests for the corner case (_time_fn() - _time_fn() == 0) and would let the other tests untouched, as they probably test for other cases which are not related to this at all. > > > Additionally, it would be possible to make those test (and the production > > code) cleaner by using Python Mock. With that you could just replace the > > implementation of time.time() and time.sleep(), thus avoiding to have the > > two additional named parameters _wait_fn and _time_fn (see > > ganeti.storage.drbd_unittest.py:429 for a related example). > > This is fixing a bug in 2.8. Python Mock isn't a dependency of > Ganeti until 2.9. > > Additionally, why mock out time.time if util.Retry is designed to accept > the timing and wait function as arguments? I didn't add these parameters, > there part of the interface for years. And it is perfectly sensible for > a test to use all arguments of an interfaces, even the optional ones. > In Ganeti, somehow the pattern of introducing additional parameters just for testing is quite common. _test_fn is such a parameter, whereas wait_fn is actually part of the interface and used by production code. IMHO, this practice is bad, because it makes the code less readable for no good reason. But you are right, that's not related to your patch, so changing it would be more like following the boy-scout rule of leaving the place cleaner behind than you found it, so feel free to go either way. BTW, the fix for the flaky test could go in 2.8, but the added and refactored tests in 2.9, right? > > -- > Klaus Aehlig > Google Germany GmbH, Dienerstr. 12, 80331 Muenchen > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
