Code review comment for lp:~stub/launchpad/garbo

On Tue, Apr 7, 2009 at 8:11 PM, Brad Crittenden <email address hidden> wrote:

>> +    def test_OpenIDAssociationPruner(self, pruner=OpenIDAssociationPruner):
>> +        store_name = pruner.store_name
>> +        table_name = pruner.table_name
>> +        LaunchpadZopelessLayer.switchDbUser('testadmin')
>> +        store_selector = getUtility(IStoreSelector)
>> +        store = store_selector.get(store_name, MASTER_FLAVOR)
>> +        now = time.time()
>> +        # Create some associations in the past with lifetimes
>> +        for delta in range(0, 20):
>> +            store.execute("""
>> +                INSERT INTO %s (server_url, handle, issued, lifetime)
>> +                VALUES (%s, %s, %d, %d)
>> +                """ % (table_name, str(delta), str(delta), now-10, delta))
>> +        transaction.commit()
>> +
>> +        num_expired = store.execute("""
>> +            SELECT COUNT(*) FROM %s
>> +            WHERE issued + lifetime < %f
>> +            """ % (table_name, now)).get_one()[0]
>> +        self.failUnless(num_expired > 0)
>> +
>> +        self.runHourly()
>> +
>> +        LaunchpadZopelessLayer.switchDbUser('testadmin')
>> +        store = store_selector.get(store_name, MASTER_FLAVOR)
>> +        num_expired = store.execute("""
>> +            SELECT COUNT(*) FROM %s
>> +            WHERE issued + lifetime < %f
>> +            """ % (table_name, now)).get_one()[0]
>> +        self.failUnlessEqual(num_expired, 0)
>
> This test depends on all three parts completing within one second, the
> granularity of your delta.  While I'm having a hard time envisioning
> that being a problem it does seem like a potential spot for random
> failure.  Perhaps if you just state the assumption in a comment that
> will suffice and be a big clue if it should ever fail.

I thought I'd fixed the logic to stop that already. I have a
consistent 'now' at the top of the test. The first test checks that
there is at least one item expired at time 'now'. The second check
tests that there are 0 items expired at time 'now'. The only change I
see if things pause or run slow is self.runHourly(), and this just
means more items might get expired. The final check doesn't care,
because it is just ensuring that all the items at the test start time
where expired and it couldn't care less if more where expired.

So if my logic is correct (it is 4:30am), the problem is that I'm not
checking that items that should not have been expired have not been
expired.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/