Merge lp:~robru/gwibber/account into lp:~barry/gwibber/py3

So barry, the thing to remember about what mardy just told us is this get_settings_iter may not quite be iterable in the way python expects, so the list comprehension in on_account_changed may explode when we give it real access to libaccounts outside of the over-mocked testsuite. I don't know how to test this properly at this point so we'll just have to remember and be aware of this when it comes time to write some integration tests when we're wrapping up the porting job.

The best solution of course would be to implement a python override in libaccounts so that it *is* an iterator in the way python expects (needs a __next__ method that raises StopIteration when the first False value is encountered), but I'm not sure how possible that would be to coordinate (I actually have experience writing python overrides for GI code, but I'm uncertain how this would impact feature freeze, and also coordinating them getting it released in time for our needs etc, might have to wait for next cycle).

Hi Robert,

Thanks again for all the awesome work on the Python 3 port of Gwibber. Here
are some detailed comments on the Accounts branch.

=== added file 'gwibber/gwibber/tests/test_account.py'
--- gwibber/gwibber/tests/test_account.py 1970-01-01 00:00:00 +0000
+++ gwibber/gwibber/tests/test_account.py 2012-08-28 21:31:33 +0000
> @@ -0,0 +1,100 @@
> +# Copyright (C) 2012 Canonical Ltd
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License version 2 as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +"""Test the Account class."""
> +
> +__all__ = [
> + 'TestAccount',
> + ]
> +
> +
> +import unittest
> +
> +from threading import Lock
> +
> +from gwibber.utils.account import Account
> +
> +try:
> + # Python 3.3
> + from unittest import mock
> +except ImportError:
> + import mock
> +
> +
> +class TestAccount(unittest.TestCase):
> + """Test Account class."""
> +
> + def test_basic_api(self):
> + fake_account_service = mock.MagicMock()
> + fake_account_service.get_account().get_settings_iter.return_value = iter([(True, 'key', 'val')])

PEP 8 recommends a maximum line length of 79 characters.

http://www.python.org/dev/peps/pep-0008/#maximum-line-length

There's ways to rewrite this code to fit, but I have another suggestion in a
moment..

> + fake_account_service.get_account().id = 'fake_id'
> + fake_account_service.get_service().get_name.return_value = 'fake_service'
> +
> + account = Account(fake_account_service)

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.

> + fake_account_service.get_auth_data.assert_called_once_with()
> +
> + fake_account_service.connect.assert_called_once_with('changed',
> + account.on_account_changed, fake_account_service.get_account())
> +
> + account['name'] = 'Fred'
> + self.assertEqual(account['name'], 'Fred')
> +
> + self.assertTrue('auth' in account)
> + self.assertTrue('id' in account['auth'])
> + self.assertTrue('method' in account['auth'])
> + self.assertTrue('mechanism' in account['auth'])
> + self.assertTrue('parameters' in account['auth'])
> + self.assertEqual(len(account['auth']), 4)
> +
> + self.assertTrue('service' in account)
> +
> + self.assertTrue('id' in account)

If you do create a fake, unmocked account_service object, then you won't need
to do the assertTrues here. You could actually test...

On Aug 28, 2012, at 02:35 PM, Robert Bruce Park wrote:

>The best solution of course would be to implement a python override in
>libaccounts so that it *is* an iterator in the way python expects (needs a
>__next__ method that raises StopIteration when the first False value is
>encountered), but I'm not sure how possible that would be to coordinate (I
>actually have experience writing python overrides for GI code, but I'm
>uncertain how this would impact feature freeze, and also coordinating them
>getting it released in time for our needs etc, might have to wait for next
>cycle).

Probably too late to do it for this cycle, but I would definitely suggest
filing a bug for next cycle! If you do, please subscribe me to it; I'd like
to work on the API for libaccounts too.

On 12-08-28 05:04 PM, Barry Warsaw wrote:
>> + fake_account_service.get_account().get_settings_iter.return_value = iter([(True, 'key', 'val')])
>
> PEP 8 recommends a maximum line length of 79 characters.
>
> http://www.python.org/dev/peps/pep-0008/#maximum-line-length

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.

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

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

>> + auth_data = account_service.get_auth_data()
>> + self['auth'] = {
>> + 'id': auth_data.get_credentials_id(),
>> + 'method': auth_data.get_method(),
>> + 'mechanism': auth_data.get_mechanism(),
>> + 'parameters': auth_data.get_parameters(),
>> + }
>
> An alternative style I sometimes like, which works if all the keys are Python
> identifiers (as is the case here), is something like this:
>
> self['auth'] = dict(
> id = auth_data.get_credentials_id(),
> method = auth_data.get_method(),
> mechanism = auth_data.get_mechanism(),
> parameters = auth_data.get_parameters(),
> )

Done.

>> + account = account_service.get_account()
>> + self.global_id = account.id
>> + service_name = account_serv...

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 (functi...

On 12-08-28 06:30 PM, Barry Warsaw wrote:
>> 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.

Yeah, part of it was that it actually took a couple different classes to
be faked so I had methods defining other classes to return and it became
quite cumersome. MagicMock just creates everything, well, magically, so
there's very little setup required ;-)

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

Ok, here's a mind-fuck for you ;-)

$ python3 -m timeit "tuple(x for x in range(100))"
100000 loops, best of 3: 9.37 usec per loop
$ python3 -m timeit "tuple([x for x in range(100)])"
100000 loops, best of 3: 5.94 usec per loop

$ python3 -m timeit "dict((x, x) for x in range(100))"
100000 loops, best of 3: 15.9 usec per loop
$ python3 -m timeit "dict([(x, x) for x in range(100)])"
100000 loops, best of 3: 15.1 usec per loop

I don't know why list comprehensions are 2x faster for tuple creation
but not dict creation, when I tested in the past I was only interested
in tuple creation. I assume the dict instantiation is doing something so
slow that it dwarfs the relative slowness of the generator. Here's
another python curiosity:

$ python3 -m timeit "[1,2,3]"
10000000 loops, best of 3: 0.0979 usec per loop
$ python3 -m timeit "(1,2,3)"
10000000 loops, best of 3: 0.024 usec per loop

Due to the above results I spent a lot of time converting my
performance-critical code to use tuples instead of lists, except that
those "slow" list brackets ended up being faster when I needed to do a
comprehension to generate my tuples ;-)

Don't get me started on performance tuning! I can do this all night!! :-P

Thanks. I finally merged this, although I did make the tests more granular.