On Aug 28, 2012, at 05:31 PM, Robert Bruce Park wrote:

>Yeah, sorry. Somewhere along the line I fell into the bad habit of regarding
>my tests as just "test code" and not "real code" and thus I've become less
>stringent about adhering to PEP8 in the tests.

No worries!

>> I'm not actually sure you need to mock this to get decent tests.  Since
>> you're passing account_service into the constructor, the test module could
>> just create a simple class that provides the required API.  Instantiate
>> that and pass it into the constructor, then have it respond the way you
>> want it to to the various calls.
>
>I had actually written a fake class originally, and there were dozens of
>lines of stub methods on it and I found it was actually quite cumbersome. The
>mock only required a few lines worth of return_value defintions in order to
>be serviceable, so I definitely prefer the mock.

Interesting!  Okay.

>>> +        fake_account_service.get_enabled.return_value = True
>>> +        self.assertTrue(account.enabled())
>>> +        fake_account_service.get_enabled.return_value = False
>>> +        self.assertFalse(account.enabled())
>>
>> Also, I typically prefer really tight, focused tests for each bit of code
>> you're testing.  That way if you get a failure because the
>> login_lock.acquire() returned False, you'll know immediately that the test
>> didn't fail because, say account['id'] had the wrong value.
>
>But the beauty of unittest is that it tells you precisely which test failed
>and why, unlike using bare 'assert' statements. I understand this test is a
>bit long, I guess I'll leave it to you to split up.
>
>> You wouldn't need to test each individual key in account['auth'], because
>> those are logically the same bit of code.  But this test could be split up
>> into several smaller tests that each flex a single logical chunk of the
>> constructor.
>
>The other reason that it's all crammed into one test is that I didn't want to
>have to duplicate the mock creation code needlessly. I guess it could be
>promoted to a module global and then referenced from different tests if you
>cared to tackle that though.

TestCases can have both a per-test setup and teardown and a per-class setup
and teardown, so all the mock setups could go into TestAccount.setUp().  I'll
give that a shot along with splitting up the tests.

>>> +    def on_account_changed(self, account_service, account):
>>> +        self.update(
>>> +            [(key, value)
>>> +            for (success, key, value)
>>> +            in account.get_settings_iter('gwibber/')
>>> +            if success])
>>
>> You don't need a list comprehension here because a generator comprehension
>> will work just fine.  There's no need to create a temporary concrete list
>> that just gets thrown away.
>
>Well, the last time I was profiling code, it turned out that generator
>expressions were actually significantly slower than list comprehensions
>(though admittedly this was in py2.7, I'm not sure if it's changed since
>then). Generators have the benefit of not using up a lot of RAM, but they do
>so by invoking python functions repeatedly, which is a well known way to do
>things very slowly (function invocation is known to have a high overhead). So
>here you just have to choose whether you want to be wasteful of CPU time, or
>of RAM. On my 8GB machine I typically prefer to waste RAM ;-)

Okay, I'm seriously intrigued, so I timed the two alternatives in both 2.7 and
3.2.  os.environ is just a convenient mapping, but I had to convert it to a
regular dictionary in the setup, otherwise the different implementations of
os.environ cloud the results.

% python3 -m timeit -s "import os; d = dict(); d.update(os.environ)" "dict([(key, value) for (key, value) in d.items()])"
100000 loops, best of 3: 10.4 usec per loop

% python3 -m timeit -s "import os; d = dict(); d.update(os.environ)" "dict((key, value) for (key, value) in d.items())"
100000 loops, best of 3: 10.1 usec per loop

So in Python 3.2, the generator comprehension is slightly faster, but probably
not significant enough to notice.  I was interested in your Python 2.7
results, so I timed it there too:

% python2.7 -m timeit -s "import os; d = dict(); d.update(os.environ)" "dict([(key, value) for (key, value) in d.items()])"
100000 loops, best of 3: 11.3 usec per loop

% python2.7 -m timeit -s "import os; d = dict(); d.update(os.environ)" "dict((key, value) for (key, value) in d.items())"
100000 loops, best of 3: 11.4 usec per loop

A slight bit slower, but again, nothing significant.  Nice that the Python 3.2
version is faster than 2.7 in both cases.

> > Also, stylistically, I'd probably format this a bit
>> differently, e.g.
>>
>>          self.update(
>>              (key, value)
>>              for (success, key, value) in account.get_settings_iter('gwibber/')
>>              if success)
>
>I find the wildly differing line lengths to be unattractive here, personally.

Yeah, WIBNI account.get_settings were a proper iterator? :)

>
>>> +    def enabled(self):
>>> +        return self.account_service.get_enabled()
>>
>> What do you think about using a property here?  E.g. adding
>>
>>          @property
>>
>> then using it like: `if account.enabled` instead of having to call it.
>
>I'm indifferent, because you're still invoking a method when you access that
>attribute, so it's not like you're saving any processing time. It's purely
>stylistic. If you're going to change this make sure you grep through and
>change everything else that calls enabled(), too.

+1

>> I'm getting merge conflicts with trunk now.  This shows up in the merge
>> proposal too.
>
>I'm not sure why that happened. The conflict is resolved by deleting the
>Account class out of dispatcher.py. Or not, considering we're going to toss
>that file soon(ish) anyway.
>
>Anyways, I've pushed some of your changes, feel free to do some of the other
>ones if you prefer. Thanks for the review!

Sounds good!