Code review comment for lp:~thomir-deactivatedaccount/canonical-identity-provider/trunk-add-gpg-key-management

Hello Michael, Thommy,

I was a little bit surprised with the adding of testtools' TestCase and testtools.matchers to these tests. I understand the difference in using matchers vs standard TestCase assertion methods, and leaving aside the discussion whether is something we want or not, I would like us to talk about adding them to SSO or any other existing big project.

SSO is a large project with a lot of code, and in many past conversations the OLS team have agreed that given the amount of people working in every project, and given the importance of being able to switch between projects easily and fast, one of the things we value the most is consistency (on top of things that may be a nice to have in other contexts).

Starting using testtools for our unit tests is something that breaks consistency, and wasn't agreed at team level in advanced. The testtools dependency was there only for the acceptance tests.

Even more, using multiple inheritance on different base TestCases is something that has been proven to be a complete disaster in the mid term, we suffered from this in the Ubuntu One code where we had the Twisted TestCase, the Django TestCase, and the testtools TestCase as multiple base test cases. All the benefits a particular TestCase can give, are lost and become a burden when combining more than one as base classes.

For our Django projects, the OLS decision was to use the Django TestCase with a few extra, custom helpers defined. After a quick look, most matchers usage can be replaced with perfectly valid assertion from the Django TestCase:

self.assertThat(keys, HasLength(1))
self.assertThat(resp.content.decode('utf8'), Not(Contains('GPG Keys')))
self.assertThat(resp.status_code, Equals(200))

self.assertEqual(len(keys), 1)
self.assertEqual(resp.status_code, 200)
self.assertNotContains(resp, 'GPG Keys')

(note that the usage of assertContains/assertNotContains will also be clever about decoding the reponse, grabbing the text content, etc, something that otherwise is being made by hand and is a more error prone).

For other more complex matchers, like these:

self.assertThat(email, EmailContainsPGPBlock())
self.assertThat(keys[0]['fingerprint'], FingerprintEquals(encryption_fingerprint))

the existing methodology is to create custom assert helpers that isolate the specific logic needed for each case:

def assert_gpg_block_in_email(self, email)
def assert_fingerprint_encryption_in keys(self, keys)

In summary, I would kindly ask you to maintain consistency in line within the existing project, or at least bring this issue to the whole OLS team and reach consensus and have a migration plan before landing this to trunk.

Thank you.